1534549 - Hit test info can be wrong when we have multiple AGRs and no positioned items.

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Matt Woodrow (:mattwoodrow)]

I think this is a regression from bug 1434243 part 4.

Test case: https://codepen.io/woodrow_matt/pen/XGeByM

Activate the scroll frame by scrolling once, and then we can APZ scroll with the mouse over the red overlap area.

We are currently saving and restoring the hit test info state (on nsDisplayListBuilder) as part of AutoBuildingDisplayList, so for every frame.

So it looks like we build the hit test info for the body and set that as the current hit test state.

Then we enter the scrolled frame, detect that it's a different AGR and build hit test info for the scrolled frame. This also updates the hit test state on the builder, but as soon as we finish this frame and exit then it gets restored.

Finally we build the red frame, and we're not an AGR, and the hit test state matches (since we restored any changes the scrolled frame made). We decide we don't need new hit test info, over though we're on top of the scrolled frame.

I'm still thinking through how to fix this, but I think we effectively want to track the 'previous' hit test info added (which just gets overwritten, not save/restored), and use that to see if we can use it. Need to figure out how positioned frames fit into that, and how to maintain bug 1529458 (which I think added more instances of this bug).

Comment 1

[Matt Woodrow (:mattwoodrow)]

Bug 1529458 indeed adds the equivalent issue with positioned elements - https://codepen.io/woodrow_matt/pen/zbEJoR (identical to the previous test, except the two outer divs now have position:relative).

That works fine in currently Nightly, but will be also broken once 1529458 merges.

Comment 2

[Matt Woodrow (:mattwoodrow)]

The more I think about this, the harder it gets.

Hit test info items are added to the BorderBackground() list, but once we've traversed through a block [1], the current BorderBackground() list will actually be the Content() or BlockBorderBackgrounds() list on the nearest stacking context.

Positioned descendants with z-index < 0 will be behind content and block border backgrounds [2][3], so a normal element might need a new hit-test info (and can't use the one generated by the parent) if there is negative z-index content behind it.

It seems like the only correct solution would be to track what the topmost hit test info within a given display list (probably by adding more information to nsDisplayListCollection/Set), and adding a new hit-test info if the current frame's size/hit-test flags don't match the previous hit-test data for that last.

When nsBlockFrame creates a new collection, and connects the BorderBackground list to the stacking context's BlockBorderBackgrounds list, we'd also want to connect the previous hit-test data in the same way.

Creating a new (pseudo-)stacking context could inherit the current top-most hit-test info for the outer positioned descendants list (where the stacking context is going to go after flattening) and use that to initialize the BorderBackground (the backmost-list in the flattend SC) previous hit test info.

PositionedDescendants is going to be sorted, so the previous data would probably have to be for the z-index:0 subset (or track previous for each unique value of z-index).

I think that works, but it's still going to create a lot more hit test info items that we'd like, since we'll be adding lots for the possibility that a different list will put something behind us.

CC'ing some more people who've looked at this code before, anyone got any better ideas?

[1] https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/layout/generic/nsBlockFrame.cpp#6418
[2] https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/layout/painting/nsDisplayList.cpp#9750
[3] https://www.w3.org/TR/CSS2/zindex.html#painting-order

Comment 3

[Matt Woodrow (:mattwoodrow)]

Testcase for the example from comment 2 - https://codepen.io/woodrow_matt/pen/VRrXVQ

The first <div> isn't positioned or special in any way, but needed a new hit-test info since the second div ends up underneath it. It doesn't get one currently, so you can scroll the second div through the first.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: hittest-block

This is a testcase covering what I mentioned earlier about nsBlockFrame connecting BorderBackgrounds -> BlockBorderBackgrounds.

Full explanation: https://pastebin.com/xYNX9EpX

It's possible that this particular issue can only be triggered with touch-action, since normally we can't trigger other hit test flags without being positioned (or a scrollbar etc).

It's possible that we could have different solutions to this bug depending on whether touch-action is enabled, but mobile is probably where the performance matters the most, and the added complexity of solving it twice isn't fun.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Discussed this with Matt today and the tentative plan is to undo bug 1503046 to get correctness back at the expense of perf. And then if we fix bug 1425863 that should get us some perf back in the WebRender case. Not sure what to do about the non-WebRender case.

Comment 6

[Miko Mynttinen]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)

