Bug 1607308 - Move the from-font value from text-underline-offset to text-underline-position, as per recent spec changes, and fix interaction between position and offset. r=emilio

Differential Revision: https://phabricator.services.mozilla.com/D59778

--HG--
extra : moz-landing-system : lando

devtools/shared/css/generated/properties-db.js

@@ -10157,7 +10157,6 @@ exports.CSS_PROPERTIES = {
     "supports": [],
     "values": [
       "auto",
-      "from-font",
       "inherit",
       "initial",
       "revert",
@@ -10172,6 +10171,7 @@ exports.CSS_PROPERTIES = {
     "supports": [],
     "values": [
       "auto",
+      "from-font",
       "inherit",
       "initial",
       "left",
@@ -10804,22 +10804,22 @@ exports.PREFERENCES = [
     "scrollbar-color",
     "layout.css.scrollbar-color.enabled"
   ],
-  [
-    "translate",
-    "layout.css.individual-transform.enabled"
-  ],
   [
     "text-decoration-thickness",
     "layout.css.text-decoration-thickness.enabled"
   ],
   [
-    "text-underline-offset",
+    "translate",
-    "layout.css.text-underline-offset.enabled"
+    "layout.css.individual-transform.enabled"
   ],
   [
     "offset-distance",
     "layout.css.motion-path.enabled"
   ],
+  [
+    "text-underline-offset",
+    "layout.css.text-underline-offset.enabled"
+  ],
   [
     "overflow-clip-box",
     "layout.css.overflow-clip-box.enabled"

layout/generic/nsTextFrame.cpp

@@ -4953,10 +4953,10 @@ static nscoord LazyGetLineBaselineOffset(nsIFrame* aChildFrame,
   }
 }
 
-static bool IsUnderlineRight(nsIFrame* aFrame) {
+static bool IsUnderlineRight(const ComputedStyle& aStyle) {
   // Check for 'left' or 'right' explicitly specified in the property;
   // if neither is there, we use auto positioning based on lang.
-  const auto position = aFrame->StyleText()->mTextUnderlinePosition;
+  const auto position = aStyle.StyleText()->mTextUnderlinePosition;
   if (position.IsLeft()) {
     return false;
   }
@@ -4964,7 +4964,7 @@ static bool IsUnderlineRight(nsIFrame* aFrame) {
     return true;
   }
   // If neither 'left' nor 'right' was specified, check the language.
-  nsAtom* langAtom = aFrame->StyleFont()->mLanguage;
+  nsAtom* langAtom = aStyle.StyleFont()->mLanguage;
   if (!langAtom) {
     return false;
   }
@@ -5082,7 +5082,7 @@ void nsTextFrame::GetTextDecorations(
       }
 
       bool swapUnderlineAndOverline =
-          vertical && !wm.IsSideways() && IsUnderlineRight(f);
+          vertical && !wm.IsSideways() && IsUnderlineRight(*context);
       const auto kUnderline = swapUnderlineAndOverline
                                   ? StyleTextDecorationLine::OVERLINE
                                   : StyleTextDecorationLine::UNDERLINE;
@@ -5304,25 +5304,23 @@ static void SetWidthIfLength(
 // this function
 // On entry, aParams.offset is already initialized with the underlineOffset
 // from the font; this function may adjust it as appropriate.
-static void SetOffsetIfLength(const StyleTextDecorationLength& aOffset,
+static void SetOffsetIfLength(const StyleTextUnderlinePosition& aPosition,
+                              const LengthPercentageOrAuto& aOffset,
                               nsCSSRendering::DecorationRectParams& aParams,
                               const gfxFont::Metrics& aFontMetrics,
                               const gfxFloat aAppUnitsPerDevPixel,
                               bool aIsSideways, bool aRightUnderline) {
-  // `auto` is treated like `from-font`, except that (for horizontal/sideways
+  // If offset is `auto`, we clamp the offset (in horizontal typographic mode)
-  // text) we clamp the offset to a minimum of 1/16 em (equivalent to 1px at
+  // to a minimum of 1/16 em (equivalent to 1px at font-size 16px) to mitigate
-  // font-size 16px) to mitigate skip-ink issues with fonts that leave the
+  // skip-ink issues with fonts that leave the underlineOffset field as zero.
-  // underlineOffset field as zero.
   if (aOffset.IsAuto()) {
     if (!aParams.vertical || aIsSideways) {
       aParams.offset =
           std::min(aParams.offset, gfx::Float(-aFontMetrics.emHeight / 16.0));
+      if (aPosition.IsUnder()) {
+        aParams.offset -= aFontMetrics.maxDescent;
       }
-    return;
     }
-
-  // If the value is `from-font`, just leave the font's value untouched.
-  if (aOffset.IsFromFont()) {
     return;
   }
 
@@ -5345,8 +5343,21 @@ static void SetOffsetIfLength(const StyleTextDecorationLength& aOffset,
     aParams.offset +=
         aOffset.AsLengthPercentage().Resolve(em) / aAppUnitsPerDevPixel;
   } else {
-    aParams.offset =
+    const gfxFloat resolvedOffset =
-        -aOffset.AsLengthPercentage().Resolve(em) / aAppUnitsPerDevPixel;
+        aOffset.AsLengthPercentage().Resolve(em) / aAppUnitsPerDevPixel;
+    if (aPosition.IsAuto()) {
+      // 'zero position' for text-underline-offset:auto is the alphabetic
+      // baseline, so we forget the input offset value (from the font).
+      aParams.offset = -resolvedOffset;
+    } else if (aPosition.IsUnder()) {
+      // 'zero position' for text-underline-offset is the font's descender
+      // metric.
+      aParams.offset = -aFontMetrics.maxDescent - resolvedOffset;
+    } else {
+      // 'zero position' is the from-font value, so we apply offset to that.
+      MOZ_ASSERT(aPosition.IsFromFont());
+      aParams.offset -= resolvedOffset;
+    }
   }
 }
 
@@ -5382,10 +5393,10 @@ void nsTextFrame::UnionAdditionalOverflow(nsPresContext* aPresContext,
     // underline position from the font.
     const auto* styleText = aBlock->StyleText();
     if (styleText->mTextUnderlinePosition.IsUnder()) {
-      underlineOffset = -fontMetrics->EmDescent();
+      underlineOffset -= fontMetrics->MaxDescent();
     }
 
-    const StyleTextDecorationLength& textUnderlineOffset =
+    const LengthPercentageOrAuto& textUnderlineOffset =
         styleText->mTextUnderlineOffset;
 
     const StyleTextDecorationLength& textDecorationThickness =
@@ -5397,6 +5408,7 @@ void nsTextFrame::UnionAdditionalOverflow(nsPresContext* aPresContext,
         underlineOffset += underlineSize;
         underlineOffset -= textUnderlineOffset.AsLengthPercentage().Resolve(em);
       } else {
+        // FIXME
         underlineOffset = -textUnderlineOffset.AsLengthPercentage().Resolve(em);
       }
     }
@@ -5496,14 +5508,12 @@ void nsTextFrame::UnionAdditionalOverflow(nsPresContext* aPresContext,
             params.defaultLineThickness = params.lineSize.height;
 
             bool swapUnderline =
-                verticalDec && !wm.IsSideways() && IsUnderlineRight(this);
+                verticalDec && !wm.IsSideways() && IsUnderlineRight(*Style());
             if (swapUnderline
                     ? lineType == StyleTextDecorationLine::OVERLINE
                     : lineType == StyleTextDecorationLine::UNDERLINE) {
-              if (dec.mTextUnderlinePosition.IsUnder()) {
+              SetOffsetIfLength(dec.mTextUnderlinePosition,
-                params.offset = -metrics.emDescent;
+                                dec.mTextUnderlineOffset, params, metrics,
-              }
-              SetOffsetIfLength(dec.mTextUnderlineOffset, params, metrics,
                                 appUnitsPerDevUnit, parentWM.IsSideways(),
                                 swapUnderline);
             }
@@ -5719,16 +5729,13 @@ void nsTextFrame::DrawSelectionDecorations(
           aTextPaintStyle.PresContext(), aFontMetrics, aSelectionType);
 
       bool swapUnderline =
-          aVertical && !wm.IsSideways() && IsUnderlineRight(this);
+          aVertical && !wm.IsSideways() && IsUnderlineRight(*Style());
       if (swapUnderline ? aDecoration == StyleTextDecorationLine::OVERLINE
                         : aDecoration == StyleTextDecorationLine::UNDERLINE) {
         const auto* styleText = StyleText();
-        if (styleText->mTextUnderlinePosition.IsUnder()) {
-          params.offset = -aFontMetrics.emDescent;
-        } else {
         params.offset = aFontMetrics.underlineOffset;
-        }
+        SetOffsetIfLength(styleText->mTextUnderlinePosition,
-        SetOffsetIfLength(styleText->mTextUnderlineOffset, params, aFontMetrics,
+                          styleText->mTextUnderlineOffset, params, aFontMetrics,
                           appUnitsPerDevPixel, wm.IsSideways(), swapUnderline);
       } else {
         params.offset = aFontMetrics.maxAscent;
@@ -6330,7 +6337,7 @@ void nsTextFrame::PaintTextSelectionDecorations(
   gfxFont* firstFont = aParams.provider->GetFontGroup()->GetFirstValidFont();
   bool verticalRun = mTextRun->IsVertical();
   bool useVerticalMetrics = verticalRun && mTextRun->UseCenterBaseline();
-  bool rightUnderline = useVerticalMetrics && IsUnderlineRight(this);
+  bool rightUnderline = useVerticalMetrics && IsUnderlineRight(*Style());
   const auto kDecoration = rightUnderline ? StyleTextDecorationLine::OVERLINE
                                           : StyleTextDecorationLine::UNDERLINE;
   gfxFont::Metrics decorationMetrics(
@@ -7011,20 +7018,15 @@ void nsTextFrame::DrawTextRunAndDecorations(
     params.defaultLineThickness = params.lineSize.height;
     params.baselineOffset = dec.mBaselineOffset / app;
 
-    if (lineType == StyleTextDecorationLine::UNDERLINE &&
-        dec.mTextUnderlinePosition.IsUnder()) {
-      params.offset = -metrics.emDescent;
-    } else {
     params.offset = metrics.*lineOffset;
-    }
 
     bool swapUnderline =
-        verticalDec && !wm.IsSideways() && IsUnderlineRight(this);
+        verticalDec && !wm.IsSideways() && IsUnderlineRight(*Style());
     if (swapUnderline ? lineType == StyleTextDecorationLine::OVERLINE
                       : lineType == StyleTextDecorationLine::UNDERLINE) {
-      SetOffsetIfLength(dec.mTextUnderlineOffset, params, metrics,
+      SetOffsetIfLength(dec.mTextUnderlinePosition, dec.mTextUnderlineOffset,
-                        PresContext()->AppUnitsPerDevPixel(), wm.IsSideways(),
+                        params, metrics, PresContext()->AppUnitsPerDevPixel(),
-                        swapUnderline);
+                        wm.IsSideways(), swapUnderline);
     }
     SetWidthIfLength(dec.mTextDecorationThickness, &params.lineSize.height,
                      metrics.emHeight, PresContext()->AppUnitsPerDevPixel());
@@ -7317,11 +7319,7 @@ bool nsTextFrame::CombineSelectionUnderlineRect(nsPresContext* aPresContext,
   nsCSSRendering::DecorationRectParams params;
   params.ascent = aPresContext->AppUnitsToGfxUnits(mAscent);
 
-  if (StyleText()->mTextUnderlinePosition.IsUnder()) {
-    params.offset = -metrics.emDescent;
-  } else {
   params.offset = fontGroup->GetUnderlineOffset();
-  }
 
   TextDecorations textDecs;
   GetTextDecorations(aPresContext, eResolvedColors, textDecs);
@@ -7379,9 +7377,10 @@ bool nsTextFrame::CombineSelectionUnderlineRect(nsPresContext* aPresContext,
                      aPresContext->AppUnitsPerDevPixel());
 
     bool swapUnderline =
-        verticalRun && !wm.IsSideways() && IsUnderlineRight(this);
+        verticalRun && !wm.IsSideways() && IsUnderlineRight(*Style());
     if (swapUnderline ? textDecs.HasOverline() : textDecs.HasUnderline()) {
-      SetOffsetIfLength(StyleText()->mTextUnderlineOffset, params, metrics,
+      SetOffsetIfLength(StyleText()->mTextUnderlinePosition,
+                        StyleText()->mTextUnderlineOffset, params, metrics,
                         aPresContext->AppUnitsPerDevPixel(), wm.IsSideways(),
                         swapUnderline);
     }

layout/generic/nsTextFrame.h

@@ -862,7 +862,7 @@ class nsTextFrame : public nsFrame {
     nscoord mBaselineOffset;
 
     // This represents the offset from the initial position of the underline
-    const mozilla::StyleTextDecorationLength mTextUnderlineOffset;
+    const mozilla::LengthPercentageOrAuto mTextUnderlineOffset;
 
     // for CSS property text-decoration-thickness, the width refers to the
     // thickness of the decoration line
@@ -876,7 +876,7 @@ class nsTextFrame : public nsFrame {
 
     LineDecoration(nsIFrame* const aFrame, const nscoord aOff,
                    mozilla::StyleTextUnderlinePosition aUnderlinePosition,
-                   const mozilla::StyleTextDecorationLength& aUnderlineOffset,
+                   const mozilla::LengthPercentageOrAuto& aUnderlineOffset,
                    const mozilla::StyleTextDecorationLength& aDecThickness,
                    const nscolor aColor, const uint8_t aStyle)
         : mFrame(aFrame),

layout/style/nsStyleStruct.cpp

@@ -3318,7 +3318,7 @@ nsStyleText::nsStyleText(const Document& aDocument)
       mLetterSpacing({0.}),
       mLineHeight(StyleLineHeight::Normal()),
       mTextIndent(LengthPercentage::Zero()),
-      mTextUnderlineOffset(StyleTextDecorationLength::Auto()),
+      mTextUnderlineOffset(LengthPercentageOrAuto::Auto()),
       mTextDecorationSkipInk(StyleTextDecorationSkipInk::Auto),
       mTextUnderlinePosition(StyleTextUnderlinePosition::AUTO),
       mWebkitTextStrokeWidth(0),

layout/style/nsStyleStruct.h

@@ -1149,7 +1149,7 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleText {
   mozilla::StyleLineHeight mLineHeight;
   mozilla::LengthPercentage mTextIndent;
 
-  mozilla::StyleTextDecorationLength mTextUnderlineOffset;
+  mozilla::LengthPercentageOrAuto mTextUnderlineOffset;
   mozilla::StyleTextDecorationSkipInk mTextDecorationSkipInk;
   mozilla::StyleTextUnderlinePosition mTextUnderlinePosition;
 
@@ -1277,7 +1277,14 @@ inline StyleTextTransform StyleTextTransform::None() {
 
 inline bool StyleTextTransform::IsNone() const { return *this == None(); }
 
-inline bool StyleTextUnderlinePosition::IsAuto() const { return *this == AUTO; }
+// Note that IsAuto() does not exclude the possibility that `left` or `right`
+// is set; it refers only to behavior in horizontal typographic mode.
+inline bool StyleTextUnderlinePosition::IsAuto() const {
+  return !(*this & (StyleTextUnderlinePosition::FROM_FONT | StyleTextUnderlinePosition::UNDER));
+}
+inline bool StyleTextUnderlinePosition::IsFromFont() const {
+  return bool(*this & StyleTextUnderlinePosition::FROM_FONT);
+}
 inline bool StyleTextUnderlinePosition::IsUnder() const {
   return bool(*this & StyleTextUnderlinePosition::UNDER);
 }

layout/style/test/property_database.js

@@ -7688,17 +7688,15 @@ var gCSSProperties = {
     applies_to_first_line: true,
     applies_to_placeholder: true,
     initial_values: ["auto"],
-    other_values: [
+    other_values: ["0", "-14px", "25px", "100em", "-45em", "43%", "-10%"],
+    invalid_values: [
+      "13",
+      "-25",
+      "rubbish",
+      ",./!@#$",
       "from-font",
-      "0",
+      "from font",
-      "-14px",
-      "25px",
-      "100em",
-      "-45em",
-      "43%",
-      "-10%",
     ],
-    invalid_values: ["13", "-25", "rubbish", ",./!@#$", "from font"],
   },
   "text-underline-position": {
     domProp: "textUnderlinePosition",
@@ -7716,16 +7714,23 @@ var gCSSProperties = {
       "under left",
       "right under",
       "under right",
+      "from-font",
+      "from-font left",
+      "from-font right",
+      "left from-font",
+      "right from-font",
     ],
     invalid_values: [
       "none",
+      "auto from-font",
       "auto under",
+      "under from-font",
       "left right",
       "right auto",
       "0",
       "1px",
       "10%",
-      "from-font",
+      "from font",
     ],
   },
   "text-emphasis": {

servo/components/style/properties/longhands/inherited_text.mako.rs

@@ -382,8 +382,8 @@ ${helpers.single_keyword(
 // text underline offset
 ${helpers.predefined_type(
     "text-underline-offset",
-    "TextDecorationLength",
+    "LengthPercentageOrAuto",
-    "generics::text::GenericTextDecorationLength::Auto",
+    "computed::LengthPercentageOrAuto::auto()",
     engines="gecko",
     animation_value_type="ComputedValue",
     gecko_pref="layout.css.text-underline-offset.enabled",

servo/components/style/values/computed/text.rs

@@ -27,7 +27,7 @@ pub use crate::values::specified::{TextDecorationSkipInk, TextTransform};
 /// A computed value for the `initial-letter` property.
 pub type InitialLetter = GenericInitialLetter<CSSFloat, CSSInteger>;
 
-/// Implements type for `text-underline-offset` and `text-decoration-thickness` properties
+/// Implements type for `text-decoration-thickness` property.
 pub type TextDecorationLength = GenericTextDecorationLength<LengthPercentage>;
 
 /// A computed value for the `letter-spacing` property.

servo/components/style/values/generics/text.rs

@@ -123,8 +123,8 @@ impl<N, L> LineHeight<N, L> {
     }
 }
 
-/// Implements type for text-underline-offset and text-decoration-thickness
+/// Implements type for text-decoration-thickness
-/// which take the grammar of auto | from-font | <length> | <percentage>
+/// which takes the grammar of auto | from-font | <length> | <percentage>
 ///
 /// https://drafts.csswg.org/css-text-decor-4/
 #[repr(C, u8)]

servo/components/style/values/specified/text.rs

@@ -1029,7 +1029,7 @@ pub enum TextDecorationSkipInk {
     None,
 }
 
-/// Implements type for `text-underline-offset` and `text-decoration-thickness` properties
+/// Implements type for `text-decoration-thickness` property
 pub type TextDecorationLength = GenericTextDecorationLength<LengthPercentage>;
 
 impl TextDecorationLength {
@@ -1048,21 +1048,23 @@ impl TextDecorationLength {
 
 bitflags! {
     #[derive(MallocSizeOf, SpecifiedValueInfo, ToComputedValue, ToResolvedValue, ToShmem)]
-    #[value_info(other_values = "auto,under,left,right")]
+    #[value_info(other_values = "auto,from-font,under,left,right")]
     #[repr(C)]
     /// Specified keyword values for the text-underline-position property.
-    /// (Non-exclusive, but not all combinations are allowed: only `under` may occur
+    /// (Non-exclusive, but not all combinations are allowed: the spec grammar gives
-    /// together with either `left` or `right`.)
+    /// `auto | [ from-font | under ] || [ left | right ]`.)
-    /// https://drafts.csswg.org/css-text-decor-3/#text-underline-position-property
+    /// https://drafts.csswg.org/css-text-decor-4/#text-underline-position-property
     pub struct TextUnderlinePosition: u8 {
         /// Use automatic positioning below the alphabetic baseline.
         const AUTO = 0;
+        /// Use underline position from the first available font.
+        const FROM_FONT = 1 << 0;
         /// Below the glyph box.
-        const UNDER = 1 << 0;
+        const UNDER = 1 << 1;
         /// In vertical mode, place to the left of the text.
-        const LEFT = 1 << 1;
+        const LEFT = 1 << 2;
         /// In vertical mode, place to the right of the text.
-        const RIGHT = 1 << 2;
+        const RIGHT = 1 << 3;
     }
 }
 
@@ -1085,7 +1087,12 @@ impl Parse for TextUnderlinePosition {
                 "auto" if result.is_empty() => {
                     return Ok(result);
                 },
-                "under" if !result.intersects(TextUnderlinePosition::UNDER) => {
+                "from-font" if !result.intersects(TextUnderlinePosition::FROM_FONT |
+                                                  TextUnderlinePosition::UNDER) => {
+                    result.insert(TextUnderlinePosition::FROM_FONT);
+                },
+                "under" if !result.intersects(TextUnderlinePosition::FROM_FONT |
+                                              TextUnderlinePosition::UNDER) => {
                     result.insert(TextUnderlinePosition::UNDER);
                 },
                 "left" if !result.intersects(TextUnderlinePosition::LEFT |
@@ -1131,6 +1138,7 @@ impl ToCss for TextUnderlinePosition {
             };
         }
 
+        maybe_write!(FROM_FONT => "from-font");
         maybe_write!(UNDER => "under");
         maybe_write!(LEFT => "left");
         maybe_write!(RIGHT => "right");

servo/ports/geckolib/cbindgen.toml

@@ -398,6 +398,7 @@ renaming_overrides_prefixing = true
 
 "TextUnderlinePosition" = """
   inline bool IsAuto() const;
+  inline bool IsFromFont() const;
   inline bool IsUnder() const;
   inline bool IsLeft() const;
   inline bool IsRight() const;

testing/web-platform/tests/css/css-text-decor/text-underline-offset-computed.html

@@ -13,7 +13,6 @@
 <div id="target"></div>
 <script>
 test_computed_value("text-underline-offset", "auto");
-test_computed_value("text-underline-offset", "from-font");
 test_computed_value("text-underline-offset", "calc(10px - 8px)", "2px");
 </script>
 </body>

testing/web-platform/tests/css/css-text-decor/text-underline-offset-invalid.html

@@ -4,13 +4,14 @@
 <meta charset="utf-8">
 <title>CSS Text Decoration Test: parsing text-underline-offset with invalid values</title>
 <link rel="help" href="https://drafts.csswg.org/css-text-decor-4/#underline-offset">
-<meta name="assert" content="text-underline-offset supports the following values: auto | from-font| <length> | <percentage>">
+<meta name="assert" content="text-underline-offset supports the following values: auto | <length> | <percentage>">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="/css/support/parsing-testcommon.js"></script>
 </head>
 <body>
 <script>
+test_invalid_value("text-underline-offset", "from-font");
 test_invalid_value("text-underline-offset", "otto");
 test_invalid_value("text-underline-offset", "asdlflj");
 test_invalid_value("text-underline-offset", "-10");

testing/web-platform/tests/css/css-text-decor/text-underline-offset-valid.html

@@ -4,7 +4,7 @@
 <meta charset="utf-8">
 <title>CSS Text Decoration Test: parsing text-underline-offset with valid values</title>
 <link rel="help" href="https://drafts.csswg.org/css-text-decor-4/#underline-offset">
-<meta name="assert" content="text-underline-offset supports the following values: auto | from-font| <length> | <percentage>">
+<meta name="assert" content="text-underline-offset supports the following values: auto | <length> | <percentage>">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="/css/support/parsing-testcommon.js"></script>
@@ -12,7 +12,6 @@
 <body>
 <script>
 test_valid_value("text-underline-offset", "auto");
-test_valid_value("text-underline-offset", "from-font");
 test_valid_value("text-underline-offset", "-10px");
 test_valid_value("text-underline-offset", "2001em");
 test_valid_value("text-underline-offset", "-49em");