Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always. r=emilio draft
authorCameron McCormack <cam@mcc.id.au>
Wed, 20 Jun 2018 18:56:34 +1000
changeset 809795 65ac01ed3f628dd2d98f6ae1df0a1475b773ba74
parent 809794 39da73805b6f7821d19a07149bbecf61379a9b81
child 809796 a356b66bde06ac46ad11e7e29928fd61e3002e14
push id113820
push userbmo:cam@mcc.id.au
push dateSat, 23 Jun 2018 03:01:03 +0000
reviewersemilio
bugs282126
milestone62.0a1
Bug 282126 - Part 4: Use horizontal metrics for ch in vertical mixed/sideways writing modes and for ex always. r=emilio MozReview-Commit-ID: 5aJ4sxEC8V1
servo/components/style/font_metrics.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/values/specified/length.rs
--- a/servo/components/style/font_metrics.rs
+++ b/servo/components/style/font_metrics.rs
@@ -4,42 +4,50 @@
 
 //! Access to font metrics from the style system.
 
 #![deny(missing_docs)]
 
 use Atom;
 use app_units::Au;
 use context::SharedStyleContext;
-use logical_geometry::WritingMode;
 use media_queries::Device;
 use properties::style_structs::Font;
 
 /// Represents the font metrics that style needs from a font to compute the
 /// value of certain CSS units like `ex`.
 #[derive(Clone, Debug, Default, PartialEq)]
 pub struct FontMetrics {
     /// The x-height of the font.
     pub x_height: Option<Au>,
     /// The zero advance. This is usually writing mode dependent
     pub zero_advance_measure: Option<Au>,
 }
 
+/// Type of font metrics to retrieve.
+#[derive(Clone, Debug, PartialEq)]
+pub enum FontMetricsOrientation {
+    /// Horizontal metrics.
+    Horizontal,
+    /// Vertical metrics.
+    Vertical,
+}
+
 /// A trait used to represent something capable of providing us font metrics.
 pub trait FontMetricsProvider {
     /// Obtain the metrics for given font family.
     ///
     /// TODO: We could make this take the full list, I guess, and save a few
     /// virtual calls in the case we are repeatedly unable to find font metrics?
     /// That is not too common in practice though.
     fn query(
         &self,
         _font: &Font,
         _font_size: Au,
-        _wm: WritingMode,
+        _orientation: FontMetricsOrientation,
         _in_media_query: bool,
         _device: &Device,
     ) -> FontMetrics {
         Default::default()
     }
 
     /// Get default size of a given language and generic family
     fn get_size(&self, font_name: &Atom, font_family: u8) -> Au;
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -18,17 +18,17 @@ use CaseSensitivityExt;
 use app_units::Au;
 use applicable_declarations::ApplicableDeclarationBlock;
 use atomic_refcell::{AtomicRefCell, AtomicRefMut};
 use author_styles::AuthorStyles;
 use context::{PostAnimationTasks, QuirksMode, SharedStyleContext, UpdateAnimationsTasks};
 use data::ElementData;
 use dom::{LayoutIterator, NodeInfo, OpaqueNode, TDocument, TElement, TNode, TShadowRoot};
 use element_state::{DocumentState, ElementState};
-use font_metrics::{FontMetrics, FontMetricsProvider};
+use font_metrics::{FontMetrics, FontMetricsOrientation, FontMetricsProvider};
 use gecko::data::GeckoStyleSheet;
 use gecko::global_style_data::GLOBAL_STYLE_DATA;
 use gecko::selector_parser::{NonTSPseudoClass, PseudoElement, SelectorImpl};
 use gecko::snapshot_helpers;
 use gecko_bindings::bindings;
 use gecko_bindings::bindings::{Gecko_ElementState, Gecko_GetDocumentLWTheme};
 use gecko_bindings::bindings::{Gecko_GetLastChild, Gecko_GetNextStyleChild};
 use gecko_bindings::bindings::{Gecko_SetNodeFlags, Gecko_UnsetNodeFlags};
@@ -57,17 +57,16 @@ use gecko_bindings::structs::ELEMENT_HAS
 use gecko_bindings::structs::EffectCompositor_CascadeLevel as CascadeLevel;
 use gecko_bindings::structs::NODE_DESCENDANTS_NEED_FRAMES;
 use gecko_bindings::structs::NODE_NEEDS_FRAME;
 use gecko_bindings::structs::nsChangeHint;
 use gecko_bindings::structs::nsIDocument_DocumentTheme as DocumentTheme;
 use gecko_bindings::structs::nsRestyleHint;
 use gecko_bindings::sugar::ownership::{HasArcFFI, HasSimpleFFI};
 use hash::FnvHashMap;
-use logical_geometry::WritingMode;
 use media_queries::Device;
 use properties::{ComputedValues, LonghandId};
 use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock};
 use properties::animated_properties::{AnimationValue, AnimationValueMap};
 use properties::style_structs::Font;
 use rule_tree::CascadeLevel as ServoCascadeLevel;
 use selector_parser::{AttrValue, Direction, PseudoClassStringArg};
 use selectors::{Element, OpaqueElement};
