Bug 185771 - [css-flexbox] WebKit mistakenly lets pointer events (click/hover/etc) pass through flex items, if they have negative margin
Summary: [css-flexbox] WebKit mistakenly lets pointer events (click/hover/etc) pass th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: FromImplementor, InRadar
Depends on:
Blocks:
 
Reported: 2018-05-18 10:25 PDT by Daniel Holbert
Modified: 2020-06-29 05:15 PDT (History)
15 users (show)

See Also:


Attachments
Patch (13.11 KB, patch)
2020-06-09 10:05 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (13.10 KB, patch)
2020-06-10 01:53 PDT, Sergio Villar Senin
jfernandez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Holbert 2018-05-18 10:25:01 PDT
What steps will reproduce the problem?
(1) Visit https://jsfiddle.net/1tf66qrn/
(2) Hover & click the orange area

What is the expected result?
Hovers/clicks on the orange area **should not** trigger the covered-up link or cause the cursor to change.

What happens instead?
Clicks do trigger the link, and hovering does trigger the "link" mouse cursor.

This only seems to happen with flexbox, as far as I've seen so far. If I rewrite the testcase to use side-by-side inline-blocks or floats (instead of flex items), then I get the correct/expected result.  Also, if I add a nondefault opacity or position:relative to the negative-margin div, then the issue goes away (it starts blocking click events like it should).

Edge 17 and Firefox 62 Nightly give "expected results".
Safari 11.1 and Chrome 68 dev edition give the other "what happens instead" result.



I filed a Chrome/Blink bug for this as well, which is https://bugs.chromium.org/p/chromium/issues/detail?id=844505
Comment 1 Daniel Holbert 2018-05-18 11:29:36 PDT
For demonstration/comparison purposes, here are two tweaks I can make to the testcase, which make it no longer reproduce the problem:

 (1) add a nondefault opacity to the child with a negative margin:
https://jsfiddle.net/1tf66qrn/1/
 (position:relative has the same effect, too)

 (2) Make the children inline-blocks rather than flex items:
https://jsfiddle.net/1tf66qrn/2/

With either of those tweaks, Safari (and Chrome) will correctly make the orange div prevent mouse events from reaching the link that it's covering up.
Comment 2 Radar WebKit Bug Importer 2018-05-21 11:14:35 PDT
<rdar://problem/40422555>
Comment 3 Carlos Alberto Lopez Perez 2020-04-06 19:17:24 PDT
The Chrome fix from  https://crbug.com/844505 added two WPT tests:

https://wpt.live/css/css-flexbox/hittest-overlapping-margin.html
https://wpt.live/css/css-flexbox/hittest-overlapping-relative.html

The first one fails on WebKit
Comment 4 Carlos Alberto Lopez Perez 2020-04-06 19:19:24 PDT
Also https://wpt.live/css/css-flexbox/hittest-overlapping-order.html which also fails
Comment 5 Sergio Villar Senin 2020-06-09 10:05:09 PDT
Created attachment 401448 [details]
Patch
Comment 6 Manuel Rego Casasnovas 2020-06-09 13:58:32 PDT
Comment on attachment 401448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401448&action=review

> Source/WebCore/ChangeLog:10
> +        This is because painting of flexbox children is done in order modified document order instead of raw document order.

Don't we have the very same issue in Grid Layout?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:258
> +bool RenderFlexibleBox::hitTestChildren(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& adjustedLocation, HitTestAction hitTestAction)

In theory this method was going to be similar to the one but it has a bunch of differences, could you elaborate on them?

Also what happens with positioned or floated children, do they work as expected regarding hit testing or do they might cause problems too?
Comment 7 Javier Fernandez 2020-06-09 14:15:15 PDT
Comment on attachment 401448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401448&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:264
> +    if (hasOverflowClip())

Did you consider using ternary operator to save an assignment ?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:909
> +        m_reversedOrderIteratorForHitTesting.append(child);

Couldn't we insert the new items at the head of the Vector and avoid the reverse() call later ?
Comment 8 Sergio Villar Senin 2020-06-10 01:11:53 PDT
Comment on attachment 401448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401448&action=review

>> Source/WebCore/ChangeLog:10
>> +        This is because painting of flexbox children is done in order modified document order instead of raw document order.
> 
> Don't we have the very same issue in Grid Layout?

Very likely yes

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:258
>> +bool RenderFlexibleBox::hitTestChildren(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& adjustedLocation, HitTestAction hitTestAction)
> 
> In theory this method was going to be similar to the one but it has a bunch of differences, could you elaborate on them?
> 
> Also what happens with positioned or floated children, do they work as expected regarding hit testing or do they might cause problems too?

Hmm I should have expressed myself badly. The RenderBlock::hitTestChildren() is the one that is equal to the original implementation, I just moved it around. The one in RenderFlexibleBox is just iterating over the flex items and performing a hit test.

Regarding float items, the spec (https://www.w3.org/TR/css-flexbox-1/#painting) does only talk about flex items so I guess there is nothing that should be done specifically for them.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:264
>> +    if (hasOverflowClip())
> 
> Did you consider using ternary operator to save an assignment ?

OK, I'll change it.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:909
>> +        m_reversedOrderIteratorForHitTesting.append(child);
> 
> Couldn't we insert the new items at the head of the Vector and avoid the reverse() call later ?

Vector only has append() not prepend(). It's true that we still have insert(position,data,size) but it looked a bit convoluted to me, and AFAIK it's hardly ever used in the code base.

I think it's because append() + reverse() is in general more efficient than prepend() because the latter implies moving around all the data on every insertion.
Comment 9 Sergio Villar Senin 2020-06-10 01:53:11 PDT
Created attachment 401520 [details]
Patch

Applied suggested changes
Comment 10 Javier Fernandez 2020-06-15 01:34:00 PDT
Comment on attachment 401448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401448&action=review

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:909
>>> +        m_reversedOrderIteratorForHitTesting.append(child);
>> 
>> Couldn't we insert the new items at the head of the Vector and avoid the reverse() call later ?
> 
> Vector only has append() not prepend(). It's true that we still have insert(position,data,size) but it looked a bit convoluted to me, and AFAIK it's hardly ever used in the code base.
> 
> I think it's because append() + reverse() is in general more efficient than prepend() because the latter implies moving around all the data on every insertion.

Sure, I was thinking on using a list; but it's rather an usual pattern in the Rendering code and probably not worth the effort in this case.

Sorry if I'm not understanding completely the purpose of this new reverse iterator, but since you are just adding the items in the same order than in the current m_orderIterator, and then just reversing the elements' position, I wonder if this reverse operation can be avoided somehow. Can't we just use a revere iterator when accessing this new vector ?
Comment 11 Sergio Villar Senin 2020-06-29 05:15:14 PDT
Committed r263659: <https://trac.webkit.org/changeset/263659>