Open Bug 1534549 Opened 8 months ago Updated 4 months ago

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

Categories

(Core :: Web Painting, defect, P2)

defect

Tracking

()

Tracking Status
firefox67 --- affected

People

(Reporter: mattwoodrow, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug, Regression)

Details

Attachments

(1 file)

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
You need to log in before you can comment on or make changes to this bug.