Changeset 263659 in webkit

Timestamp:

Jun 29, 2020 5:15:06 AM (4 years ago)

Author:

svillar@igalia.com

Message:

[css-flexbox] WebKit mistakenly lets pointer events (click/hover/etc) pass through flex items, if they have negative margin
​https://bugs.webkit.org/show_bug.cgi?id=185771

Reviewed by Javier Fernandez.

LayoutTests/imported/w3c:

• web-platform-tests/css/css-flexbox/hittest-overlapping-margin-expected.txt: Replaced FAIL by PASS expectation.
• web-platform-tests/css/css-flexbox/hittest-overlapping-order-expected.txt: Ditto.

Source/WebCore:

When multiple child elements of a flexbox overlap (for example, due to negative margins), the element drawn in the
foreground may not actually capture the hit if the element underneath it is hit-tested despite being occluded.
This is because painting of flexbox children is done in order modified document order instead of raw document order.

In order to achieve this we should inspect flex items in reverse order modified document order. As the OrderIterator
cannot go backwards, we cache the reverse order of items when doing the layout in order to have fast hit testing in
flexbox containers.

As this behaviour is different to the one implemented in RenderBlock a new virtual method to perform hit testing of children
was extracted from RenderBlock:nodeAtPoint() to a new method called RenderBlock::hitTestChildren. The RenderBlock
implementation is identical to the current one but flexbox containers overwrite it.

Two WPT flexbox hittests are passing now thanks to this patch.

• rendering/RenderBlock.cpp:

(WebCore::RenderBlock::hitTestChildren): Implementation of the new virtual method extracted from nodeAtPoint.
(WebCore::RenderBlock::nodeAtPoint): Moved code to hit test children to hitTestChildren()

• rendering/RenderBlock.h: Added hitTestChildren new virtual method.
• rendering/RenderFlexibleBox.cpp:

(WebCore::RenderFlexibleBox::hitTestChildren): Implemented.
(WebCore::RenderFlexibleBox::layoutFlexItems): Cache the reverse of the order iterator to be used by hit testing.

• rendering/RenderFlexibleBox.h: Added hitTestChildren.

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/hittest-overlapping-margin-expected.txt (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/hittest-overlapping-order-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderBlock.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderBlock.h (modified) (1 diff)
• Source/WebCore/rendering/RenderFlexibleBox.cpp (modified) (4 diffs)
• Source/WebCore/rendering/RenderFlexibleBox.h (modified) (2 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2020-06-09  Sergio Villar Senin  <svillar@igalia.com>
+
+        [css-flexbox] WebKit mistakenly lets pointer events (click/hover/etc) pass through flex items, if they have negative margin
+        https://bugs.webkit.org/show_bug.cgi?id=185771
+
+        Reviewed by Javier Fernandez.
+
+        * web-platform-tests/css/css-flexbox/hittest-overlapping-margin-expected.txt: Replaced FAIL by PASS expectation.
+        * web-platform-tests/css/css-flexbox/hittest-overlapping-order-expected.txt: Ditto.
+
 2020-06-29  Rob Buis  <rbuis@igalia.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/hittest-overlapping-margin-expected.txt

@@ -1 +1 @@
 foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo
 
-FAIL Flexboxes should perform hit testing in reverse paint order for overlapping elements: negative margin case (crbug.com/844505) assert_equals: expected "DIV" but got "A"
+PASS Flexboxes should perform hit testing in reverse paint order for overlapping elements: negative margin case (crbug.com/844505) 
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/hittest-overlapping-order-expected.txt

@@ -1 +1 @@
 foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo
 
-FAIL Flexboxes should perform hit testing in reverse paint order for overlapping elements: flex order case (crbug.com/844505) assert_equals: expected "DIV" but got "A"
+PASS Flexboxes should perform hit testing in reverse paint order for overlapping elements: flex order case (crbug.com/844505) 
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-06-09  Sergio Villar Senin  <svillar@igalia.com>
+
+        [css-flexbox] WebKit mistakenly lets pointer events (click/hover/etc) pass through flex items, if they have negative margin
+        https://bugs.webkit.org/show_bug.cgi?id=185771
+
+        Reviewed by Javier Fernandez.
+
+        When multiple child elements of a flexbox overlap (for example, due to negative margins), the element drawn in the
+        foreground may not actually capture the hit if the element underneath it is hit-tested despite being occluded.
+        This is because painting of flexbox children is done in order modified document order instead of raw document order.
+
+        In order to achieve this we should inspect flex items in reverse order modified document order. As the OrderIterator
+        cannot go backwards, we cache the reverse order of items when doing the layout in order to have fast hit testing in
+        flexbox containers.
+
+        As this behaviour is different to the one implemented in RenderBlock a new virtual method to perform hit testing of children
+        was extracted from RenderBlock:nodeAtPoint() to a new method called RenderBlock::hitTestChildren. The RenderBlock 
+        implementation is identical to the current one but flexbox containers overwrite it.
+
+        Two WPT flexbox hittests are passing now thanks to this patch.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::hitTestChildren): Implementation of the new virtual method extracted from nodeAtPoint.
+        (WebCore::RenderBlock::nodeAtPoint): Moved code to hit test children to hitTestChildren()
+        * rendering/RenderBlock.h: Added hitTestChildren new virtual method.
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::hitTestChildren): Implemented.
+        (WebCore::RenderFlexibleBox::layoutFlexItems): Cache the reverse of the order iterator to be used by hit testing.
+        * rendering/RenderFlexibleBox.h: Added hitTestChildren.
+
 2020-06-29  Xabier Rodriguez Calvar  <calvaris@igalia.com>
 

