Rework list marker spacing for better web compatibility.

third_party/WebKit/Source/core/layout/LayoutListMarker.cpp

Index: third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp b/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
index e44011989136eb710c69f07f7d3fbbd11ed657bc..fe1550abdd90d77779d3fae9782bee4b69ef99cd 100644
--- a/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
@@ -36,6 +36,9 @@
 
 namespace blink {
 
+// TODO(wkorman): A seemingly arbitrary number tacked on following any image
+// and included as part of margin for any text. Look into history, other
+// browser behavior, and update or remove as appropriate.
 const int cMarkerPadding = 7;

[eae, 2015/10/12 06:02:01]

cMarkerPaddingPx (assuming it is in pixels).

[wkorman, 2015/10/14 01:55:30]

Done.

 
 enum SequenceType { NumericSequence, AlphabeticSequence };
@@ -1034,8 +1037,11 @@ LayoutRect LayoutListMarker::localSelectionRect() const
     if (!box)
         return LayoutRect(LayoutPoint(), size());
     RootInlineBox& root = inlineBoxWrapper()->root();
-    LayoutUnit newLogicalTop = root.block().style()->isFlippedBlocksWritingMode() ? inlineBoxWrapper()->logicalBottom() - root.selectionBottom() : root.selectionTop() - inlineBoxWrapper()->logicalTop();
-    if (root.block().style()->isHorizontalWritingMode())
+    const ComputedStyle* blockStyle = root.block().style();
+    LayoutUnit newLogicalTop = blockStyle->isFlippedBlocksWritingMode()
+        ? inlineBoxWrapper()->logicalBottom() - root.selectionBottom()
+        : root.selectionTop() - inlineBoxWrapper()->logicalTop();
+    if (blockStyle->isHorizontalWritingMode())
         return LayoutRect(0, newLogicalTop, size().width(), root.selectionHeight());
     return LayoutRect(newLogicalTop, 0, root.selectionHeight(), size().height());
 }
@@ -1090,34 +1096,15 @@ void LayoutListMarker::updateMarginsAndContent()
     updateMargins();
 }
 
