shaping: Don't assume there's a space glyph when rendering tabs (#32979)

Previously if a font didn't have a space advance and it was needed to
make advances for tabs, Servo would try to read the advance from the
font. If the font didn't have a space glyph, Servo would panic. This
fixes that issue by making the space advance part of the `FontMetrics`
of a font (like Gecko) and falling back properly if that glyph doesn't
exist. The rendered glyph is still the "space" glyph, but we make
sure to select a font that supports that glyph explicitly.

This prevents a crash, but tabs still aren't handled properly. In
reality, tab stops should be calculated in layout and the size of
the space character of the current font shouldn't come into play.
The addition of the space advance metric will make this easier.

Fixes #32970.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

components/fonts/font.rs

@@ -12,6 +12,7 @@ use std::{iter, str};
 use app_units::Au;
 use bitflags::bitflags;
 use euclid::default::{Point2D, Rect, Size2D};
+use euclid::num::Zero;
 use log::debug;
 use malloc_size_of_derive::MallocSizeOf;
 use parking_lot::RwLock;
@@ -137,6 +138,9 @@ pub struct FontMetrics {
     pub line_gap: Au,
     pub zero_horizontal_advance: Option<Au>,
     pub ic_horizontal_advance: Option<Au>,
+    /// The advance of the space character (' ') in this font or if there is no space,
+    /// the average char advance.
+    pub space_advance: Au,
 }
 
 impl FontMetrics {
@@ -144,20 +148,21 @@ impl FontMetrics {
     /// no font can be found.
     pub fn empty() -> Self {
         Self {
-            underline_size: Au(0),
-            underline_offset: Au(0),
-            strikeout_size: Au(0),
-            strikeout_offset: Au(0),
-            leading: Au(0),
-            x_height: Au(0),
-            em_size: Au(0),
-            ascent: Au(0),
-            descent: Au(0),
-            max_advance: Au(0),
-            average_advance: Au(0),
-            line_gap: Au(0),
+            underline_size: Au::zero(),
+            underline_offset: Au::zero(),
+            strikeout_size: Au::zero(),
+            strikeout_offset: Au::zero(),
+            leading: Au::zero(),
+            x_height: Au::zero(),
+            em_size: Au::zero(),
+            ascent: Au::zero(),
+            descent: Au::zero(),
+            max_advance: Au::zero(),
+            average_advance: Au::zero(),
+            line_gap: Au::zero(),
             zero_horizontal_advance: None,
             ic_horizontal_advance: None,
+            space_advance: Au::zero(),
         }
     }
 }
@@ -517,6 +522,14 @@ impl FontGroup {
         codepoint: char,
         next_codepoint: Option<char>,
     ) -> Option<FontRef> {
+        // Tab characters are converted into spaces when rendering.
+        // TODO: We should not render a tab character. Instead they should be converted into tab stops
+        // based upon the width of a space character in inline formatting contexts.
+        let codepoint = match codepoint {
+            '\t' => ' ',
+            _ => codepoint,
+        };
+
         let options = FallbackFontSelectionOptions::new(codepoint, next_codepoint);
 
         let should_look_for_small_caps = self.descriptor.variant == font_variant_caps::T::SmallCaps &&

components/fonts/platform/freetype/font.rs

@@ -351,6 +351,10 @@ impl PlatformFontMethods for PlatformFont {
             .glyph_index('\u{6C34}')
             .and_then(|idx| self.glyph_h_advance(idx))
             .map(Au::from_f64_px);
+        let space_advance = self
+            .glyph_index(' ')
+            .and_then(|idx| self.glyph_h_advance(idx))
+            .unwrap_or(average_advance);
 
         FontMetrics {
             underline_size: Au::from_f64_px(underline_size),
@@ -367,6 +371,7 @@ impl PlatformFontMethods for PlatformFont {
             line_gap: Au::from_f64_px(line_height),
             zero_horizontal_advance,
             ic_horizontal_advance,
+            space_advance: Au::from_f64_px(space_advance),
         }
     }
 

components/fonts/platform/macos/font.rs

@@ -272,11 +272,17 @@ impl PlatformFontMethods for PlatformFont {
             .glyph_index('0')
             .and_then(|idx| self.glyph_h_advance(idx))
             .map(Au::from_f64_px);
+        let average_advance = zero_horizontal_advance.unwrap_or(max_advance);
+
         let ic_horizontal_advance = self
             .glyph_index('\u{6C34}')
             .and_then(|idx| self.glyph_h_advance(idx))
             .map(Au::from_f64_px);
-        let average_advance = zero_horizontal_advance.unwrap_or(max_advance);
+        let space_advance = self
+            .glyph_index(' ')
+            .and_then(|index| self.glyph_h_advance(index))
+            .map(Au::from_f64_px)
+            .unwrap_or(average_advance);
 
         let metrics = FontMetrics {
             underline_size: Au::from_f64_au(underline_thickness),
@@ -301,6 +307,7 @@ impl PlatformFontMethods for PlatformFont {
             line_gap: Au::from_f64_px(line_gap),
             zero_horizontal_advance,
             ic_horizontal_advance,
+            space_advance,
         };
         debug!(
             "Font metrics (@{} pt): {:?}",

components/fonts/platform/windows/font.rs

@@ -19,6 +19,7 @@ use log::{debug, warn};
 use style::computed_values::font_stretch::T as StyleFontStretch;
 use style::computed_values::font_weight::T as StyleFontWeight;
 use style::values::computed::font::FontStyle as StyleFontStyle;
+use style::Zero;
 use truetype::tables::WindowsMetrics;
 use truetype::value::Read;
 use webrender_api::FontInstanceFlags;
@@ -242,6 +243,17 @@ impl PlatformFontMethods for PlatformFont {
             .and_then(|idx| self.glyph_h_advance(idx))
             .map(Au::from_f64_px);
 
+        // TODO: These should be retrieved from the OS/2 table if possible and then
+        // fall back to measuring 'x' if that is not available.
+        let average_advance = Au::zero();
+        let max_advance = Au::zero();
+
+        let space_advance = self
+            .glyph_index(' ')
+            .and_then(|index| self.glyph_h_advance(index))
+            .map(Au::from_f64_px)
+            .unwrap_or(average_advance);
+
         let metrics = FontMetrics {
             underline_size: au_from_du(dm.underlineThickness as i32),
             underline_offset: au_from_du_s(dm.underlinePosition as i32),
@@ -252,11 +264,12 @@ impl PlatformFontMethods for PlatformFont {
             em_size: au_from_em(self.em_size as f64),
             ascent: au_from_du_s(dm.ascent as i32),
             descent: au_from_du_s(dm.descent as i32),
-            max_advance: au_from_pt(0.0),     // FIXME
-            average_advance: au_from_pt(0.0), // FIXME
+            max_advance,
+            average_advance,
             line_gap: au_from_du_s((dm.ascent + dm.descent + dm.lineGap as u16) as i32),
             zero_horizontal_advance,
             ic_horizontal_advance,
+            space_advance,
         };
         debug!("Font metrics (@{} pt): {:?}", self.em_size * 12., metrics);
         metrics

components/fonts/shaper.rs

@@ -545,20 +545,27 @@ impl Shaper {
                 let character = text[byte_range.clone()].chars().next().unwrap();
                 if is_bidi_control(character) {
                     // Don't add any glyphs for bidi control chars
-                } else if character == '\t' {
-                    // Treat tabs in pre-formatted text as a fixed number of spaces.
-                    //
-                    // TODO: Proper tab stops.
-                    const TAB_COLS: i32 = 8;
-                    let (space_glyph_id, space_advance) = glyph_space_advance(self.font);
-                    let advance = Au::from_f64_px(space_advance) * TAB_COLS;
-                    let data =
-                        GlyphData::new(space_glyph_id, advance, Default::default(), true, true);
-                    glyphs.add_glyph_for_byte_index(byte_idx, character, &data);
                 } else {
-                    let shape = glyph_data.entry_for_glyph(glyph_span.start, &mut y_pos);
-                    let advance = self.advance_for_shaped_glyph(shape.advance, character, options);
-                    let data = GlyphData::new(shape.codepoint, advance, shape.offset, true, true);
+                    let (glyph_id, advance, offset) = if character == '\t' {
+                        // Treat tabs in pre-formatted text as a fixed number of spaces. The glyph id does
+                        // not matter here as Servo doesn't render any glyphs for whitespace.
+                        //
+                        // TODO: Proper tab stops. This should happen in layout and be based on the
+                        // size of the space character of the inline formatting context.
+                        let font = unsafe { &(*self.font) };
+                        (
+                            font.glyph_index(' ').unwrap_or(0) as hb_codepoint_t,
+                            font.metrics.space_advance * 8,
+                            Default::default(),
+                        )
+                    } else {
+                        let shape = glyph_data.entry_for_glyph(glyph_span.start, &mut y_pos);
+                        let advance =
+                            self.advance_for_shaped_glyph(shape.advance, character, options);
+                        (shape.codepoint, advance, shape.offset)
+                    };
+
+                    let data = GlyphData::new(glyph_id, advance, offset, true, true);
                     glyphs.add_glyph_for_byte_index(byte_idx, character, &data);
                 }
             } else {
@@ -708,16 +715,6 @@ extern "C" fn glyph_h_advance_func(
     }
 }
 
-fn glyph_space_advance(font: *const Font) -> (hb_codepoint_t, f64) {
-    let space_unicode = ' ';
-    let space_glyph: hb_codepoint_t = match unsafe { (*font).glyph_index(space_unicode) } {
-        Some(g) => g as hb_codepoint_t,
-        None => panic!("No space info"),
-    };
-    let space_advance = unsafe { (*font).glyph_h_advance(space_glyph as GlyphId) };
-    (space_glyph, space_advance)
-}
-
 // Callback to get a font table out of a font.
 extern "C" fn font_table_func(
     _: *mut hb_face_t,

tests/wpt/meta-legacy-layout/css/css-text/tab-size/tab-size-integer-005.html.ini

@@ -1,2 +1,2 @@
 [tab-size-integer-005.html]
-  expected: CRASH
+  expected: FAIL

tests/wpt/meta/css/css-text/tab-size/tab-size-integer-005.html.ini

@@ -1,2 +1,2 @@
 [tab-size-integer-005.html]
-  expected: CRASH
+  expected: FAIL