From 0f983cd11f0442d5e6ed756cf22291d12acd8924 Mon Sep 17 00:00:00 2001 From: Ulf Nilsson Date: Sun, 8 May 2016 18:38:09 +0200 Subject: [PATCH] Make `text-align: justify` incremental layout safe --- components/gfx/paint_context.rs | 6 +- components/gfx/text/glyph.rs | 112 ++++++++++-------- components/gfx/text/text_run.rs | 6 +- components/layout/fragment.rs | 26 ++-- components/layout/inline.rs | 30 ++--- components/layout/webrender_helpers.rs | 6 +- .../html/text-align-justify-001.htm.ini | 3 - 7 files changed, 97 insertions(+), 92 deletions(-) delete mode 100644 tests/wpt/metadata-css/css-text-3_dev/html/text-align-justify-001.htm.ini diff --git a/components/gfx/paint_context.rs b/components/gfx/paint_context.rs index 626cc77b3929..3d4283c72df7 100644 --- a/components/gfx/paint_context.rs +++ b/components/gfx/paint_context.rs @@ -1799,7 +1799,11 @@ impl ScaledFontExtensionMethods for ScaledFont { for slice in run.natural_word_slices_in_visual_order(range) { for glyph in slice.glyphs.iter_glyphs_for_byte_range(&slice.range) { - let glyph_advance = glyph.advance(); + let glyph_advance = if glyph.char_is_space() { + glyph.advance() + run.extra_word_spacing + } else { + glyph.advance() + }; if !slice.glyphs.is_whitespace() { let glyph_offset = glyph.offset().unwrap_or(Point2D::zero()); let azglyph = struct__AzGlyph { diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index 161282d2f4ec..3f32b248c43d 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -71,13 +71,14 @@ pub type GlyphId = u32; // TODO: make this more type-safe. -const FLAG_CHAR_IS_SPACE: u32 = 0x40000000; -const FLAG_IS_SIMPLE_GLYPH: u32 = 0x80000000; +const FLAG_CHAR_IS_SPACE: u32 = 0x40000000; +const FLAG_CHAR_IS_SPACE_SHIFT: u32 = 30; +const FLAG_IS_SIMPLE_GLYPH: u32 = 0x80000000; // glyph advance; in Au's. -const GLYPH_ADVANCE_MASK: u32 = 0x3FFF0000; -const GLYPH_ADVANCE_SHIFT: u32 = 16; -const GLYPH_ID_MASK: u32 = 0x0000FFFF; +const GLYPH_ADVANCE_MASK: u32 = 0x3FFF0000; +const GLYPH_ADVANCE_SHIFT: u32 = 16; +const GLYPH_ID_MASK: u32 = 0x0000FFFF; // Non-simple glyphs (more than one glyph per char; missing glyph, // newline, tab, large advance, or nonzero x/y offsets) may have one @@ -371,6 +372,15 @@ impl<'a> GlyphInfo<'a> { } } } + + pub fn char_is_space(self) -> bool { + let (store, entry_i) = match self { + GlyphInfo::Simple(store, entry_i) => (store, entry_i), + GlyphInfo::Detail(store, entry_i, _) => (store, entry_i), + }; + + store.char_is_space(entry_i) + } } /// Stores the glyph data belonging to a text run. @@ -406,6 +416,8 @@ pub struct GlyphStore { /// A cache of the advance of the entire glyph store. total_advance: Au, + /// A cache of the number of spaces in the entire glyph store. + total_spaces: i32, /// Used to check if fast path should be used in glyph iteration. has_detailed_glyphs: bool, @@ -432,6 +444,7 @@ impl<'a> GlyphStore { entry_buffer: vec![GlyphEntry::initial(); length], detail_store: DetailedGlyphStore::new(), total_advance: Au(0), + total_spaces: 0, has_detailed_glyphs: false, is_whitespace: is_whitespace, is_rtl: is_rtl, @@ -450,20 +463,21 @@ impl<'a> GlyphStore { pub fn finalize_changes(&mut self) { self.detail_store.ensure_sorted(); - self.cache_total_advance() + self.cache_total_advance_and_spaces() } #[inline(never)] - fn cache_total_advance(&mut self) { + fn cache_total_advance_and_spaces(&mut self) { let mut total_advance = Au(0); + let mut total_spaces = 0; for glyph in self.iter_glyphs_for_byte_range(&Range::new(ByteIndex(0), self.len())) { - total_advance = total_advance + glyph.advance() + total_advance = total_advance + glyph.advance(); + if glyph.char_is_space() { + total_spaces += 1; + } } - self.total_advance = total_advance - } - - pub fn total_advance(&self) -> Au { - self.total_advance + self.total_advance = total_advance; + self.total_spaces = total_spaces; } /// Adds a single glyph. @@ -538,27 +552,35 @@ impl<'a> GlyphStore { } #[inline] - pub fn advance_for_byte_range(&self, range: &Range) -> Au { + 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 + self.total_advance + extra_word_spacing * self.total_spaces } else if !self.has_detailed_glyphs { - self.advance_for_byte_range_simple_glyphs(range) + self.advance_for_byte_range_simple_glyphs(range, extra_word_spacing) } else { - self.advance_for_byte_range_slow_path(range) + self.advance_for_byte_range_slow_path(range, extra_word_spacing) } } #[inline] - pub fn advance_for_byte_range_slow_path(&self, range: &Range) -> Au { + pub fn advance_for_byte_range_slow_path(&self, range: &Range, extra_word_spacing: Au) -> Au { self.iter_glyphs_for_byte_range(range) - .fold(Au(0), |advance, glyph| advance + glyph.advance()) + .fold(Au(0), |advance, glyph| { + if glyph.char_is_space() { + advance + glyph.advance() + extra_word_spacing + } else { + advance + glyph.advance() + } + }) } #[inline] #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] - fn advance_for_byte_range_simple_glyphs(&self, range: &Range) -> Au { - let mask = u32x4::splat(GLYPH_ADVANCE_MASK); + fn advance_for_byte_range_simple_glyphs(&self, range: &Range, extra_word_spacing: Au) -> Au { + let advance_mask = u32x4::splat(GLYPH_ADVANCE_MASK); + let space_flag_mask = u32x4::splat(FLAG_CHAR_IS_SPACE); let mut simd_advance = u32x4::splat(0); + let mut simd_spaces = u32x4::splat(0); let begin = range.begin().to_usize(); let len = range.length().to_usize(); let num_simd_iterations = len / 4; @@ -566,10 +588,11 @@ impl<'a> GlyphStore { let buf = self.transmute_entry_buffer_to_u32_buffer(); for i in 0..num_simd_iterations { - let mut v = u32x4::load(buf, begin + i * 4); - v = v & mask; - v = v >> GLYPH_ADVANCE_SHIFT; - simd_advance = simd_advance + v; + let v = u32x4::load(buf, begin + i * 4); + let advance = (v & advance_mask) >> GLYPH_ADVANCE_SHIFT; + let spaces = (v & space_flag_mask) >> FLAG_CHAR_IS_SPACE_SHIFT; + simd_advance = simd_advance + advance; + simd_spaces = simd_spaces + spaces; } let advance = @@ -577,18 +600,27 @@ impl<'a> GlyphStore { simd_advance.extract(1) + simd_advance.extract(2) + simd_advance.extract(3)) as i32; - let mut leftover = Au(0); + let spaces = + (simd_spaces.extract(0) + + simd_spaces.extract(1) + + simd_spaces.extract(2) + + simd_spaces.extract(3)) as i32; + let mut leftover_advance = Au(0); + let mut leftover_spaces = 0; for i in leftover_entries..range.end().to_usize() { - leftover = leftover + self.entry_buffer[i].advance(); + leftover_advance = leftover_advance + self.entry_buffer[i].advance(); + if self.entry_buffer[i].char_is_space() { + leftover_spaces += 1; + } } - Au(advance) + leftover + Au(advance) + leftover_advance + extra_word_spacing * (spaces + leftover_spaces) } /// When SIMD isn't available (non-x86_x64/aarch64), fallback to the slow path. #[inline] #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))] - fn advance_for_byte_range_simple_glyphs(&self, range: &Range) -> Au { - self.advance_for_byte_range_slow_path(range) + fn advance_for_byte_range_simple_glyphs(&self, range: &Range, extra_word_spacing: Au) -> Au { + self.advance_for_byte_range_slow_path(range, extra_word_spacing) } /// Used for SIMD. @@ -612,26 +644,6 @@ impl<'a> GlyphStore { } spaces } - - pub fn distribute_extra_space_in_range(&mut self, range: &Range, space: f64) { - debug_assert!(space >= 0.0); - if range.is_empty() { - return - } - for index in range.each_index() { - // TODO(pcwalton): Handle spaces that are detailed glyphs -- these are uncommon but - // possible. - let entry = &mut self.entry_buffer[index.to_usize()]; - if entry.is_simple() && entry.char_is_space() { - // FIXME(pcwalton): This can overflow for very large font-sizes. - let advance = - ((entry.value & GLYPH_ADVANCE_MASK) >> GLYPH_ADVANCE_SHIFT) + - Au::from_f64_px(space).0 as u32; - entry.value = (entry.value & !GLYPH_ADVANCE_MASK) | - (advance << GLYPH_ADVANCE_SHIFT); - } - } - } } impl fmt::Debug for GlyphStore { diff --git a/components/gfx/text/text_run.rs b/components/gfx/text/text_run.rs index 7dd0b027839f..af8dba7b8a09 100644 --- a/components/gfx/text/text_run.rs +++ b/components/gfx/text/text_run.rs @@ -33,6 +33,7 @@ pub struct TextRun { /// The glyph runs that make up this text run. pub glyphs: Arc>, pub bidi_level: u8, + pub extra_word_spacing: Au, } impl Drop for TextRun { @@ -188,6 +189,7 @@ impl<'a> TextRun { actual_pt_size: font.actual_pt_size, glyphs: Arc::new(glyphs), bidi_level: bidi_level, + extra_word_spacing: Au(0), } } @@ -247,7 +249,7 @@ impl<'a> TextRun { // TODO(Issue #98): using inter-char and inter-word spacing settings when measuring text self.natural_word_slices_in_range(range) .fold(Au(0), |advance, slice| { - advance + slice.glyphs.advance_for_byte_range(&slice.range) + advance + slice.glyphs.advance_for_byte_range(&slice.range, self.extra_word_spacing) }) } @@ -259,7 +261,7 @@ impl<'a> TextRun { pub fn metrics_for_slice(&self, glyphs: &GlyphStore, slice_range: &Range) -> RunMetrics { - RunMetrics::new(glyphs.advance_for_byte_range(slice_range), + RunMetrics::new(glyphs.advance_for_byte_range(slice_range, self.extra_word_spacing), self.font_metrics.ascent, self.font_metrics.descent) } diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index e0468b2f7c20..23fe90d7eeda 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -1751,23 +1751,23 @@ impl Fragment { /// Restore any whitespace that was stripped from a text fragment, and recompute inline metrics /// if necessary. pub fn reset_text_range_and_inline_size(&mut self) { - match &mut self.specific { - &mut SpecificFragmentInfo::ScannedText(ref mut info) => { - // FIXME (mbrubeck): Do we need to restore leading too? - let range_end = info.range_end_including_stripped_whitespace; - if info.range.end() == range_end { - return - } - info.range.extend_to(range_end); - info.content_size.inline = info.run.metrics_for_range(&info.range).advance_width; - self.border_box.size.inline = info.content_size.inline + - self.border_padding.inline_start_end(); + if let SpecificFragmentInfo::ScannedText(ref mut info) = self.specific { + if info.run.extra_word_spacing != Au(0) { + Arc::make_mut(&mut info.run).extra_word_spacing = Au(0); } - _ => {} + + // FIXME (mbrubeck): Do we need to restore leading too? + let range_end = info.range_end_including_stripped_whitespace; + if info.range.end() == range_end { + return + } + info.range.extend_to(range_end); + info.content_size.inline = info.run.metrics_for_range(&info.range).advance_width; + self.border_box.size.inline = info.content_size.inline + + self.border_padding.inline_start_end(); } } - /// Assigns replaced inline-size, padding, and margins for this fragment only if it is replaced /// content per CSS 2.1 ยง 10.3.2. pub fn assign_replaced_inline_size_if_necessary(&mut self, container_inline_size: Au) { diff --git a/components/layout/inline.rs b/components/layout/inline.rs index aa04771093a2..6db570eb6eb5 100644 --- a/components/layout/inline.rs +++ b/components/layout/inline.rs @@ -961,7 +961,7 @@ impl InlineFlow { } // First, calculate the number of expansion opportunities (spaces, normally). - let mut expansion_opportunities = 0i32; + let mut expansion_opportunities = 0; for fragment_index in line.range.each_index() { let fragment = fragments.get(fragment_index.to_usize()); let scanned_text_fragment_info = match fragment.specific { @@ -971,13 +971,16 @@ impl InlineFlow { let fragment_range = scanned_text_fragment_info.range; for slice in scanned_text_fragment_info.run.character_slices_in_range(&fragment_range) { - expansion_opportunities += slice.glyphs.space_count_in_range(&slice.range) as i32 + expansion_opportunities += slice.glyphs.space_count_in_range(&slice.range) } } + if expansion_opportunities == 0 { + return + } + // Then distribute all the space across the expansion opportunities. - let space_per_expansion_opportunity = slack_inline_size.to_f64_px() / - (expansion_opportunities as f64); + let space_per_expansion_opportunity = slack_inline_size / expansion_opportunities as i32; for fragment_index in line.range.each_index() { let fragment = fragments.get_mut(fragment_index.to_usize()); let mut scanned_text_fragment_info = match fragment.specific { @@ -985,25 +988,8 @@ impl InlineFlow { _ => continue }; let fragment_range = scanned_text_fragment_info.range; - - // FIXME(pcwalton): This is an awful lot of uniqueness making. I don't see any easy way - // to get rid of it without regressing the performance of the non-justified case, - // though. let run = Arc::make_mut(&mut scanned_text_fragment_info.run); - { - let glyph_runs = Arc::make_mut(&mut run.glyphs); - for mut glyph_run in &mut *glyph_runs { - let mut range = glyph_run.range.intersect(&fragment_range); - if range.is_empty() { - continue - } - range.shift_by(-glyph_run.range.begin()); - - let glyph_store = Arc::make_mut(&mut glyph_run.glyph_store); - glyph_store.distribute_extra_space_in_range(&range, - space_per_expansion_opportunity); - } - } + run.extra_word_spacing = space_per_expansion_opportunity; // Recompute the fragment's border box size. let new_inline_size = run.advance_for_range(&fragment_range); diff --git a/components/layout/webrender_helpers.rs b/components/layout/webrender_helpers.rs index a06fc0455001..06c94414fd39 100644 --- a/components/layout/webrender_helpers.rs +++ b/components/layout/webrender_helpers.rs @@ -393,7 +393,11 @@ impl WebRenderDisplayItemConverter for DisplayItem { for slice in item.text_run.natural_word_slices_in_visual_order(&item.range) { for glyph in slice.glyphs.iter_glyphs_for_byte_range(&slice.range) { - let glyph_advance = glyph.advance(); + let glyph_advance = if glyph.char_is_space() { + glyph.advance() + item.text_run.extra_word_spacing + } else { + glyph.advance() + }; if !slice.glyphs.is_whitespace() { let glyph_offset = glyph.offset().unwrap_or(Point2D::zero()); let glyph = webrender_traits::GlyphInstance { diff --git a/tests/wpt/metadata-css/css-text-3_dev/html/text-align-justify-001.htm.ini b/tests/wpt/metadata-css/css-text-3_dev/html/text-align-justify-001.htm.ini deleted file mode 100644 index 93112461ad9d..000000000000 --- a/tests/wpt/metadata-css/css-text-3_dev/html/text-align-justify-001.htm.ini +++ /dev/null @@ -1,3 +0,0 @@ -[text-align-justify-001.htm] - type: reftest - expected: FAIL