Rework list marker spacing for better web compatibility.

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  * Copyright (C) 2010 Daniel Bates (dbates@intudata.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 18 matching lines @@
 #include "core/layout/LayoutAnalyzer.h"
 #include "core/layout/LayoutListItem.h"
 #include "core/layout/TextRunConstructor.h"
 #include "core/paint/ListMarkerPainter.h"
 #include "core/paint/PaintLayer.h"
 #include "platform/fonts/Font.h"
 #include "wtf/text/StringBuilder.h"
 
 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 };
 
 static String toRoman(int number, bool upper)
 {
     // FIXME: CSS3 describes how to make this work for much larger numbers,
     // using overbars and special characters. It also specifies the characters
     // in the range U+2160 to U+217F instead of standard ASCII ones.
     ASSERT(number >= 1 && number <= 3999);
 
@@ skipping 977 matching lines @@
 {
     return m_image && !m_image->errorOccurred();
 }
 
 LayoutRect LayoutListMarker::localSelectionRect() const
 {
     InlineBox* box = inlineBoxWrapper();
     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());
 }
 
 void LayoutListMarker::paint(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     ListMarkerPainter(*this).paint(paintInfo, paintOffset);
 }
 
 void LayoutListMarker::layout()
@@ skipping 34 matching lines @@
     else
         setShouldDoFullPaintInvalidation();
 }
 
 void LayoutListMarker::updateMarginsAndContent()
 {
     updateContent();
     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;
+        return NoneSimpleStyle;
+    case Disc:
     case Circle:
-    case Disc:
     case Square:
-        m_text = listMarkerText(type, 0); // value is ignored for these types
-        break;
+        return SymbolSimpleStyle;
     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 Hebrew:
     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:
-        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());
     updateContent();
 
     if (isImage()) {
         LayoutSize imageSize = m_image->imageSize(this, style()->effectiveZoom());
         m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = style()->isHorizontalWritingMode() ? imageSize.width() : imageSize.height();
         clearPreferredLogicalWidthsDirty();
         updateMargins();
         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;
     }
 
     m_minPreferredLogicalWidth = logicalWidth;
     m_maxPreferredLogicalWidth = logicalWidth;
 
     clearPreferredLogicalWidthsDirty();
 
     updateMargins();
 }
 
 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));
     mutableStyleRef().setMarginEnd(Length(marginEnd, Fixed));
 }
 
 LayoutUnit LayoutListMarker::lineHeight(bool firstLine, LineDirectionMode direction, LinePositionMode linePositionMode) const
 {
     if (!isImage())
         return m_listItem->lineHeight(firstLine, direction, PositionOfInteriorLineBoxes);
@@ skipping 12 matching lines @@
 {
     return m_listItem->notInList() || style()->listStylePosition() == INSIDE;
 }
 
 IntRect LayoutListMarker::getRelativeMarkerRect() const
 {
     if (isImage())
         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);
+    switch (simplifiedListStyle()) {
+    case NoneSimpleStyle:
+        return IntRect();
+    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 NoneListStyle:
-        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 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()) {
         relativeRect = relativeRect.transposedRect();
         relativeRect.setX(size().width() - relativeRect.x() - relativeRect.width());
     }
 
     return relativeRect;
 }
 
 void LayoutListMarker::setSelectionState(SelectionState state)
 {
     // The selection state for our containing block hierarchy is updated by the base class call.
     LayoutBox::setSelectionState(state);
 
     if (inlineBoxWrapper() && canUpdateSelectionOnRootLineBoxes())
         inlineBoxWrapper()->root().setHasSelectedChildren(state != SelectionNone);
 }
 
 LayoutRect LayoutListMarker::selectionRectForPaintInvalidation(const LayoutBoxModelObject* paintInvalidationContainer) const
 {
     ASSERT(!needsLayout());
 
     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);
     // FIXME: groupedMapping() leaks the squashing abstraction.
     if (paintInvalidationContainer->layer()->groupedMapping())
         PaintLayer::mapRectToPaintBackingCoordinates(paintInvalidationContainer, rect);
     return rect;
 }
 
 void LayoutListMarker::listItemStyleDidChange()
 {
     RefPtr<ComputedStyle> newStyle = ComputedStyle::create();
     // The marker always inherits from the list item, regardless of where it might end
     // up (e.g., in some deeply nested line box). See CSS3 spec.
     newStyle->inheritFrom(m_listItem->styleRef());
     if (style()) {
         // Reuse the current margins. Otherwise resetting the margins to initial values
         // would trigger unnecessary layout.
         newStyle->setMarginStart(style()->marginStart());
         newStyle->setMarginEnd(style()->marginRight());
     }
     setStyle(newStyle.release());
 }
 
 } // namespace blink