@@ -1003,25 +1002,25 @@ impl FontMetricsProvider for GeckoFontMe
         cache.push((font_name.clone(), sizes));
         sizes.size_for_generic(font_family)
     }
 
     fn query(
         &self,
         font: &Font,
         font_size: Au,
-        wm: WritingMode,
+        orientation: FontMetricsOrientation,
         in_media_query: bool,
         device: &Device,
     ) -> FontMetrics {
         use gecko_bindings::bindings::Gecko_GetFontMetrics;
         let gecko_metrics = unsafe {
             Gecko_GetFontMetrics(
                 device.pres_context(),
-                wm.is_vertical() && !wm.is_sideways(),
+                orientation == FontMetricsOrientation::Vertical,
                 font.gecko(),
                 font_size.0,
                 // we don't use the user font set in a media query
                 !in_media_query,
             )
         };
         FontMetrics {
             x_height: Some(Au(gecko_metrics.mXSize)),
--- a/servo/components/style/values/specified/length.rs
+++ b/servo/components/style/values/specified/length.rs
@@ -4,17 +4,17 @@
 
 //! [Length values][length].
 //!
 //! [length]: https://drafts.csswg.org/css-values/#lengths
 
 use app_units::Au;
 use cssparser::{Parser, Token};
 use euclid::Size2D;
-use font_metrics::FontMetrics;
+use font_metrics::{FontMetrics, FontMetricsOrientation};
 use parser::{Parse, ParserContext};
 use std::cmp;
 use std::ops::{Add, Mul};
 use style_traits::{ParseError, SpecifiedValueInfo, StyleParseErrorKind};
 use style_traits::values::specified::AllowedNumericType;
 use super::{AllowQuirks, Number, Percentage, ToComputedValue};
 use values::{Auto, CSSFloat, Either, Normal};
 use values::computed::{self, CSSPixelLength, Context, ExtremumLength};
@@ -116,21 +116,22 @@ impl FontRelativeLength {
     fn reference_font_size_and_length(
         &self,
         context: &Context,
         base_size: FontBaseSize,
     ) -> (Au, CSSFloat) {
         fn query_font_metrics(
             context: &Context,
             reference_font_size: Au,
+            orientation: FontMetricsOrientation,
         ) -> FontMetrics {
             context.font_metrics_provider.query(
                 context.style().get_font(),
                 reference_font_size,
-                context.style().writing_mode,
+                orientation,
                 context.in_media_query,
                 context.device(),
             )
         }
 
         let reference_font_size = base_size.resolve(context);
         match *self {
             FontRelativeLength::Em(length) => {
@@ -148,45 +149,64 @@ impl FontRelativeLength {
                 } else {
                     (reference_font_size, length)
                 }
             },
             FontRelativeLength::Ex(length) => {
                 if context.for_non_inherited_property.is_some() {
                     context.rule_cache_conditions.borrow_mut().set_uncacheable();
                 }
-                let metrics = query_font_metrics(context, reference_font_size);
+                // The x-height is an intrinsically horizontal metric.
+                let metrics = query_font_metrics(
+                    context,
+                    reference_font_size,
+                    FontMetricsOrientation::Horizontal
+                );
                 let reference_size = metrics.x_height.unwrap_or_else(|| {
                     // https://drafts.csswg.org/css-values/#ex
                     //
                     //     In the cases where it is impossible or impractical to
                     //     determine the x-height, a value of 0.5em must be
                     //     assumed.
                     //
                     reference_font_size.scale_by(0.5)
                 });
                 (reference_size, length)
             },
             FontRelativeLength::Ch(length) => {
                 if context.for_non_inherited_property.is_some() {
                     context.rule_cache_conditions.borrow_mut().set_uncacheable();
                 }
-                let metrics = query_font_metrics(context, reference_font_size);
+                // https://drafts.csswg.org/css-values/#ch:
+                //
+                //     Equal to the used advance measure of the “0” (ZERO,
+                //     U+0030) glyph in the font used to render it. (The advance
+                //     measure of a glyph is its advance width or height,
+                //     whichever is in the inline axis of the element.) 
+                //
+                let wm = context.style().writing_mode;
+                let is_vertical_upright = wm.is_vertical() && wm.is_upright();
+                let orientation = if is_vertical_upright {
+                    FontMetricsOrientation::Vertical
+                } else {
+                    FontMetricsOrientation::Horizontal
+                };
+                let metrics = query_font_metrics(context, reference_font_size, orientation);
                 let reference_size = metrics.zero_advance_measure.unwrap_or_else(|| {
                     // https://drafts.csswg.org/css-values/#ch
                     //
                     //     In the cases where it is impossible or impractical to
                     //     determine the measure of the “0” glyph, it must be
                     //     assumed to be 0.5em wide by 1em tall. Thus, the ch
                     //     unit falls back to 0.5em in the general case, and to
                     //     1em when it would be typeset upright (i.e.
                     //     writing-mode is vertical-rl or vertical-lr and
                     //     text-orientation is upright).
                     //
-                    if context.style().writing_mode.is_vertical() {
+                    if is_vertical_upright {
                         reference_font_size
                     } else {
                         reference_font_size.scale_by(0.5)
                     }
                 });
                 (reference_size, length)
             },
             FontRelativeLength::Rem(length) => {