Support language-appropriate position for "text-underline-position:under"

third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp

Index: third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
diff --git a/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp b/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
index 0f183ce21a86cce5e2fc5fb95d25d107d2060891..1f219561b76310ac302d91b09ce2aba322d4a52f 100644
--- a/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
+++ b/third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
@@ -36,6 +36,41 @@ std::pair<unsigned, unsigned> GetMarkerPaintOffsets(
 }
 }
 
+enum class ResolvedUnderlinePosition { Roman, Under };
+
+static ResolvedUnderlinePosition resolveUnderlinePosition(
+    const ComputedStyle& style,
+    const InlineTextBox* inlineTextBox,
+    TextDecoration* flipDecoration) {
+  // |auto| should resolve to |under| to avoid drawing through glyphs in
+  // scripts where it would not be appropriate (e.g., ideographs.)
+  // However, this has performance implications. For now, we only work with
+  // vertical text.
+  switch (inlineTextBox->root().baselineType()) {
+    default:
+      NOTREACHED();
+    // Fall though.
+    case AlphabeticBaseline:
+      switch (style.getTextUnderlinePosition()) {
+        default:
+          NOTREACHED();
+        // Fall though.
+        case TextUnderlinePositionAuto:
+          return ResolvedUnderlinePosition::Roman;
+        case TextUnderlinePositionUnder:
+          return ResolvedUnderlinePosition::Under;
+      }
+      break;
+    case IdeographicBaseline:
+      // Compute language-appropriate default underline position.
+      // https://drafts.csswg.org/css-text-decor-3/#default-stylesheet
+      UScriptCode script = style.getFontDescription().script();
+      if (script == USCRIPT_KATAKANA_OR_HIRAGANA || script == USCRIPT_HANGUL)

[drott, 2017/02/07 16:20:52]

Are we sure this couldn't be USCRIPT_KATAKANA or USCRIPT_HIRAGANA, or does this
comparison take care of those?

[kojii, 2017/02/07 23:13:44]

The former, we use this convention in several places since Japanese is
multi-script. Conversion here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/text/...
and an exmple here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/text/...

+        *flipDecoration = TextDecorationUnderline | TextDecorationOverline;
+      return ResolvedUnderlinePosition::Under;
+  }
+}
+
 static LineLayoutItem enclosingUnderlineObject(
     const InlineTextBox* inlineTextBox) {
   bool firstLine = inlineTextBox->isFirstLineStyle();
@@ -98,19 +133,19 @@ static int computeUnderlineOffsetForRoman(const FontMetrics& fontMetrics,
   return fontMetrics.ascent() + gap;
 }
 
-static int computeUnderlineOffset(const ComputedStyle& style,
+static int computeUnderlineOffset(ResolvedUnderlinePosition underlinePosition,
+                                  const ComputedStyle& style,
                                   const FontMetrics& fontMetrics,
                                   const InlineTextBox* inlineTextBox,
                                   const float textDecorationThickness) {
-  // FIXME: We support only horizontal text for now.
-  switch (style.getTextUnderlinePosition()) {
+  switch (underlinePosition) {
     default:
       NOTREACHED();
     // Fall through.
-    case TextUnderlinePositionAuto:
+    case ResolvedUnderlinePosition::Roman:
       return computeUnderlineOffsetForRoman(fontMetrics,
                                             textDecorationThickness);
-    case TextUnderlinePositionUnder: {
+    case ResolvedUnderlinePosition::Under: {
       // Position underline at the under edge of the lowest element's
       // content box.
       LayoutUnit offset = computeUnderlineOffsetForUnder(style, inlineTextBox);
@@ -120,6 +155,14 @@ static int computeUnderlineOffset(const ComputedStyle& style,
   }
 }
 
+static int computeOverlineOffset(const ComputedStyle& style,
+                                 const InlineTextBox* inlineTextBox) {
+  // Position underline at the over edge of the highest element's
+  // content box.
+  LayoutUnit offset = computeUnderlineOffsetForUnder(style, inlineTextBox);

[drott, 2017/02/07 16:20:52]

I don't fully understand why this works and why the
computerUnderlineOffsetForUnder function can help compute the offset for the
overline. Could you explain to which base coordinate the offset is applied and
why negating it works for computing the overline position?

[kojii, 2017/02/07 23:13:44]

Maybe function name is bad...computeUnderlineOffsetForUnder() computes position
across elements, and on the next line in |computeUnderlineOffset| adds
|logicalHeight| to move it down to the bottom.

I'll rename this in the next CL.

+  return std::min(-offset, LayoutUnit()).toInt();
+}
+
 static bool shouldSetDecorationAntialias(
     const Vector<AppliedTextDecoration>& decorations) {
   for (const AppliedTextDecoration& decoration : decorations) {
@@ -1140,12 +1183,18 @@ void InlineTextBoxPainter::paintDecorations(
   bool skipIntercepts =
       styleToUse.getTextDecorationSkip() & TextDecorationSkipInk;
 
+  // text-underline-position may flip underline and overline.
+  TextDecoration flipDecoration = TextDecorationNone;
+  ResolvedUnderlinePosition underlinePosition =

[drott, 2017/02/07 16:20:52]

Minor and optional: I find this flipDecoration XOR trick hard to read and a
surprising by-reference argument of this function. If you could find a more
readable way of expressing the idea here, that'd be good.

[kojii, 2017/02/07 23:13:44]

Ok, will add if() on every loop. I think it's rare to have multiple decorations,
so it shouldn't differ much.

+      resolveUnderlinePosition(styleToUse, &m_inlineTextBox, &flipDecoration);
+
   for (const AppliedTextDecoration& decoration : decorations) {
     TextDecoration lines = decoration.lines();
+    lines = static_cast<TextDecoration>(lines ^ flipDecoration);
     if ((lines & TextDecorationUnderline) && fontData) {
-      const int underlineOffset =
-          computeUnderlineOffset(styleToUse, fontData->getFontMetrics(),
-                                 &m_inlineTextBox, textDecorationThickness);
+      const int underlineOffset = computeUnderlineOffset(
+          underlinePosition, styleToUse, fontData->getFontMetrics(),
+          &m_inlineTextBox, textDecorationThickness);
       AppliedDecorationPainter decorationPainter(
           context, FloatPoint(localOrigin) + FloatPoint(0, underlineOffset),
           width.toFloat(), decoration, textDecorationThickness, doubleOffset, 1,
@@ -1160,9 +1209,12 @@ void InlineTextBoxPainter::paintDecorations(
       decorationPainter.paint();
     }
     if (lines & TextDecorationOverline) {
+      const int overlineOffset =
+          computeOverlineOffset(styleToUse, &m_inlineTextBox);
       AppliedDecorationPainter decorationPainter(
-          context, FloatPoint(localOrigin), width.toFloat(), decoration,
-          textDecorationThickness, -doubleOffset, 1, antialiasDecoration);
+          context, FloatPoint(localOrigin) + FloatPoint(0, overlineOffset),
+          width.toFloat(), decoration, textDecorationThickness, -doubleOffset,
+          1, antialiasDecoration);
       if (skipIntercepts) {
         textPainter.clipDecorationsStripe(
             -baseline + decorationPainter.decorationBounds().y() -