1613199 - Scroll hit testing broken in Gecko profiler arrow panel

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Markus Stange [:mstange]]

Steps to reproduce:

1. Make sure WebRender is enabled.
2. Go to https://perfht.ml/2Opit9q
3. Click the button at the top that says "Firefox (74.0) Intel Mac OS X 10.15"
4. Scroll in the arrow panel.

Expected results:
The panel contents should scroll.

Actual results:
The elements underneath the panel are scrolled instead.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=776c7e62ea269631d808ec6020bbe9cc486398e8&tochange=473309c218b720f2d606d491e44f853e79115aac

Comment 2

[Alice0775 White]

I can reproduce on Windows10.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

That's some action-at-a-distance. My understanding from comment 0 is that bug 1526725 just made the wheel event not go through the main thread, and that unveiled a webrender hit-testing regression. But let me know if that's off.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, same on Linux.

Comment 5

[Nicolas Silva [:nical]]

Auto-ni? botond because hit-testing.

Comment 6

[Alexandra Martin]

Reproduced the issue on Windows 10 x64 and Ubuntu 18.04.3 LTS using Firefox Nightly 74.0a1 (2020-02-06).

Latest Beta and Release versions are not affected on Windows 10 and Ubuntu 18.04.3 LTS. Likewise for all Firefox versions on Mac 10.14.

Will add the according flags.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Alice0775 White from comment #1)

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=776c7e62ea269631d808ec6020bbe9cc486398e8&tochange=473309c218b720f2d606d491e44f853e79115aac

Alice, would mind testing in 74 beta to see if this issue is held to nightly or rolling out?

Comment 8

[Alice0775 White]

(In reply to Jim Mathies [:jimm] from comment #7)

(In reply to Alice0775 White from comment #1)

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=776c7e62ea269631d808ec6020bbe9cc486398e8&tochange=473309c218b720f2d606d491e44f853e79115aac

Alice, would mind testing in 74 beta to see if this issue is held to nightly or rolling out?

I can not reproduce on Firefox74.0beta2 Windows10 with WebRender.
I can still reproduce Nightly75.0a1(20200212093201) Windows10 with WebRender.

Comment 9

[Botond Ballo [:botond]]

Kats, feel free to take a look at this if you have cycles to do so.

Comment 10

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

Attachment: Reduced test case with same root problem

Here's a reduced test case of the problem. Not quite the same problem - instead of scrolling the larger scrollframe under the small one, it just doesn't scroll anything. But the underlying problem is the same here compared to the gecko profiler original page.

The problem is that the scrollframe being targeted is inside an element with a filter, and so is an "inactive" scrollframe. The display list generates a "hoisted" nsDisplayScrollInfoLayer item via this codepath. This nsDisplayScrollInfoLayer item is completely ignored from WR's point of view, because nsDisplayScrollInfoLayer doesn't have a CreateWebRenderCommands implementation and so WR doesn't ever find out about this inactive scrollframe. So when WR does the hit test, it just goes to whatever is underneath instead.

I have a patch that implements CreateWebRenderCommands on the nsDisplayScrollInfoLayer by using basically the same hit-test info that goes on the corresponding nsDisplayCompositorHitTestInfo item, and that seems to fix the problem. Still need to do a try push and write a proper test.

I also have a vague recollection that cases where we generate a blob we should be doing this which should tell WR to delegate to the main-thread hit-test, but maybe we're not hitting this codepath?

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=289964335&revision=ddffd828010ec5ed73b7015ca902e29c518286a5

Try push looks ok (with the fix). Need to clean up the new test still.

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

I also have a vague recollection that cases where we generate a blob we should be doing this which should tell WR to delegate to the main-thread hit-test, but maybe we're not hitting this codepath?

I checked and we indeed are not hitting that codepath for this scenario.

Comment 12

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

With new test: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=477118ffc9d4599520019615c364d3bd2cbf22c8

Comment 13

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

Attachment: Bug 1613199 - Fix hit-testing for scrollframes with hoisted scrollinfos. r?tnikkel,botond

In some cases (such as the case from this bug) the display list contains a
"hoisted" scrollinfo display item, which indicates the presence of a scroller
inside an inactive layer subtree (e.g. a div with certain kinds of filters).
The scrollinfo display item is "hoisted" outside the display list subtree so
that it doesn't get flattened away inside the inactive subtree. That display
item then causes the compositor hit-test regions to updated appropriately so
that APZ knows about the scrollframe inside the flattened content. This in turn
allows APZ to request main-thread scrolling for that scrollframe when input
events are directed to it.

With the WebRender codepath, the information represented by the hoisted
scrollinfo display item was being lost instead of being propagated to the
compositor. This was because the mechanism used for information propagation is
different (WebRender commands vs layers EventRegions). This patch ensures that
the scrollinfo display items also generate appropriate WebRender commands so
that the information is not lost, and WR knows about the scrollframe inside
the flattened content.

The patch includes:

• A code movement in nsGfxScrolllFrame.cpp so that necessary information can
  be provided to the nsDisplayScrollInfoLayer constructor
• Updates to nsDisplayScrollInfoLayer members to store the necessary information
• Addition of nsDisplayScrollInfoLayer::CreateWebRenderCommands which propagates
  the information to the WR display list
• A test to exercise the changes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46caa1633229
Fix hit-testing for scrollframes with hoisted scrollinfos. r=botond

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/46caa1633229