Discussed this with Matt today and the tentative plan is to undo bug 1503046 to get correctness back at the expense of perf. And then if we fix bug 1425863 that should get us some perf back in the WebRender case. Not sure what to do about the non-WebRender case.

Bug 1503046 should not affect the correctness. Bug 1434243 added the incomplete reuse of parent frame hit test info.

Comment 7

[Botond Ballo [:botond]]

Sounds like this should be a Fission M6c because it blocks a Fission M6c (bug 1676466).

Comment 8

[Neha Kochar [:neha]]

Botond, will you be working on this for M6c (Fx85)?

Comment 9

[Miko Mynttinen]

(In reply to Neha Kochar [:neha] from comment #8)

Botond, will you be working on this for M6c (Fx85)?

I am currently working on this, I think that the fix will be the same for this and bug 1676466.

Comment 11

[Miko Mynttinen]

Attachment: Bug 1534549 - Part 1: Remove old FLB hit test info optimization r=mstange

Comment 12

[Miko Mynttinen]

Attachment: Bug 1534549 - Part 2: Allow all display items to carry hit testing information r=mstange

Depends on D102514

Comment 14

[Miko Mynttinen]

Attachment: Bug 1534549 - Part 3: Add tests r=mstange

Depends on D102515

Comment 15

[Chris Peterson [:cpeterson]]

Miko, do you have an ETA for landing your hit test patches? It looks like all your patches have r+ in Phabricator.

Comment 16

[Miko Mynttinen]

Attachment: Bug 1534549 - Part 4: Shrink nsDisplayCompositorHitTestInfo r=mstange

These changes make nsDisplayCompositorHitTestInfo inherit directly from nsDisplayItem, which should shrink it slightly. These also clean up the logic: hit testing information is now available at nsDisplayItem level as opposed to nsPaintedDisplayItem.

Depends on D103045

Comment 17

[Jim Mathies [:jimm]]

Update from Miko - Broke an FLB assertion with hit test patches while rebasing, fixed it and will land the patches if try run comes green

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b390459263
Part 1: Remove old FLB hit test info optimization r=mstange
https://hg.mozilla.org/integration/autoland/rev/dabd0c5d21cf
Part 2: Allow all display items to carry hit testing information r=mstange
https://hg.mozilla.org/integration/autoland/rev/2ac9c5dbe9f5
Part 3: Add tests r=mstange
https://hg.mozilla.org/integration/autoland/rev/a03c745620d6
Part 4: Shrink nsDisplayCompositorHitTestInfo r=mstange

Comment 19

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1534549) for causing mochitest failures in test_group_wheelevents.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=I0vEzPkFRLefRowt-fir8g.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a03c745620d6633d6ef56c806cc466b6b5e144d4

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328938668&repo=autoland&lineNumber=13849

Backout: https://hg.mozilla.org/integration/autoland/rev/8c5ccbd328c366b9c5ee390a5d40210ac8552fca

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56d9644b5a41
Part 1: Remove old FLB hit test info optimization r=mstange
https://hg.mozilla.org/integration/autoland/rev/fe81233c13cf
Part 2: Allow all display items to carry hit testing information r=mstange
https://hg.mozilla.org/integration/autoland/rev/1907d855cb60
Part 3: Add tests r=mstange
https://hg.mozilla.org/integration/autoland/rev/f72030ee5289
Part 4: Shrink nsDisplayCompositorHitTestInfo r=mstange

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/56d9644b5a41
https://hg.mozilla.org/mozilla-central/rev/fe81233c13cf
https://hg.mozilla.org/mozilla-central/rev/1907d855cb60
https://hg.mozilla.org/mozilla-central/rev/f72030ee5289