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 <atbrakhi@igalia.com>
This commit is contained in:
Martin Robinson 2024-01-23 12:47:13 +01:00 committed by GitHub
parent 1d67aa44ca
commit 7de0486e2e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 25 additions and 19 deletions

View file

@ -434,7 +434,7 @@ pub struct GlyphStore {
/// A cache of the number of word separators in the entire glyph store. /// A cache of the number of word separators in the entire glyph store.
/// See <https://drafts.csswg.org/css-text/#word-separator>. /// See <https://drafts.csswg.org/css-text/#word-separator>.
total_word_separators: i32, total_word_separators: usize,
/// Used to check if fast path should be used in glyph iteration. /// Used to check if fast path should be used in glyph iteration.
has_detailed_glyphs: bool, has_detailed_glyphs: bool,
@ -476,7 +476,7 @@ impl<'a> GlyphStore {
} }
#[inline] #[inline]
pub fn total_word_separators(&self) -> i32 { pub fn total_word_separators(&self) -> usize {
self.total_word_separators self.total_word_separators
} }
@ -622,7 +622,7 @@ impl<'a> GlyphStore {
#[inline] #[inline]
pub fn advance_for_byte_range(&self, range: &Range<ByteIndex>, extra_word_spacing: Au) -> Au { pub fn advance_for_byte_range(&self, range: &Range<ByteIndex>, extra_word_spacing: Au) -> Au {
if range.begin() == ByteIndex(0) && range.end() == self.len() { 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 { } else if !self.has_detailed_glyphs {
self.advance_for_byte_range_simple_glyphs(range, extra_word_spacing) self.advance_for_byte_range_simple_glyphs(range, extra_word_spacing)
} else { } else {

View file

@ -179,13 +179,14 @@ impl LineUnderConstruction {
// > as well as any trailing U+1680 OGHAM SPACE MARK whose white-space // > as well as any trailing U+1680 OGHAM SPACE MARK whose white-space
// > property is normal, nowrap, or pre-line. // > property is normal, nowrap, or pre-line.
let mut whitespace_trimmed = Length::zero(); 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() { 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; break;
} }
} }
self.justification_opportunities -= spaces_trimmed;
self.justification_opportunities -= word_seperators_trimmed;
whitespace_trimmed whitespace_trimmed
} }
} }
@ -399,14 +400,15 @@ impl UnbreakableSegmentUnderConstruction {
/// This prevents whitespace from being added to the beginning of a line. /// This prevents whitespace from being added to the beginning of a line.
fn trim_leading_whitespace(&mut self) { fn trim_leading_whitespace(&mut self) {
let mut whitespace_trimmed = Length::zero(); 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() { 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; break;
} }
} }
self.inline_size -= whitespace_trimmed; 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 /// Prepare this segment for placement on a new and empty line. This happens when the

View file

@ -120,11 +120,11 @@ impl LineItem {
pub(super) fn trim_whitespace_at_end( pub(super) fn trim_whitespace_at_end(
&mut self, &mut self,
whitespace_trimmed: &mut Length, whitespace_trimmed: &mut Length,
spaces_trimmed: &mut usize, word_seperators_trimmed: &mut usize,
) -> bool { ) -> bool {
match self { match self {
LineItem::TextRun(ref mut item) => { 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::StartInlineBox(_) => true,
LineItem::EndInlineBox => true, LineItem::EndInlineBox => true,
@ -137,11 +137,11 @@ impl LineItem {
pub(super) fn trim_whitespace_at_start( pub(super) fn trim_whitespace_at_start(
&mut self, &mut self,
whitespace_trimmed: &mut Length, whitespace_trimmed: &mut Length,
spaces_trimmed: &mut usize, word_separators_trimmed: &mut usize,
) -> bool { ) -> bool {
match self { match self {
LineItem::TextRun(ref mut item) => { 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::StartInlineBox(_) => true,
LineItem::EndInlineBox => true, LineItem::EndInlineBox => true,
@ -165,7 +165,7 @@ impl TextRunLineItem {
fn trim_whitespace_at_end( fn trim_whitespace_at_end(
&mut self, &mut self,
whitespace_trimmed: &mut Length, whitespace_trimmed: &mut Length,
spaces_trimmed: &mut usize, word_seperators_trimmed: &mut usize,
) -> bool { ) -> bool {
if self if self
.parent_style .parent_style
@ -184,11 +184,13 @@ impl TextRunLineItem {
.map(|offset_from_end| self.text.len() - offset_from_end); .map(|offset_from_end| self.text.len() - offset_from_end);
let first_whitespace_index = index_of_last_non_whitespace.unwrap_or(0); let first_whitespace_index = index_of_last_non_whitespace.unwrap_or(0);
*spaces_trimmed += self.text.len() - first_whitespace_index;
*whitespace_trimmed += self *whitespace_trimmed += self
.text .text
.drain(first_whitespace_index..) .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(); .sum();
// Only keep going if we only encountered whitespace. // Only keep going if we only encountered whitespace.
@ -198,7 +200,7 @@ impl TextRunLineItem {
fn trim_whitespace_at_start( fn trim_whitespace_at_start(
&mut self, &mut self,
whitespace_trimmed: &mut Length, whitespace_trimmed: &mut Length,
spaces_trimmed: &mut usize, word_separators_trimmed: &mut usize,
) -> bool { ) -> bool {
if self if self
.parent_style .parent_style
@ -215,11 +217,13 @@ impl TextRunLineItem {
.position(|glyph| !glyph.is_whitespace()) .position(|glyph| !glyph.is_whitespace())
.unwrap_or(self.text.len()); .unwrap_or(self.text.len());
*spaces_trimmed += index_of_first_non_whitespace;
*whitespace_trimmed += self *whitespace_trimmed += self
.text .text
.drain(0..index_of_first_non_whitespace) .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(); .sum();
// Only keep going if we only encountered whitespace. // Only keep going if we only encountered whitespace.