-void LayoutListMarker::updateContent()
+LayoutListMarker::SimplifiedListStyle LayoutListMarker::simplifiedListStyle() const
 {
-    // FIXME: This if-statement is just a performance optimization, but it's messy to use the preferredLogicalWidths dirty bit for this.
-    // It's unclear if this is a premature optimization.
-    if (!preferredLogicalWidthsDirty())
-        return;
-
-    m_text = "";
-
-    if (isImage()) {
-        // FIXME: This is a somewhat arbitrary width.  Generated images for markers really won't become particularly useful
-        // until we support the CSS3 marker pseudoclass to allow control over the width and height of the marker box.
-        int bulletWidth = style()->fontMetrics().ascent() / 2;
-        IntSize defaultBulletSize(bulletWidth, bulletWidth);
-        IntSize imageSize = calculateImageIntrinsicDimensions(m_image.get(), defaultBulletSize, DoNotScaleByEffectiveZoom);
-        m_image->setContainerSizeForLayoutObject(this, imageSize, style()->effectiveZoom());
-        return;
-    }
-
-    EListStyleType type = style()->listStyleType();
-    switch (type) {
+    switch (style()->listStyleType()) {
     case NoneListStyle:
-        break;
-    case Circle:
+        return NoneSimpleStyle;
     case Disc:
+    case Circle:
     case Square:
-        m_text = listMarkerText(type, 0); // value is ignored for these types
-        break;
+        return SymbolSimpleStyle;
     case ArabicIndic:
     case Armenian:
     case Bengali:
@@ -1135,12 +1122,12 @@ void LayoutListMarker::updateContent()
     case Georgian:
     case Gujarati:
     case Gurmukhi:
+    case Hebrew:
     case Hangul:
     case HangulConsonant:
     case KoreanHangulFormal:
     case KoreanHanjaFormal:
     case KoreanHanjaInformal:
-    case Hebrew:
     case Hiragana:
     case HiraganaIroha:
     case Kannada:
@@ -1170,11 +1157,55 @@ void LayoutListMarker::updateContent()
     case UpperLatin:
     case UpperRoman:
     case Urdu:
-        m_text = listMarkerText(type, m_listItem->value());
+        return LanguageSimpleStyle;
+    default:
+        return NoneSimpleStyle;
+    }
+}
+
+void LayoutListMarker::updateContent()
+{
+    // FIXME: This if-statement is just a performance optimization, but it's messy to use the preferredLogicalWidths dirty bit for this.
+    // It's unclear if this is a premature optimization.
+    if (!preferredLogicalWidthsDirty())
+        return;
+
+    m_text = "";
+
+    if (isImage()) {
+        // FIXME: This is a somewhat arbitrary width.  Generated images for markers really won't become particularly useful
+        // until we support the CSS3 marker pseudoclass to allow control over the width and height of the marker box.
+        int bulletWidth = style()->fontMetrics().ascent() / 2;

[eae, 2015/10/12 06:02:01]

Is flooring the result the tight thing to do here? If so please make it explicit
by adding a floorf call.

[wkorman, 2015/10/14 01:55:30]

Likely to revise this in subsequent changes when I review image lists in more
detail. This preserves existing behavior for now.

Is floorf the right thing to use with ints? ascent is an int as well so it's
implicitly floored/truncated, I also prefer being explicit, just not sure what
Blink style is here.

+        IntSize defaultBulletSize(bulletWidth, bulletWidth);
+        IntSize imageSize = calculateImageIntrinsicDimensions(m_image.get(), defaultBulletSize, DoNotScaleByEffectiveZoom);
+        m_image->setContainerSizeForLayoutObject(this, imageSize, style()->effectiveZoom());
+        return;
+    }
+
+    switch (simplifiedListStyle()) {
+    case NoneSimpleStyle:
+        break;
+    case SymbolSimpleStyle:
+        m_text = listMarkerText(style()->listStyleType(), 0); // value is ignored for these types
+        break;
+    case LanguageSimpleStyle:
+        m_text = listMarkerText(style()->listStyleType(), m_listItem->value());
         break;
     }
 }
 
+LayoutUnit LayoutListMarker::getWidthOfTextWithSuffix(UChar* suffix, int suffixLength) const
+{
+    if (m_text.isEmpty())
+        return 0;
+
+    const Font& font = style()->font();
+    LayoutUnit itemWidth = font.width(m_text);
+    TextRun run = constructTextRun(font, suffix, suffixLength, styleRef(), style()->direction());

[eae, 2015/10/12 06:02:01]

How about constructing a text run with both the text and the suffix? Should make
bidi handling easaier. Obviously that'll require that we paint them together
too... 

[wkorman, 2015/10/14 01:55:30]

Added TODO to look at this, I agree there's redundant logic this could clean up.

+    LayoutUnit suffixSpaceWidth = font.width(run);
+    return itemWidth + suffixSpaceWidth;
+}
+
 void LayoutListMarker::computePreferredLogicalWidths()
 {
     ASSERT(preferredLogicalWidthsDirty());
@@ -1188,77 +1219,24 @@ void LayoutListMarker::computePreferredLogicalWidths()
         return;
     }
 
-    const Font& font = style()->font();
-
     LayoutUnit logicalWidth = 0;
-    EListStyleType type = style()->listStyleType();
-    switch (type) {
-    case NoneListStyle:
+    switch (simplifiedListStyle()) {
+    case NoneSimpleStyle:
         break;
-    case Circle:
-    case Disc:
-    case Square:
-        logicalWidth = (font.fontMetrics().ascent() * 2 / 3 + 1) / 2 + 2;
+    case SymbolSimpleStyle: {
+        // For these styles, the text of the marker will be the single character itself,
+        // and so the only suffix will be a single space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffix[1] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()) };
+        logicalWidth = getWidthOfTextWithSuffix(suffix, 1);
+        }
         break;
-    case ArabicIndic:
-    case Armenian:
-    case Bengali:
-    case Cambodian:
-    case CJKIdeographic:
-    case CjkEarthlyBranch:
-    case CjkHeavenlyStem:
-    case DecimalLeadingZero:
-    case DecimalListStyle:
-    case Devanagari:
-    case EthiopicHalehame:
-    case EthiopicHalehameAm:
-    case EthiopicHalehameTiEr:
-    case EthiopicHalehameTiEt:
-    case Georgian:
-    case Gujarati:
-    case Gurmukhi:
-    case Hangul:
-    case HangulConsonant:
-    case KoreanHangulFormal:
-    case KoreanHanjaFormal:
-    case KoreanHanjaInformal:
-    case Hebrew:
-    case Hiragana:
-    case HiraganaIroha:
-    case Kannada:
-    case Katakana:
-    case KatakanaIroha:
-    case Khmer:
-    case Lao:
-    case LowerAlpha:
-    case LowerArmenian:
-    case LowerGreek:
-    case LowerLatin:
-    case LowerRoman:
-    case Malayalam:
-    case Mongolian:
-    case Myanmar:
-    case Oriya:
-    case Persian:
-    case SimpChineseFormal:
-    case SimpChineseInformal:
-    case Telugu:
-    case Thai:
-    case Tibetan:
-    case TradChineseFormal:
-    case TradChineseInformal:
-    case UpperAlpha:
-    case UpperArmenian:
-    case UpperLatin:
-    case UpperRoman:
-    case Urdu:
-        if (m_text.isEmpty()) {
-            logicalWidth = 0;
-        } else {
-            LayoutUnit itemWidth = font.width(m_text);
-            UChar suffixSpace[2] = { listMarkerSuffix(type, m_listItem->value()), ' ' };
-            LayoutUnit suffixSpaceWidth = font.width(constructTextRun(font, suffixSpace, 2, styleRef(), style()->direction()));
-            logicalWidth = itemWidth + suffixSpaceWidth;
+    case LanguageSimpleStyle: {
+        // For these styles, we have some kind of a language-specific separating
+        // character, followed by a space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffixSpace[2] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()), ' ' };
+        logicalWidth = getWidthOfTextWithSuffix(suffixSpace, 2);
         }
         break;
     }
@@ -1273,65 +1251,30 @@ void LayoutListMarker::computePreferredLogicalWidths()
 
 void LayoutListMarker::updateMargins()
 {
-    const FontMetrics& fontMetrics = style()->fontMetrics();
-
     LayoutUnit marginStart = 0;
     LayoutUnit marginEnd = 0;
 
     if (isInside()) {
         if (isImage()) {
             marginEnd = cMarkerPadding;
-        } else {
-            switch (style()->listStyleType()) {
-            case Disc:
-            case Circle:
-            case Square:
-                marginStart = -1;
-                marginEnd = fontMetrics.ascent() - minPreferredLogicalWidth() + 1;
-                break;
-            default:
-                break;
-            }
+        } else if (simplifiedListStyle() == SymbolSimpleStyle) {
+            marginStart = minPreferredLogicalWidth();
+            marginEnd = 0;
         }
     } else {
         if (style()->isLeftToRightDirection()) {
-            if (isImage()) {
+            if (isImage())
                 marginStart = -minPreferredLogicalWidth() - cMarkerPadding;
-            } else {
-                int offset = fontMetrics.ascent() * 2 / 3;
-                switch (style()->listStyleType()) {
-                case Disc:
-                case Circle:
-                case Square:
-                    marginStart = -offset - cMarkerPadding - 1;
-                    break;
-                case NoneListStyle:
-                    break;
-                default:
-                    marginStart = m_text.isEmpty() ? LayoutUnit() : -minPreferredLogicalWidth() - offset / 2;
-                }
-            }
+            else if (style()->listStyleType() != NoneListStyle)
+                marginStart = m_text.isEmpty() ? LayoutUnit() : -minPreferredLogicalWidth();
             marginEnd = -marginStart - minPreferredLogicalWidth();
         } else {
-            if (isImage()) {
+            if (isImage())
                 marginEnd = cMarkerPadding;
-            } else {
-                int offset = fontMetrics.ascent() * 2 / 3;
-                switch (style()->listStyleType()) {
-                case Disc:
-                case Circle:
-                case Square:
-                    marginEnd = offset + cMarkerPadding + 1 - minPreferredLogicalWidth();
-                    break;
-                case NoneListStyle:
-                    break;
-                default:
-                    marginEnd = m_text.isEmpty() ? 0 : offset / 2;
-                }
-            }
+            else if (style()->listStyleType() != NoneListStyle)
+                marginEnd = 0;
             marginStart = -marginEnd - minPreferredLogicalWidth();
         }
-
     }
 
     mutableStyleRef().setMarginStart(Length(marginStart, Fixed));
@@ -1364,79 +1307,27 @@ IntRect LayoutListMarker::getRelativeMarkerRect() const
         return IntRect(0, 0, m_image->imageSize(this, style()->effectiveZoom()).width(), m_image->imageSize(this, style()->effectiveZoom()).height());
 
     IntRect relativeRect;
-    EListStyleType type = style()->listStyleType();
-    switch (type) {
-    case Disc:
-    case Circle:
-    case Square: {
-        // FIXME: Are these particular rounding rules necessary?
-        const FontMetrics& fontMetrics = style()->fontMetrics();
-        int ascent = fontMetrics.ascent();
-        int bulletWidth = (ascent * 2 / 3 + 1) / 2;
-        relativeRect = IntRect(1, 3 * (ascent - ascent * 2 / 3) / 2, bulletWidth, bulletWidth);
-        break;
-    }
-    case NoneListStyle:
+    switch (simplifiedListStyle()) {
+    case NoneSimpleStyle:
         return IntRect();
-    case ArabicIndic:
-    case Armenian:
-    case Bengali:
-    case Cambodian:
-    case CJKIdeographic:
-    case CjkEarthlyBranch:
-    case CjkHeavenlyStem:
-    case DecimalLeadingZero:
-    case DecimalListStyle:
-    case Devanagari:
-    case EthiopicHalehame:
-    case EthiopicHalehameAm:
-    case EthiopicHalehameTiEr:
-    case EthiopicHalehameTiEt:
-    case Georgian:
-    case Gujarati:
-    case Gurmukhi:
-    case Hangul:
-    case KoreanHangulFormal:
-    case KoreanHanjaFormal:
-    case KoreanHanjaInformal:
-    case HangulConsonant:
-    case Hebrew:
-    case Hiragana:
-    case HiraganaIroha:
-    case Kannada:
-    case Katakana:
-    case KatakanaIroha:
-    case Khmer:
-    case Lao:
-    case LowerAlpha:
-    case LowerArmenian:
-    case LowerGreek:
-    case LowerLatin:
-    case LowerRoman:
-    case Malayalam:
-    case Mongolian:
-    case Myanmar:
-    case Oriya:
-    case Persian:
-    case SimpChineseFormal:
-    case SimpChineseInformal:
-    case Telugu:
-    case Thai:
-    case Tibetan:
-    case TradChineseFormal:
-    case TradChineseInformal:
-    case UpperAlpha:
-    case UpperArmenian:
-    case UpperLatin:
-    case UpperRoman:
-    case Urdu:
-        if (m_text.isEmpty())
-            return IntRect();
-        const Font& font = style()->font();
-        int itemWidth = font.width(m_text);
-        UChar suffixSpace[2] = { listMarkerSuffix(type, m_listItem->value()), ' ' };
-        int suffixSpaceWidth = font.width(constructTextRun(font, suffixSpace, 2, styleRef(), style()->direction()));
-        relativeRect = IntRect(0, 0, itemWidth + suffixSpaceWidth, font.fontMetrics().height());
+    case SymbolSimpleStyle: {
+        // For these styles, the text of the marker will be the single character itself,
+        // and so the only suffix will be a single space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffix[1] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()) };
+        LayoutUnit itemWidth = getWidthOfTextWithSuffix(suffix, 1);
+        relativeRect = IntRect(0, 0, itemWidth, style()->font().fontMetrics().height());
+        }
+        break;
+    case LanguageSimpleStyle: {
+        // For these styles, we have some kind of a language-specific separating
+        // character, followed by a space character to provide sufficient
+        // separation between the list marker text and the associated list item text.
+        UChar suffixSpace[2] = { listMarkerSuffix(style()->listStyleType(), m_listItem->value()), ' ' };
+        LayoutUnit itemWidth = getWidthOfTextWithSuffix(suffixSpace, 2);
+        relativeRect = IntRect(0, 0, itemWidth, style()->font().fontMetrics().height());
+        }
+        break;
     }
 
     if (!style()->isHorizontalWritingMode()) {
@@ -1463,6 +1354,8 @@ LayoutRect LayoutListMarker::selectionRectForPaintInvalidation(const LayoutBoxMo
     if (selectionState() == SelectionNone || !inlineBoxWrapper())
         return LayoutRect();
 
+    // TODO(wkorman): Make sure the output rect from this method matches what we expect to see.
+
     RootInlineBox& root = inlineBoxWrapper()->root();
     LayoutRect rect(0, root.selectionTop() - location().y(), size().width(), root.selectionHeight());
     mapRectToPaintInvalidationBacking(paintInvalidationContainer, rect, 0);