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

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/paint/InlineTextBoxPainter.h"
 
 #include "core/editing/CompositionUnderline.h"
 #include "core/editing/Editor.h"
 #include "core/editing/markers/DocumentMarkerController.h"
 #include "core/frame/LocalFrame.h"
@@ skipping 18 matching lines @@
     const InlineTextBox& textBox) {
   const unsigned startOffset = marker.startOffset() > textBox.start()
                                    ? marker.startOffset() - textBox.start()
                                    : 0U;
   const unsigned endOffset =
       std::min(marker.endOffset() - textBox.start(), textBox.len());
   return std::make_pair(startOffset, endOffset);
 }
 }
 
+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();
   for (LineLayoutItem current = inlineTextBox->parent()->getLineLayoutItem();
        ;) {
     if (current.isLayoutBlock())
       return current;
     if (!current.isLayoutInline() || current.isRubyText())
       return nullptr;
 
@@ skipping 42 matching lines @@
 
   if (fontMetrics.underlinePosition())
     gap = -fontMetrics.underlinePosition();
   else
     gap = std::max<int>(1, ceilf(textDecorationThickness / 2.f));
 
   // Position underline near the alphabetic baseline.
   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);
       offset = inlineTextBox->logicalHeight() + std::max(offset, LayoutUnit());
       return offset.toInt();
     }
   }
 }
 
+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) {
     TextDecorationStyle decorationStyle = decoration.style();
     if (decorationStyle == TextDecorationStyleDotted ||
         decorationStyle == TextDecorationStyleDashed)
       return true;
   }
   return false;
 }
@@ skipping 1000 matching lines @@
 
   context.setStrokeThickness(textDecorationThickness);
 
   bool antialiasDecoration = shouldSetDecorationAntialias(decorations);
 
   // Offset between lines - always non-zero, so lines never cross each other.
   float doubleOffset = textDecorationThickness + 1.f;
   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,
           antialiasDecoration);
       if (skipIntercepts) {
         textPainter.clipDecorationsStripe(
             -baseline + decorationPainter.decorationBounds().y() -
                 FloatPoint(localOrigin).y(),
             decorationPainter.decorationBounds().height(),
             textDecorationThickness);
       }
       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() -
                 FloatPoint(localOrigin).y(),
             decorationPainter.decorationBounds().height(),
             textDecorationThickness);
       }
       decorationPainter.paint();
     }
     if (lines & TextDecorationLineThrough) {
@@ skipping 152 matching lines @@
 
   LayoutRect boxRect(boxOrigin, LayoutSize(m_inlineTextBox.logicalWidth(),
                                            m_inlineTextBox.logicalHeight()));
   context.clip(FloatRect(boxRect));
   context.drawHighlightForText(font, run, FloatPoint(boxOrigin),
                                boxRect.height().toInt(), color,
                                paintOffsets.first, paintOffsets.second);
 }
 
 }  // namespace blink