Bug 1582016 - When text-underline-offset is `auto`, clamp the minimum used underline-offset to 1/16em. r=dholbert a=RyanVM
authorJonathan Kew <jkew@mozilla.com>
Wed, 18 Sep 2019 17:31:14 +0000
changeset 555183 64c1b75ba5a7541e5f1aa8b3843fa4c4206b105f
parent 555182 8cd0caa2a4df8651ee709644be3ddd88f664595c
child 555184 3e843672eec79d1b4c239429556a99b26ba000f2
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert, RyanVM
bugs1582016
milestone70.0
Bug 1582016 - When text-underline-offset is `auto`, clamp the minimum used underline-offset to 1/16em. r=dholbert a=RyanVM This is to avoid skip-ink problems with fonts that leave the OpenType field at zero, which is unlikely to ever be what is really wanted. Authors can still avoid the clamp by explicitly using text-underline-offset:from-font. Differential Revision: https://phabricator.services.mozilla.com/D46268
layout/generic/nsTextFrame.cpp
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -5363,26 +5363,42 @@ static void SetWidthIfLength(
         aDecorationThickness.AsLength().ToAppUnits() / aAppUnitsPerDevPixel;
   }
 }
 
 // helper function for implementing text-underline-offset
 // https://drafts.csswg.org/css-text-decor-4/#underline-offset
 // params.defaultLineThickness should be set before calling
 // 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,
                               nsCSSRendering::DecorationRectParams& aParams,
+                              const gfxFont::Metrics& aFontMetrics,
                               const gfxFloat aAppUnitsPerDevPixel,
                               bool aIsSideways, bool aRightUnderline) {
-  // auto is from-font (the automatic offset is derived from font metrics) so we
-  // don't change the value unless it is a length
-  if (!aOffset.IsLength()) {
+  // `auto` is treated like `from-font`, except that (for horizontal/sideways
+  // text) we clamp the offset to a minimum of 1/16 em (equivalent to 1px at
+  // font-size 16px) to mitigate skip-ink issues with fonts that leave the
+  // underlineOffset field as zero.
+  if (aOffset.IsAuto()) {
+    if (!aParams.vertical || aIsSideways) {
+      aParams.offset =
+          std::min(aParams.offset, gfx::Float(-aFontMetrics.emHeight / 16.0));
+    }
     return;
   }
 
+  // If the value is `from-font`, just leave the font's value untouched.
+  if (aOffset.IsFromFont()) {
+    return;
+  }
+
+  MOZ_ASSERT(aOffset.IsLength(), "unexpected value type!");
+
   if (aParams.vertical && !aIsSideways &&
       aParams.decoration == StyleTextDecorationLine_UNDERLINE) {
     // vertical text has a baseline that goes through the center of the text
     // the user would expect that an offset of 0 would be slightly behind the
     // automatic offset, not down the middle of the text
     // use the default line thickness to set the offset behind (closer to the
     // text) than the automatic offset, then set the user's defined offset
     aParams.offset += aParams.defaultLineThickness;
@@ -5530,17 +5546,17 @@ void nsTextFrame::UnionAdditionalOverflo
             params.lineSize.height = metrics.*lineSize;
 
             params.offset = metrics.*lineOffset;
             params.defaultLineThickness = params.lineSize.height;
 
             bool swapUnderline = verticalDec && IsUnderlineRight(this);
             if (swapUnderline ? lineType == StyleTextDecorationLine_OVERLINE
                               : lineType == StyleTextDecorationLine_UNDERLINE) {
-              SetOffsetIfLength(dec.mTextUnderlineOffset, params,
+              SetOffsetIfLength(dec.mTextUnderlineOffset, params, metrics,
                                 appUnitsPerDevUnit, parentWM.IsSideways(),
                                 swapUnderline);
             }
 
             SetWidthIfLength(dec.mTextDecorationThickness,
                              &params.lineSize.height, appUnitsPerDevUnit);
 
             const nsRect decorationRect =
@@ -5749,17 +5765,18 @@ void nsTextFrame::DrawSelectionDecoratio
           aTextPaintStyle.PresContext(), aFontMetrics, aSelectionType);
 
       bool swapUnderline = aVertical && IsUnderlineRight(this);
       if (swapUnderline ? aDecoration == StyleTextDecorationLine_OVERLINE
                         : aDecoration == StyleTextDecorationLine_UNDERLINE) {
         WritingMode wm = GetWritingMode();
         params.offset = aFontMetrics.underlineOffset;
         SetOffsetIfLength(StyleText()->mTextUnderlineOffset, params,
-                          appUnitsPerDevPixel, wm.IsSideways(), swapUnderline);
+                          aFontMetrics, appUnitsPerDevPixel, wm.IsSideways(),
+                          swapUnderline);
       } else {
         params.offset = aFontMetrics.maxAscent;
       }
 
       params.lineSize.height = params.defaultLineThickness;
       SetWidthIfLength(decThickness, &params.lineSize.height,
                        appUnitsPerDevPixel);
 
@@ -7033,17 +7050,17 @@ void nsTextFrame::DrawTextRunAndDecorati
     params.color = dec.mColor;
     params.defaultLineThickness = params.lineSize.height;
     params.offset = metrics.*lineOffset;
     params.baselineOffset = dec.mBaselineOffset / app;
 
     bool swapUnderline = verticalDec && IsUnderlineRight(this);
     if (swapUnderline ? lineType == StyleTextDecorationLine_OVERLINE
                       : lineType == StyleTextDecorationLine_UNDERLINE) {
-      SetOffsetIfLength(dec.mTextUnderlineOffset, params,
+      SetOffsetIfLength(dec.mTextUnderlineOffset, params, metrics,
                         PresContext()->AppUnitsPerDevPixel(), wm.IsSideways(),
                         swapUnderline);
     }
     SetWidthIfLength(dec.mTextDecorationThickness, &params.lineSize.height,
                      PresContext()->AppUnitsPerDevPixel());
 
     params.style = dec.mStyle;
     PaintDecorationLine(params);
@@ -7386,17 +7403,17 @@ bool nsTextFrame::CombineSelectionUnderl
     params.defaultLineThickness = ComputeSelectionUnderlineHeight(
         aPresContext, metrics, sd->mSelectionType);
     params.lineSize.height = params.defaultLineThickness;
     SetWidthIfLength(decThickness, &params.lineSize.height,
                      aPresContext->AppUnitsPerDevPixel());
 
     bool swapUnderline = verticalRun && IsUnderlineRight(this);
     if (swapUnderline ? textDecs.HasOverline() : textDecs.HasUnderline()) {
-      SetOffsetIfLength(StyleText()->mTextUnderlineOffset, params,
+      SetOffsetIfLength(StyleText()->mTextUnderlineOffset, params, metrics,
                         aPresContext->AppUnitsPerDevPixel(), wm.IsSideways(),
                         swapUnderline);
     }
     relativeSize = std::max(relativeSize, 1.0f);
     params.lineSize.height *= relativeSize;
     params.defaultLineThickness *= relativeSize;
     decorationArea =
         nsCSSRendering::GetTextDecorationRect(aPresContext, params);