layout: Resolve word_spacing ahead of time.

It's not possible anymore, in the presence of min() / max(), to split a <length-percentage> value into a <length> and a <percentage> component. Tweak word_spacing to do what Gecko does (resolving it in advance).
servo · Feb 12, 2020 · f03026b · f03026b
1 parent e227715
commit f03026b
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 24 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/components/gfx/Cargo.toml b/components/gfx/Cargo.toml
@@ -27,7 +27,6 @@ libc = "0.2"
 log = "0.4"
 malloc_size_of = { path = "../malloc_size_of" }
 net_traits = {path = "../net_traits"}
-ordered-float = "1.0"
 range = {path = "../range"}
 serde = "1.0"
 servo_arc = {path = "../servo_arc"}

diff --git a/components/gfx/font.rs b/components/gfx/font.rs
@@ -13,7 +13,6 @@ use crate::text::shaping::ShaperMethods;
 use crate::text::Shaper;
 use app_units::Au;
 use euclid::default::{Point2D, Rect, Size2D};
-use ordered_float::NotNan;
 use servo_atoms::Atom;
 use smallvec::SmallVec;
 use std::borrow::ToOwned;
@@ -38,6 +37,7 @@ macro_rules! ot_tag {
 pub const GPOS: u32 = ot_tag!('G', 'P', 'O', 'S');
 pub const GSUB: u32 = ot_tag!('G', 'S', 'U', 'B');
 pub const KERN: u32 = ot_tag!('k', 'e', 'r', 'n');
+pub const LAST_RESORT_GLYPH_ADVANCE: FractionalPixel = 10.0;
 
 static TEXT_SHAPING_PERFORMANCE_COUNTER: AtomicUsize = AtomicUsize::new(0);
 
@@ -197,7 +197,7 @@ pub struct ShapingOptions {
     /// NB: You will probably want to set the `IGNORE_LIGATURES_SHAPING_FLAG` if this is non-null.
     pub letter_spacing: Option<Au>,
     /// Spacing to add between each word. Corresponds to the CSS 2.1 `word-spacing` property.
-    pub word_spacing: (Au, NotNan<f32>),
+    pub word_spacing: Au,
     /// The Unicode script property of the characters in this run.
     pub script: Script,
     /// Various flags.
@@ -278,8 +278,7 @@ impl Font {
             let mut advance = Au::from_f64_px(self.glyph_h_advance(glyph_id));
             if character == ' ' {
                 // https://drafts.csswg.org/css-text-3/#word-spacing-property
-                let (length, percent) = options.word_spacing;
-                advance = (advance + length) + Au((advance.0 as f32 * percent.into_inner()) as i32);
+                advance += options.word_spacing;
             }
             if let Some(letter_spacing) = options.letter_spacing {
                 advance += letter_spacing;
@@ -343,7 +342,7 @@ impl Font {
             .or_insert_with(|| {
                 match self.handle.glyph_h_advance(glyph) {
                     Some(adv) => adv,
-                    None => 10f64 as FractionalPixel, // FIXME: Need fallback strategy
+                    None => LAST_RESORT_GLYPH_ADVANCE as FractionalPixel, // FIXME: Need fallback strategy
                 }
             })
     }
@@ -516,8 +515,8 @@ impl RunMetrics {
         RunMetrics {
             advance_width: advance,
             bounding_box: bounds,
-            ascent: ascent,
-            descent: descent,
+            ascent,
+            descent,
         }
     }
 }

diff --git a/components/gfx/text/shaping/harfbuzz.rs b/components/gfx/text/shaping/harfbuzz.rs
@@ -434,9 +434,7 @@ impl Shaper {
         // We elect to only space the two required code points.
         if character == ' ' || character == '\u{a0}' {
             // https://drafts.csswg.org/css-text-3/#word-spacing-property
-            let (length, percent) = options.word_spacing;
-            advance =
-                (advance + length) + Au::new((advance.0 as f32 * percent.into_inner()) as i32);
+            advance += options.word_spacing;
         }
 
         advance

diff --git a/components/layout/text.rs b/components/layout/text.rs
@@ -10,7 +10,7 @@ use crate::fragment::{ScannedTextFragmentInfo, SpecificFragmentInfo, UnscannedTe
 use crate::inline::{InlineFragmentNodeFlags, InlineFragments};
 use crate::linked_list::split_off_head;
 use app_units::Au;
-use gfx::font::{FontMetrics, FontRef, RunMetrics, ShapingFlags, ShapingOptions};
+use gfx::font::{self, FontMetrics, FontRef, RunMetrics, ShapingFlags, ShapingOptions};
 use gfx::text::glyph::ByteIndex;
 use gfx::text::text_run::TextRun;
 use gfx::text::util::{self, CompressionMode};
@@ -195,7 +195,24 @@ impl TextRunScanner {
                 };
                 text_transform = inherited_text_style.text_transform;
                 letter_spacing = inherited_text_style.letter_spacing;
-                word_spacing = inherited_text_style.word_spacing.to_hash_key();
+                word_spacing = inherited_text_style
+                    .word_spacing
+                    .to_length()
+                    .map(|l| l.into())
+                    .unwrap_or_else(|| {
+                        let space_width = font_group
+                            .borrow_mut()
+                            .find_by_codepoint(&mut font_context, ' ')
+                            .and_then(|font| {
+                                let font = font.borrow();
+                                font.glyph_index(' ')
+                                    .map(|glyph_id| font.glyph_h_advance(glyph_id))
+                            })
+                            .unwrap_or(font::LAST_RESORT_GLYPH_ADVANCE);
+                        inherited_text_style
+                            .word_spacing
+                            .to_used_value(Au::from_f64_px(space_width))
+                    });
                 text_rendering = inherited_text_style.text_rendering;
                 word_break = inherited_text_style.word_break;
             }

diff --git a/components/layout_2020/flow/inline.rs b/components/layout_2020/flow/inline.rs
@@ -601,13 +601,6 @@ impl TextRun {
             flags.insert(ShapingFlags::KEEP_ALL_FLAG);
         }
 
-        let shaping_options = gfx::font::ShapingOptions {
-            letter_spacing,
-            word_spacing: inherited_text_style.word_spacing.to_hash_key(),
-            script: unicode_script::Script::Common,
-            flags,
-        };
-
         crate::context::with_thread_local_font_context(layout_context, |font_context| {
             let font_group = font_context.font_group(font_style);
             let font = font_group
@@ -616,6 +609,25 @@ impl TextRun {
                 .expect("could not find font");
             let mut font = font.borrow_mut();
 
+            let word_spacing = &inherited_text_style.word_spacing;
+            let word_spacing = word_spacing
+                .to_length()
+                .map(|l| l.into())
+                .unwrap_or_else(|| {
+                    let space_width = font
+                        .glyph_index(' ')
+                        .map(|glyph_id| font.glyph_h_advance(glyph_id))
+                        .unwrap_or(gfx::font::LAST_RESORT_GLYPH_ADVANCE);
+                    word_spacing.to_used_value(Au::from_f64_px(space_width))
+                });
+
+            let shaping_options = gfx::font::ShapingOptions {
+                letter_spacing,
+                word_spacing,
+                script: unicode_script::Script::Common,
+                flags,
+            };
+
             let (runs, break_at_start) = gfx::text::text_run::TextRun::break_and_shape(
                 &mut font,
                 &self.text,

diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml
@@ -55,7 +55,6 @@ num_cpus = {version = "1.1.0"}
 num-integer = "0.1"
 num-traits = "0.2"
 num-derive = "0.2"
-ordered-float = "1.0"
 owning_ref = "0.4"
 parking_lot = "0.9"
 precomputed-hash = "0.1.1"

diff --git a/components/style/lib.rs b/components/style/lib.rs
@@ -72,7 +72,6 @@ extern crate num_cpus;
 extern crate num_derive;
 extern crate num_integer;
 extern crate num_traits;
-extern crate ordered_float;
 extern crate owning_ref;
 extern crate parking_lot;
 extern crate precomputed_hash;