Attachment 357937 Details for Bug 192980 – Patch

[patch] Patch

bug-192980-20181221200932.patch (text/plain), 14.59 KB, created by cathiechen on 2018-12-21 04:09:34 PST

(hide)

Description:

Filename:

MIME Type:

Creator: cathiechen

Created: 2018-12-21 04:09:34 PST

Size: 14.59 KB

patch

obsolete

>Subversion Revision: 239150
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index fb4e46e47b2cbf2594c5501df7c78f2265a30379..3c0f879dc0e59c06ed86501d35f605bf1bbc69fe 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-12-21  cathie chen  <cathiechen@igalia.com>
>+
>+        Eliminate line-breaks and disappear of list marker
>+        https://bugs.webkit.org/show_bug.cgi?id=192980
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests (OOPS!).
>+
>+        * rendering/RenderListItem.cpp:
>+        (WebCore::RenderListItem::positionListMarker):
>+        (WebCore::RenderListItem::alignMarkerInBlockDirection):
>+        * rendering/RenderListItem.h:
>+        * rendering/RenderListMarker.h:
>+        * rendering/updating/RenderTreeBuilderList.cpp:
>+        (WebCore::getParentOfFirstLineBox):
>+        (WebCore::forceLogicalHeight):
>+        (WebCore::RenderTreeBuilder::List::updateItemMarker):
>+
> 2018-12-12  Simon Fraser  <simon.fraser@apple.com>
> 
>         REGRESSION (r238090): CAPCHA UI jumps to the wrong location
>diff --git a/Source/WebCore/rendering/RenderListItem.cpp b/Source/WebCore/rendering/RenderListItem.cpp
>index 653c1a64b69de3e906549687b67e37d4dd2c2205..6cc949313fd58a9930824c816415f9a6ad333361 100644
>--- a/Source/WebCore/rendering/RenderListItem.cpp
>+++ b/Source/WebCore/rendering/RenderListItem.cpp
>@@ -269,6 +269,9 @@ void RenderListItem::positionListMarker()
>     if (m_marker->isInside() || !m_marker->inlineBoxWrapper())
>         return;
> 
>+    if (m_needBlockDirectionAlign)
>+        alignMarkerInBlockDirection();
>+
>     LayoutUnit markerOldLogicalLeft = m_marker->logicalLeft();
>     LayoutUnit blockOffset;
>     LayoutUnit lineOffset;
>@@ -439,4 +442,67 @@ bool RenderListItem::isInReversedOrderedList() const
>     return is<HTMLOListElement>(list) && downcast<HTMLOListElement>(*list).isReversed();
> }
> 
>+// Align marker_inline_box in block direction according to line_box_root's baseline.
>+void RenderListItem::alignMarkerInBlockDirection()
>+{
>+    // Specify wether need to restore to the original baseline which is the baseline of marker parent.
>+    // Because we might adjust the position at the last layout pass.
>+    // So if there's no line box in baselineProvider make sure it back to its original position.
>+    bool backToOriginalBaseline = false;
>+    auto* baselineProvider = m_baselineProvider.get();
>+    if (!baselineProvider)
>+        backToOriginalBaseline = true;
>+    else {
>+        // Don't align marker if baselineProvider has a different writing-mode.
>+        // Just let marker positioned at the left-top of baselineProvider.
>+        if (baselineProvider->isWritingModeRoot())
>+            backToOriginalBaseline = true;
>+    }
>+
>+    InlineBox* markerInlineBox = m_marker->inlineBoxWrapper();
>+    RootInlineBox& markerRoot = markerInlineBox->root();
>+    if (baselineProvider && is<RenderBlockFlow>(baselineProvider)) {
>+        // If marker_ and baselineProvider share a same RootInlineBox, no need to align marker.
>+        if (downcast<RenderBlockFlow>(baselineProvider)->firstRootBox() == &markerRoot)
>+            return;
>+    }
>+
>+    LayoutUnit offset;
>+    if (!backToOriginalBaseline) {
>+        RenderBox* box = baselineProvider;
>+        offset = box->firstLineBaseline().value();
>+    }
>+
>+    if (backToOriginalBaseline || offset == -1) {
>+        baselineProvider = m_marker->containingBlock();
>+        RenderBox* box = baselineProvider;
>+        offset = box->firstLineBaseline().value();
>+    }
>+
>+    if (offset != -1) {
>+        for (RenderBox* o = baselineProvider; o != this; o = o->parentBox())
>+            offset += o->logicalTop();
>+
>+        // Compute markerInlineBox's baseline.
>+        // We shouldn't use firstLineBaseline here. firstLineBaseline is the baseline of root.
>+        // We should compute markerInlineBox's baseline instead.
>+        // BaselinePosition is workable when marker is an image.
>+        // However, when marker is text, BaselinePosition contains lineheight information.
>+        // So use markerFontMetrics.Ascent when marker is text.
>+        if (m_marker->isImage())
>+            offset -= markerInlineBox->baselinePosition(markerRoot.baselineType());
>+        else {
>+            const FontMetrics& markerFontMetrics = m_marker->style().fontMetrics();
>+            offset -= markerFontMetrics.ascent(markerRoot.baselineType());
>+        }
>+        offset -= markerInlineBox->logicalTop();
>+
>+        for (RenderBox* o = m_marker->parentBox(); o != this; o = o->parentBox())
>+            offset -= o->logicalTop();
>+
>+        if (offset)
>+            markerInlineBox->adjustBlockDirectionPosition(offset);
>+    }
>+}
>+
> } // namespace WebCore
>diff --git a/Source/WebCore/rendering/RenderListItem.h b/Source/WebCore/rendering/RenderListItem.h
>index b701567e8fd882718bb76d1d753aa97f5d099696..f760a369bb42792c7c3d19e2548edecb5fe1ee12 100644
>--- a/Source/WebCore/rendering/RenderListItem.h
>+++ b/Source/WebCore/rendering/RenderListItem.h
>@@ -61,6 +61,10 @@ public:
> 
>     bool isInReversedOrderedList() const;
> 
>+    void setNeedBlockDirectionAlign(bool need) { m_needBlockDirectionAlign = need; }
>+    bool needBlockDirectionAlign() { return m_needBlockDirectionAlign; }
>+    void setBaselineProvider(RenderBlock* provider) { m_baselineProvider = makeWeakPtr(provider); }
>+
> private:
> 
>     const char* renderName() const final { return "RenderListItem"; }
>@@ -82,10 +86,14 @@ private:
>     void updateValueNow() const;
>     void explicitValueChanged();
> 
>+    void alignMarkerInBlockDirection();
>+
>     WeakPtr<RenderListMarker> m_marker;
>+    WeakPtr<RenderBlock> m_baselineProvider;
>     mutable std::optional<int> m_value;
>     bool m_valueWasSetExplicitly { false };
>     bool m_notInList { false };
>+    bool m_needBlockDirectionAlign { false };
> };
> 
> bool isHTMLListElement(const Node&);
>diff --git a/Source/WebCore/rendering/RenderListMarker.h b/Source/WebCore/rendering/RenderListMarker.h
>index d9498fbe92bc5906149ea01ca917350309b1013d..627d7346c44425cf03772a48a6eeb97b97fd3b2f 100644
>--- a/Source/WebCore/rendering/RenderListMarker.h
>+++ b/Source/WebCore/rendering/RenderListMarker.h
>@@ -47,6 +47,8 @@ public:
> 
>     void updateMarginsAndContent();
> 
>+    bool isImage() const override;
>+
> #if !ASSERT_DISABLED
>     RenderListItem& listItem() const { return m_listItem; }
> #endif
>@@ -73,7 +75,6 @@ private:
>     LayoutUnit lineHeight(bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const override;
>     int baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const override;
> 
>-    bool isImage() const override;
>     bool isText() const { return !isImage(); }
> 
>     void setSelectionState(SelectionState) override;
>diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp b/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp
>index be436e096629e57652d53335326ad3f6f06763fb..e16b5d1cb2d5b0b7fb8fd1ce13e68c9b4dd0950d 100644
>--- a/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp
>+++ b/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp
>@@ -45,10 +45,13 @@ static RenderBlock* getParentOfFirstLineBox(RenderBlock& current, RenderObject&
>         if (child.isFloating() || child.isOutOfFlowPositioned())
>             continue;
> 
>-        if (!is<RenderBlock>(child) || is<RenderTable>(child) || is<RenderRubyAsBlock>(child))
>-            break;
>+        if (current.hasOverflowClip())
>+            return &current;
>+
>+        if (is<RenderBlock>(child) && (!is<RenderBlockFlow>(child) || (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())))
>+            return &downcast<RenderBlock>(child);
> 
>-        if (is<RenderBox>(child) && downcast<RenderBox>(child).isWritingModeRoot())
>+        if (!is<RenderBlock>(child) || is<RenderTable>(child) || is<RenderRubyAsBlock>(child))
>             break;
> 
>         if (is<RenderListItem>(current) && inQuirksMode && child.node() && isHTMLListElement(*child.node()))
>@@ -69,6 +72,15 @@ static RenderObject* firstNonMarkerChild(RenderBlock& parent)
>     return child;
> }
> 
>+void forceLogicalHeight(RenderElement& element, Length&& height)
>+{
>+    auto style = RenderStyle::clone(element.style());
>+    if (style.logicalHeight() == height)
>+        return;
>+    style.setLogicalHeight(WTFMove(height));
>+    element.setStyle(WTFMove(style));
>+}
>+
> RenderTreeBuilder::List::List(RenderTreeBuilder& builder)
>     : m_builder(builder)
> {
>@@ -98,27 +110,92 @@ void RenderTreeBuilder::List::updateItemMarker(RenderListItem& listItemRenderer)
> 
>     RenderElement* currentParent = markerRenderer->parent();
>     RenderBlock* newParent = getParentOfFirstLineBox(listItemRenderer, *markerRenderer);
>-    if (!newParent) {
>-        // If the marker is currently contained inside an anonymous box,
>-        // then we are the only item in that anonymous box (since no line box
>-        // parent was found). It's ok to just leave the marker where it is
>-        // in this case.
>-        if (currentParent && currentParent->isAnonymousBlock())
>+    // Create a zero height parent for marker, so that marker could be displayed or no line break between marker and content.
>+    // 1. Place marker as a child of <li>. Make sure don't share parent with empty inline elements which don't generate InlineBox.
>+    // 2. Manage the logicalHeight of markerContainer(marker's anonymous parent):
>+    // If marker is the only child of markerContainer, set LogicalHeight of markerContainer to 0px; else restore it to LogicalHeight of <li>.
>+    if (!markerRenderer->isInside() && newParent && (newParent->hasOverflowClip() || !is<RenderBlockFlow>(newParent) || newParent->isWritingModeRoot()))
>+        listItemRenderer.setNeedBlockDirectionAlign(true);
>+
>+    // Prepare for block direction align
>+    if (listItemRenderer.needBlockDirectionAlign()) {
>+        // Deal with the situation of render tree changed.
>+        RenderPtr<RenderObject> originalMarker;
>+        if (currentParent && currentParent->isAnonymous()) {
>+            // When list-position-style change from outside to inside, we need to restore LogicalHeight.
>+            // So add IsInside() here.
>+            if (markerRenderer->isInside() || markerRenderer->nextSibling()) {
>+                // Restore LogicalHeight.
>+                if (currentParent->style().logicalHeight().isZero())
>+                    forceLogicalHeight(*currentParent, Length(style.logicalHeight()));
>+
>+                // If currentParent isn't the ancestor of newParent, marker might generate a new empty line.
>+                // We need to remove marker here.E.g: <li><span><div>text<div><span></li>
>+                if (markerRenderer->isInside() || (newParent && !newParent->isDescendantOf(currentParent))) {
>+                    originalMarker = m_builder.detach(*currentParent, *markerRenderer);
>+                    currentParent = nullptr;
>+                }
>+            } else if (newParent)
>+                forceLogicalHeight(*currentParent, Length(0, Fixed));
>+        } else if (currentParent && currentParent != &listItemRenderer) {
>+            originalMarker = m_builder.detach(*currentParent, *markerRenderer);
>+            currentParent = nullptr;
>+        }
>+
>+        // Create markerContainer, set its height to 0px, and add it to li.
>+        if (!currentParent) {
>+            auto* beforeChild = firstNonMarkerChild(listItemRenderer);
>+            if (!markerRenderer->isInside() && beforeChild && !beforeChild->isInline()) {
>+                // Create markerContainer and set its LogicalHeight to 0px.
>+                auto markerContainer = listItemRenderer.createAnonymousBlock();
>+                if (newParent)
>+                    forceLogicalHeight(*markerContainer, Length(0, Fixed));
>+                if (originalMarker.get())
>+                    m_builder.attach(*markerContainer, WTFMove(originalMarker), firstNonMarkerChild(*markerContainer));
>+                else
>+                    m_builder.attach(*markerContainer, WTFMove(newMarkerRenderer), firstNonMarkerChild(*markerContainer));
>+                m_builder.attach(listItemRenderer, WTFMove(markerContainer), beforeChild);
>+            } else {
>+                if (markerRenderer->isInside()) {
>+                    listItemRenderer.setNeedBlockDirectionAlign(false);
>+                    if (!newParent)
>+                        newParent = &listItemRenderer;
>+                    if (originalMarker.get())
>+                        m_builder.attach(*newParent, WTFMove(originalMarker), firstNonMarkerChild(*newParent));
>+                    else
>+                        m_builder.attach(*newParent, WTFMove(newMarkerRenderer), firstNonMarkerChild(*newParent));
>+
>+                } else if (originalMarker.get())
>+                    m_builder.attach(listItemRenderer, WTFMove(originalMarker), beforeChild);
>+                else
>+                    m_builder.attach(listItemRenderer, WTFMove(newMarkerRenderer), beforeChild);
>+            }
>+        }
>+
>+        listItemRenderer.setBaselineProvider(newParent);
>+    } else {
>+        if (!newParent) {
>+            // If the marker is currently contained inside an anonymous box,
>+            // then we are the only item in that anonymous box (since no line box
>+            // parent was found). It's ok to just leave the marker where it is
>+            // in this case.
>+            if (currentParent && currentParent->isAnonymousBlock())
>+                return;
>+            if (auto* multiColumnFlow = listItemRenderer.multiColumnFlow())
>+                newParent = multiColumnFlow;
>+            else
>+                newParent = &listItemRenderer;
>+        }
>+
>+        if (newParent == currentParent)
>             return;
>-        if (auto* multiColumnFlow = listItemRenderer.multiColumnFlow())
>-            newParent = multiColumnFlow;
>+
>+        if (currentParent)
>+            m_builder.attach(*newParent, m_builder.detach(*currentParent, *markerRenderer, RenderTreeBuilder::CanCollapseAnonymousBlock::No), firstNonMarkerChild(*newParent));
>         else
>-            newParent = &listItemRenderer;
>+            m_builder.attach(*newParent, WTFMove(newMarkerRenderer), firstNonMarkerChild(*newParent));
>     }
> 
>-    if (newParent == currentParent)
>-        return;
>-
>-    if (currentParent)
>-        m_builder.attach(*newParent, m_builder.detach(*currentParent, *markerRenderer, RenderTreeBuilder::CanCollapseAnonymousBlock::No), firstNonMarkerChild(*newParent));
>-    else
>-        m_builder.attach(*newParent, WTFMove(newMarkerRenderer), firstNonMarkerChild(*newParent));
>-
>     // If current parent is an anonymous block that has lost all its children, destroy it.
>     if (currentParent && currentParent->isAnonymousBlock() && !currentParent->firstChild() && !downcast<RenderBlock>(*currentParent).continuation())
>         m_builder.destroy(*currentParent);

Actions: View | Formatted Diff | Diff

Attachments on bug 192980: 357937 | 357938 | 357939 | 357940 | 357942 | 357943 | 357944 | 357945 | 357946 | 358035 | 358036 | 358037 | 358038 | 358039 | 358041 | 358063 | 358065 | 358066 | 358067 | 358068 | 358069 | 358105 | 358106 | 358109 | 358114 | 358171 | 358673 | 358674 | 358789 | 358793 | 358794 | 358797 | 358881 | 359016 | 359606 | 366487