Bug 185771 – [css-flexbox] WebKit mistakenly lets pointer events (click/hover/etc) pass through flex items, if they have negative margin

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Daniel Holbert]

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

[Daniel Holbert]

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.

[Radar WebKit Bug Importer]

<rdar://problem/40422555>

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

Also https://wpt.live/css/css-flexbox/hittest-overlapping-order.html which also fails

[Sergio Villar Senin]

Created attachment 401448 [details]
Patch

[Manuel Rego Casasnovas]

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?

[Javier Fernandez]

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 ?

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 401520 [details]
Patch

Applied suggested changes

[Javier Fernandez]

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 ?

[Sergio Villar Senin]

Committed r263659: <https://trac.webkit.org/changeset/263659>