From 7de0486e2e67a17e4cdcc881c7f3bd3fd1a66fb6 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Tue, 23 Jan 2024 12:47:13 +0100 Subject: [PATCH] layout: Count word separators as justification opportunities when trimming whitespace (#31161) Before counting whitepsace-only `GlyphStore`s where counted as a single justification opportunity when trimming whitespace from the front and back of lines. This isn't correct, instead count the actual number of word seperators of the trimmed `GlyphStore`s. These two counts can be different in the case where whitespace collapse isn't happening yet (flexbox). In addition, using word seperators means the code is making less assumptions about the contents of the line and is more robust. This fixes some crashes in flexbox tests on debug builds. Co-authored-by: Rakhi Sharma --- components/gfx/text/glyph.rs | 6 +++--- components/layout_2020/flow/inline.rs | 14 ++++++++------ components/layout_2020/flow/line.rs | 24 ++++++++++++++---------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index 727e693726a..98dc39bc0e4 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -434,7 +434,7 @@ pub struct GlyphStore { /// A cache of the number of word separators in the entire glyph store. /// See . - total_word_separators: i32, + total_word_separators: usize, /// Used to check if fast path should be used in glyph iteration. has_detailed_glyphs: bool, @@ -476,7 +476,7 @@ impl<'a> GlyphStore { } #[inline] - pub fn total_word_separators(&self) -> i32 { + pub fn total_word_separators(&self) -> usize { self.total_word_separators } @@ -622,7 +622,7 @@ impl<'a> GlyphStore { #[inline] pub fn advance_for_byte_range(&self, range: &Range, extra_word_spacing: Au) -> Au { if range.begin() == ByteIndex(0) && range.end() == self.len() { - self.total_advance + extra_word_spacing * self.total_word_separators + self.total_advance + extra_word_spacing * (self.total_word_separators as i32) } else if !self.has_detailed_glyphs { self.advance_for_byte_range_simple_glyphs(range, extra_word_spacing) } else { diff --git a/components/layout_2020/flow/inline.rs b/components/layout_2020/flow/inline.rs index 6b9e8669468..6a2b7cf879e 100644 --- a/components/layout_2020/flow/inline.rs +++ b/components/layout_2020/flow/inline.rs @@ -179,13 +179,14 @@ impl LineUnderConstruction { // > as well as any trailing U+1680   OGHAM SPACE MARK whose white-space // > property is normal, nowrap, or pre-line. let mut whitespace_trimmed = Length::zero(); - let mut spaces_trimmed = 0; + let mut word_seperators_trimmed = 0; for item in self.line_items.iter_mut().rev() { - if !item.trim_whitespace_at_end(&mut whitespace_trimmed, &mut spaces_trimmed) { + if !item.trim_whitespace_at_end(&mut whitespace_trimmed, &mut word_seperators_trimmed) { break; } } - self.justification_opportunities -= spaces_trimmed; + + self.justification_opportunities -= word_seperators_trimmed; whitespace_trimmed } } @@ -399,14 +400,15 @@ impl UnbreakableSegmentUnderConstruction { /// This prevents whitespace from being added to the beginning of a line. fn trim_leading_whitespace(&mut self) { let mut whitespace_trimmed = Length::zero(); - let mut spaces_trimmed = 0; + let mut word_seperators_trimmed = 0; for item in self.line_items.iter_mut() { - if !item.trim_whitespace_at_start(&mut whitespace_trimmed, &mut spaces_trimmed) { + if !item.trim_whitespace_at_start(&mut whitespace_trimmed, &mut word_seperators_trimmed) + { break; } } self.inline_size -= whitespace_trimmed; - self.justification_opportunities -= spaces_trimmed; + self.justification_opportunities -= word_seperators_trimmed; } /// Prepare this segment for placement on a new and empty line. This happens when the diff --git a/components/layout_2020/flow/line.rs b/components/layout_2020/flow/line.rs index 42df588538e..0c0a4c026de 100644 --- a/components/layout_2020/flow/line.rs +++ b/components/layout_2020/flow/line.rs @@ -120,11 +120,11 @@ impl LineItem { pub(super) fn trim_whitespace_at_end( &mut self, whitespace_trimmed: &mut Length, - spaces_trimmed: &mut usize, + word_seperators_trimmed: &mut usize, ) -> bool { match self { LineItem::TextRun(ref mut item) => { - item.trim_whitespace_at_end(whitespace_trimmed, spaces_trimmed) + item.trim_whitespace_at_end(whitespace_trimmed, word_seperators_trimmed) }, LineItem::StartInlineBox(_) => true, LineItem::EndInlineBox => true, @@ -137,11 +137,11 @@ impl LineItem { pub(super) fn trim_whitespace_at_start( &mut self, whitespace_trimmed: &mut Length, - spaces_trimmed: &mut usize, + word_separators_trimmed: &mut usize, ) -> bool { match self { LineItem::TextRun(ref mut item) => { - item.trim_whitespace_at_start(whitespace_trimmed, spaces_trimmed) + item.trim_whitespace_at_start(whitespace_trimmed, word_separators_trimmed) }, LineItem::StartInlineBox(_) => true, LineItem::EndInlineBox => true, @@ -165,7 +165,7 @@ impl TextRunLineItem { fn trim_whitespace_at_end( &mut self, whitespace_trimmed: &mut Length, - spaces_trimmed: &mut usize, + word_seperators_trimmed: &mut usize, ) -> bool { if self .parent_style @@ -184,11 +184,13 @@ impl TextRunLineItem { .map(|offset_from_end| self.text.len() - offset_from_end); let first_whitespace_index = index_of_last_non_whitespace.unwrap_or(0); - *spaces_trimmed += self.text.len() - first_whitespace_index; *whitespace_trimmed += self .text .drain(first_whitespace_index..) - .map(|glyph| Length::from(glyph.total_advance())) + .map(|glyph| { + *word_seperators_trimmed += glyph.total_word_separators(); + Length::from(glyph.total_advance()) + }) .sum(); // Only keep going if we only encountered whitespace. @@ -198,7 +200,7 @@ impl TextRunLineItem { fn trim_whitespace_at_start( &mut self, whitespace_trimmed: &mut Length, - spaces_trimmed: &mut usize, + word_separators_trimmed: &mut usize, ) -> bool { if self .parent_style @@ -215,11 +217,13 @@ impl TextRunLineItem { .position(|glyph| !glyph.is_whitespace()) .unwrap_or(self.text.len()); - *spaces_trimmed += index_of_first_non_whitespace; *whitespace_trimmed += self .text .drain(0..index_of_first_non_whitespace) - .map(|glyph| Length::from(glyph.total_advance())) + .map(|glyph| { + *word_separators_trimmed += glyph.total_word_separators(); + Length::from(glyph.total_advance()) + }) .sum(); // Only keep going if we only encountered whitespace.