trunk/Source/WebCore/rendering/RenderBlock.cpp

@@ -2004 +2004 @@
 }
 
+bool RenderBlock::hitTestChildren(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& adjustedLocation, HitTestAction hitTestAction)
+{
+    // Hit test descendants first.
+    const LayoutSize localOffset = toLayoutSize(adjustedLocation);
+    const LayoutSize scrolledOffset(localOffset - toLayoutSize(scrollPosition()));
+
+    if (hitTestAction == HitTestFloat && hitTestFloats(request, result, locationInContainer, toLayoutPoint(scrolledOffset)))
+        return true;
+    if (hitTestContents(request, result, locationInContainer, toLayoutPoint(scrolledOffset), hitTestAction)) {
+        updateHitTestResult(result, flipForWritingMode(locationInContainer.point() - localOffset));
+        return true;
+    }
+    return false;
+}
+
 bool RenderBlock::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, HitTestAction hitTestAction)
 {
-    LayoutPoint adjustedLocation(accumulatedOffset + location());
-    LayoutSize localOffset = toLayoutSize(adjustedLocation);
+    const LayoutPoint adjustedLocation(accumulatedOffset + location());
+    const LayoutSize localOffset = toLayoutSize(adjustedLocation);
 
     if (!isRenderView()) {
@@ -2055 +2070 @@
     bool useClip = (hasControlClip() || useOverflowClip);
     bool checkChildren = !useClip || (hasControlClip() ? locationInContainer.intersects(controlClipRect(adjustedLocation)) : locationInContainer.intersects(overflowClipRect(adjustedLocation, nullptr, IncludeOverlayScrollbarSize)));
-    if (checkChildren) {
-        // Hit test descendants first.
-        LayoutSize scrolledOffset(localOffset - toLayoutSize(scrollPosition()));
-
-        if (hitTestAction == HitTestFloat && hitTestFloats(request, result, locationInContainer, toLayoutPoint(scrolledOffset)))
-            return true;
-        if (hitTestContents(request, result, locationInContainer, toLayoutPoint(scrolledOffset), hitTestAction)) {
-            updateHitTestResult(result, flipForWritingMode(locationInContainer.point() - localOffset));
-            return true;
-        }
-    }
+    if (checkChildren && hitTestChildren(request, result, locationInContainer, adjustedLocation, hitTestAction))
+        return true;
 
     if (!checkChildren && hitTestExcludedChildrenInBorder(request, result, locationInContainer, adjustedLocation, hitTestAction))

trunk/Source/WebCore/rendering/RenderBlock.h

@@ -453 +453 @@
     // FIXME-BLOCKFLOW: Remove virtualization when all callers have moved to RenderBlockFlow
     virtual bool hitTestFloats(const HitTestRequest&, HitTestResult&, const HitTestLocation&, const LayoutPoint&) { return false; }
+    virtual bool hitTestChildren(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& adjustedLocation, HitTestAction);
     virtual bool hitTestInlineChildren(const HitTestRequest&, HitTestResult&, const HitTestLocation&, const LayoutPoint&, HitTestAction) { return false; }
     bool hitTestExcludedChildrenInBorder(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, HitTestAction);

trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp

@@ -33 +33 @@
 
 #include "FlexibleBoxAlgorithm.h"
+#include "HitTestResult.h"
 #include "LayoutRepainter.h"
 #include "RenderChildIterator.h"
@@ -253 +254 @@
         }
     }
+}
+
+bool RenderFlexibleBox::hitTestChildren(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& adjustedLocation, HitTestAction hitTestAction)
+{
+    if (hitTestAction != HitTestForeground)
+        return false;
+
+    LayoutPoint scrolledOffset = hasOverflowClip() ? adjustedLocation - toLayoutSize(scrollPosition()) : adjustedLocation;
+
+    for (auto* child : m_reversedOrderIteratorForHitTesting) {
+        if (child->hasSelfPaintingLayer())
+            continue;
+        auto childPoint = flipForWritingModeForChild(child, scrolledOffset);
+        if (child->hitTest(request, result, locationInContainer, childPoint)) {
+            updateHitTestResult(result, flipForWritingMode(toLayoutPoint(locationInContainer.point() - adjustedLocation)));
+            return true;
+        }
+    }
+
+    return false;
 }
 
@@ -873 +894 @@
     // should work off this list of a subset.
     // TODO(cbiesinger): That second part is not yet true.
+    // Also initialize the reversed order iterator that would be eventually used for hit testing.
     Vector<FlexItem> allItems;
+    m_reversedOrderIteratorForHitTesting.clear();
     m_orderIterator.first();
     for (RenderBox* child = m_orderIterator.currentChild(); child; child = m_orderIterator.next()) {
@@ -882 +905 @@
             continue;
         }
+        m_reversedOrderIteratorForHitTesting.append(child);
         allItems.append(constructFlexItem(*child, relayoutChildren));
     }
+    m_reversedOrderIteratorForHitTesting.reverse();
     
     const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());

trunk/Source/WebCore/rendering/RenderFlexibleBox.h

@@ -58 +58 @@
 
     void styleDidChange(StyleDifference, const RenderStyle*) override;
+    bool hitTestChildren(const HitTestRequest&, HitTestResult&, const HitTestLocation&, const LayoutPoint& adjustedLocation, HitTestAction) override;
     void paintChildren(PaintInfo& forSelf, const LayoutPoint&, PaintInfo& forChild, bool usePrintRect) override;
 
@@ -209 +210 @@
     
     mutable OrderIterator m_orderIterator { *this };
+    Vector<RenderBox*> m_reversedOrderIteratorForHitTesting;
     int m_numberOfInFlowChildrenOnFirstLine { -1 };