Closed Bug 1534549 Opened 5 years ago Closed 3 years ago

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

Categories

(Core :: Web Painting, defect, P2)

defect

Tracking

()

RESOLVED FIXED
87 Branch
Fission Milestone M6c
Tracking Status
firefox-esr78 --- wontfix
firefox67 --- wontfix
firefox85 --- wontfix
firefox86 --- wontfix
firefox87 --- fixed

People

(Reporter: mattwoodrow, Assigned: mikokm)

References

(Depends on 2 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(5 files)

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).

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.

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

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.

Blocks: 1527026
Priority: -- → P2
Attached patch hittest-blockSplinter Review

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.

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.

Regressed by: 1503046

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

No longer blocks: 1434243
Regressed by: 1434243
No longer regressed by: 1503046
Depends on: 1559706
Depends on: 1564914
Blocks: 1676466

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

Fission Milestone: --- → M6c

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

Flags: needinfo?(botond)

(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.

Assignee: nobody → mikokm
Status: NEW → ASSIGNED
Flags: needinfo?(botond)

Depends on D102515

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

Flags: needinfo?(mikokm)
Whiteboard: [ETA ? patches have r+ in Phabricator]

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

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

Flags: needinfo?(mikokm)
Pushed by mikokm@gmail.com:
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
Flags: needinfo?(mikokm)
Whiteboard: [ETA ? patches have r+ in Phabricator] → [2/5] patches bounced
Pushed by mikokm@gmail.com:
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
Blocks: 1544451
Depends on: 1715338
Whiteboard: [2/5] patches bounced
Regressions: 1730856
Has Regression Range: --- → yes
Regressions: 1750285
Regressions: 1747409
You need to log in before you can comment on or make changes to this bug.