RenderBox::parent/firstChild/nextSibling/previousSiblingBox() functions should type...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Mar 2018 20:51:06 +0000 (20:51 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Mar 2018 20:51:06 +0000 (20:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184032
<rdar://problem/38384984>

Reviewed by Antti Koivisto.

Source/WebCore:

We cannot rely on the correctness of the render tree structure when querying for parent/child/next and previous
sibling since some features (multicolumn/spanners) move subtrees out of their original position (which is highly
undesired and should not be encouraged at all though).
It should also be noted that these functions are not equivalent of typeOfChildren<RenderBox> and the following usage
    for (auto* boxChild = firstChildBox(); boxChild; boxChild = boxChild->nextSiblingBox())
can lead to unexpected result.

Test: fast/multicol/parent-box-when-spanner-is-present.html

* rendering/RenderBox.h:
(WebCore::RenderBox::parentBox const):
(WebCore::RenderBox::firstChildBox const):
(WebCore::RenderBox::lastChildBox const):
(WebCore::RenderBox::previousSiblingBox const):
(WebCore::RenderBox::nextSiblingBox const):
* rendering/RenderListItem.cpp:
(WebCore::RenderListItem::positionListMarker):
* rendering/RenderListMarker.cpp:
(WebCore::RenderListMarker::layout):
* rendering/RenderMultiColumnSet.cpp:
(WebCore::RenderMultiColumnSet::updateLogicalWidth):

LayoutTests:

* fast/multicol/parent-box-when-spanner-is-present-expected.txt: Added.
* fast/multicol/parent-box-when-spanner-is-present.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230004 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/multicol/parent-box-when-spanner-is-present-expected.txt [new file with mode: 0644]
LayoutTests/fast/multicol/parent-box-when-spanner-is-present.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBox.h
Source/WebCore/rendering/RenderListItem.cpp
Source/WebCore/rendering/RenderListMarker.cpp
Source/WebCore/rendering/RenderMultiColumnSet.cpp

index b8bc09eb1c6beb6078196224bfc156b925cb6ee3..8b8643f387e70048914bde0a658bc45971c113c2 100644 (file)
@@ -1,3 +1,14 @@
+2018-03-27  Zalan Bujtas  <zalan@apple.com>
+
+        RenderBox::parent/firstChild/nextSibling/previousSiblingBox() functions should type check.
+        https://bugs.webkit.org/show_bug.cgi?id=184032
+        <rdar://problem/38384984>
+
+        Reviewed by Antti Koivisto.
+
+        * fast/multicol/parent-box-when-spanner-is-present-expected.txt: Added.
+        * fast/multicol/parent-box-when-spanner-is-present.html: Added.
+
 2018-03-27  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Stop using internals.pauseAnimationAtTimeOnElement() in favor of Web Animations API for animations tests
diff --git a/LayoutTests/fast/multicol/parent-box-when-spanner-is-present-expected.txt b/LayoutTests/fast/multicol/parent-box-when-spanner-is-present-expected.txt
new file mode 100644 (file)
index 0000000..2b9632d
--- /dev/null
@@ -0,0 +1,2 @@
+PASS if no crash
+
diff --git a/LayoutTests/fast/multicol/parent-box-when-spanner-is-present.html b/LayoutTests/fast/multicol/parent-box-when-spanner-is-present.html
new file mode 100644 (file)
index 0000000..4ba929f
--- /dev/null
@@ -0,0 +1,6 @@
+PASS if no crash
+<ul style="column-width: 10px;"><li style="border-bottom-style: solid;"><div style="column-span: all"><input></div></li></ul>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
\ No newline at end of file
index 583373f0ce1da70c295f54bcf8780b095be29b81..ba7780b608504faf3683360d88840cdf27cb2340 100644 (file)
@@ -1,3 +1,33 @@
+2018-03-27  Zalan Bujtas  <zalan@apple.com>
+
+        RenderBox::parent/firstChild/nextSibling/previousSiblingBox() functions should type check.
+        https://bugs.webkit.org/show_bug.cgi?id=184032
+        <rdar://problem/38384984>
+
+        Reviewed by Antti Koivisto.
+
+        We cannot rely on the correctness of the render tree structure when querying for parent/child/next and previous
+        sibling since some features (multicolumn/spanners) move subtrees out of their original position (which is highly
+        undesired and should not be encouraged at all though).
+        It should also be noted that these functions are not equivalent of typeOfChildren<RenderBox> and the following usage
+            for (auto* boxChild = firstChildBox(); boxChild; boxChild = boxChild->nextSiblingBox())
+        can lead to unexpected result.
+        Test: fast/multicol/parent-box-when-spanner-is-present.html
+
+        * rendering/RenderBox.h:
+        (WebCore::RenderBox::parentBox const):
+        (WebCore::RenderBox::firstChildBox const):
+        (WebCore::RenderBox::lastChildBox const):
+        (WebCore::RenderBox::previousSiblingBox const):
+        (WebCore::RenderBox::nextSiblingBox const):
+        * rendering/RenderListItem.cpp:
+        (WebCore::RenderListItem::positionListMarker):
+        * rendering/RenderListMarker.cpp:
+        (WebCore::RenderListMarker::layout):
+        * rendering/RenderMultiColumnSet.cpp:
+        (WebCore::RenderMultiColumnSet::updateLogicalWidth):
+
 2018-03-27  Brent Fulgham  <bfulgham@apple.com>
 
         Further refine cookie read/write logging
index ad4491bd7cada4306a5e41124194c2bbb262141e..8fcdfd481b6b9af2689ae36a2f946d8eb88e666f 100644 (file)
@@ -61,10 +61,6 @@ public:
     // Returns false for the body renderer if its background is propagated to the root.
     bool paintsOwnBackground() const;
 
-    // Use this with caution! No type checking is done!
-    RenderBox* firstChildBox() const;
-    RenderBox* lastChildBox() const;
-
     LayoutUnit x() const { return m_frameRect.x(); }
     LayoutUnit y() const { return m_frameRect.y(); }
     LayoutUnit width() const { return m_frameRect.width(); }
@@ -178,10 +174,12 @@ public:
     FloatRect repaintRectInLocalCoordinates() const override { return borderBoxRect(); }
     FloatRect objectBoundingBox() const override { return borderBoxRect(); }
 
-    // Use this with caution! No type checking is done!
+    // Note these functions are not equivalent of childrenOfType<RenderBox>
+    RenderBox* parentBox() const;
+    RenderBox* firstChildBox() const;
+    RenderBox* lastChildBox() const;
     RenderBox* previousSiblingBox() const;
     RenderBox* nextSiblingBox() const;
-    RenderBox* parentBox() const;
 
     // Visual and layout overflow are in the coordinate space of the box.  This means that they aren't purely physical directions.
     // For horizontal-tb and vertical-lr they will match physical directions, but for horizontal-bt and vertical-rl, the top/bottom and left/right
@@ -740,29 +738,49 @@ private:
     static bool s_hadOverflowClip;
 };
 
-inline RenderBox* RenderBox::previousSiblingBox() const
+inline RenderBox* RenderBox::parentBox() const
 {
-    return downcast<RenderBox>(previousSibling());
+    if (is<RenderBox>(parent()))
+        return downcast<RenderBox>(parent());
+
+    ASSERT(!parent());
+    return nullptr;
 }
 
-inline RenderBox* RenderBox::nextSiblingBox() const
-{ 
-    return downcast<RenderBox>(nextSibling());
+inline RenderBox* RenderBox::firstChildBox() const
+{
+    if (is<RenderBox>(firstChild()))
+        return downcast<RenderBox>(firstChild());
+
+    ASSERT(!firstChild());
+    return nullptr;
 }
 
-inline RenderBox* RenderBox::parentBox() const
+inline RenderBox* RenderBox::lastChildBox() const
 {
-    return downcast<RenderBox>(parent());
+    if (is<RenderBox>(lastChild()))
+        return downcast<RenderBox>(lastChild());
+
+    ASSERT(!lastChild());
+    return nullptr;
 }
 
-inline RenderBox* RenderBox::firstChildBox() const
+inline RenderBox* RenderBox::previousSiblingBox() const
 {
-    return downcast<RenderBox>(firstChild());
+    if (is<RenderBox>(previousSibling()))
+        return downcast<RenderBox>(previousSibling());
+
+    ASSERT(!previousSibling());
+    return nullptr;
 }
 
-inline RenderBox* RenderBox::lastChildBox() const
+inline RenderBox* RenderBox::nextSiblingBox() const
 {
-    return downcast<RenderBox>(lastChild());
+    if (is<RenderBox>(nextSibling()))
+        return downcast<RenderBox>(nextSibling());
+
+    ASSERT(!nextSibling());
+    return nullptr;
 }
 
 inline void RenderBox::setInlineBoxWrapper(InlineElementBox* boxWrapper)
index 76736fcd156ffd7b811c4fb578ac1fce07bb621c..c738a9be468b6d765e13ea02f8624e4a09202fef 100644 (file)
@@ -272,9 +272,9 @@ void RenderListItem::positionListMarker()
     LayoutUnit markerOldLogicalLeft = m_marker->logicalLeft();
     LayoutUnit blockOffset = 0;
     LayoutUnit lineOffset = 0;
-    for (RenderBox* o = m_marker->parentBox(); o != this; o = o->parentBox()) {
-        blockOffset += o->logicalTop();
-        lineOffset += o->logicalLeft();
+    for (auto* ancestor = m_marker->parentBox(); ancestor && ancestor != this; ancestor = ancestor->parentBox()) {
+        blockOffset += ancestor->logicalTop();
+        lineOffset += ancestor->logicalLeft();
     }
 
     bool adjustOverflow = false;
index dd187772822ca2217ac62cda49933229b2fd7a05..904f9948b11a067df690a9163e764de28b045df4 100644 (file)
@@ -1382,8 +1382,8 @@ void RenderListMarker::layout()
     ASSERT(needsLayout());
 
     LayoutUnit blockOffset;
-    for (auto* box = parentBox(); box && box != &m_listItem; box = box->parentBox())
-        blockOffset += box->logicalTop();
+    for (auto* ancestor = parentBox(); ancestor && ancestor != &m_listItem; ancestor = ancestor->parentBox())
+        blockOffset += ancestor->logicalTop();
     if (style().isLeftToRightDirection())
         m_lineOffsetForListItem = m_listItem.logicalLeftOffsetForLine(blockOffset, DoNotIndentText, LayoutUnit());
     else
index ceb85f76c16f7e27ca42d976d0a918af6a023ae5..41c1c08c2b5b7937b07baac6173f3ad14d45c641 100644 (file)
@@ -316,7 +316,7 @@ void RenderMultiColumnSet::updateLogicalWidth()
     
     // FIXME: When we add fragments support, we'll start it off at the width of the multi-column
     // block in that particular fragment.
-    setLogicalWidth(parentBox()->contentLogicalWidth());
+    setLogicalWidth(multiColumnBlockFlow()->contentLogicalWidth());
 }
 
 bool RenderMultiColumnSet::requiresBalancing() const