Closed Bug 1434846 Opened 7 years ago Closed 7 years ago

Incorrect hit testing on page with floated div

Categories

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

60 Branch
Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: noszalyaron4, Assigned: botond)

References

()

Details

(Keywords: regression)

Attachments

(6 files)

Attached video 2018-02-01 09-47-51.flv (deleted) —
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20180131220303 Steps to reproduce: Using latest nightly with gfx.webrender.all pref. My gpu is intel igpu. Actual results: Upon visiting https://www.codechef.com/viewsolution/7402256 if I drag the source code viewer's scrollbar then the whole site moves. This is similar to bug 1416847 but in this case it doesn't jump to to end, I could freely navigate on the site if I wanted to as you can see in the attachment.
Assignee: nobody → bugmail
Stealing with permission.
Assignee: bugmail → botond
Scrollbar dragging is not the only scrolling method that's affected: wheeling over the subframe area also scrolls the page.
Attached file Reduced testcase (deleted) —
Attached is a reduced testcase. The issue seems to be related to use of the "float" property.
Attachment #8949885 - Attachment mime type: text/plain → text/html
Summary: Scrollbar in the page doesn't work with webrender → Incorrect hit testing on page with floated div
Attached file Display list dump with WebRender (deleted) —
Attached file Display list dump without WebRender (deleted) —
Attached are display list dumps with and without WebRender. I think the problem stems from the fact that with WebRender, there is a CompositorHitTestInfo display item for the floated div that's on top of the display items for its children (such as the scrollbar thumb), and it's eating the events directed at the children. There is no corresponding LayerEventRegions display item in the non-WebRender dump. I investigated why this difference arises, and the proximate answer seems to be that for CompositorHitTestInfo items, we create a new one for each frame, whereas for LayerEventRegions items we sometimes just add the frame to an existing item. I don't yet understand the underlying logic by which these two approaches are expected to yield the same result.
Actually, it looks like bug 1434243 has made a recent change in this area which resolves the problem. I'm going to leave this bug open because I'd like to write a mochitest based on the reduced testcase, to make sure we don't regress this scenario.
Depends on: 1434243
Comment on attachment 8950386 [details] Bug 1434846 - Factor out some utility functions for testing APZ scrollbar hit testing. https://reviewboard.mozilla.org/r/219604/#review225578 ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:417 (Diff revision 2) > +// The computed information is an object with three fields: > +// utils: the nsIDOMWindowUtils instance for this window > +// isWebRender: true if WebRender is enabled > +// isWindow: true if the platform is Windows > +function getHitTestConfig() { > + if (!("hitTestConfig" in window)) { nit: 2-space indent ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:461 (Diff revision 2) > +// If directions.horizontal is true, the horizontal scrollbar will be tested. > +// Both may be true in a single call (in which case two tests are performed). > +// expectedScrollId: The scroll id that is expected to be hit. > +// trackLocation: One of ScrollbarTrackLocation.{START, END}. > +// Determines which end of the scrollbar track is targeted. > +// expectThumb: Whether the scrollbar thumb is expected to be present nit: extra space between "to be" ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:471 (Diff revision 2) > +// There is no return value. > +// Tests that use this function must set the pref > +// "layout.scrollbars.always-layerize-track". > +function hitTestScrollbar(params) { > + var config = getHitTestConfig(); > + nit: trailing whitespace here and throughout the patch ::: gfx/layers/apz/test/mochitest/apz_test_utils.js:474 (Diff revision 2) > +function hitTestScrollbar(params) { > + var config = getHitTestConfig(); > + > + var elem = params.element; > + > + var verticalScrollbarWidth = elem.getBoundingClientRect().width - elem.clientWidth; Might be worth stashing getBoundingClientRect() in a local var since it's used often
Attachment #8950386 - Flags: review?(bugmail) → review+
Comment on attachment 8950387 [details] Bug 1434846 - APZ mochitest for hit test on scrollbar thumb of scrollframe inside float. https://reviewboard.mozilla.org/r/219606/#review225592 ::: gfx/layers/apz/test/mochitest/helper_hittest_subframe_float.html:18 (Diff revision 2) > + #subframe { > + overflow: scroll; > + height: 300px; > + } > + #subframe-content { > + width: 300px; nit: trailing ws ::: gfx/layers/apz/test/mochitest/helper_hittest_subframe_float.html:43 (Diff revision 2) > + var utils = getHitTestConfig().utils; > + > + hitTestScrollbar({ > + element: document.getElementById('subframe'), > + directions: { vertical: true }, > + expectedScrollId: utils.getViewId(document.scrollingElement), Is this right? Wouldn't the expected scroll id be that of the subframe rather than the document.scrollingElement?
Attachment #8950387 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14) > ::: gfx/layers/apz/test/mochitest/helper_hittest_subframe_float.html:43 > (Diff revision 2) > > + var utils = getHitTestConfig().utils; > > + > > + hitTestScrollbar({ > > + element: document.getElementById('subframe'), > > + directions: { vertical: true }, > > + expectedScrollId: utils.getViewId(document.scrollingElement), > > Is this right? Wouldn't the expected scroll id be that of the subframe > rather than the document.scrollingElement? I believe it's right: since we're doing a LayerState.INACTIVE test, the subframe hasn't been layerized yet and thus doesn't have its own view id. This also matches the behaviour of the LayerState.INACTIVE test in helper_hittest_basic.html.
(In reply to Botond Ballo [:botond] from comment #15) > I believe it's right: since we're doing a LayerState.INACTIVE test, the > subframe hasn't been layerized yet and thus doesn't have its own view id. Ah, yes, makes sense. Thanks!
All other review comments are addressed in the updated patches. The Try push [1] shows that the test is failing intermittently. The failure is similar to the one in bug 1436287, so probably the same issue, though it's hard to be sure. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7943e2602a430ab4d9e9acaccc7931174e074461&selectedJob=161829237
(In reply to Botond Ballo [:botond] from comment #21) > The Try push [1] shows that the test is failing intermittently. The failure > is similar to the one in bug 1436287, so probably the same issue, though > it's hard to be sure. I'm quite convinced that it's the same issue, for two reasons. First, the intermittent only happens in opt builds, just like the failures in bug 1436287. Second, even though the first failure message in bug 1436287 is "got 1, expected 3" (which is not for a scrollbar hit-test), several runs have a subsequent "got 1, expected 451" failure message (which *is* for a scrollbar hit test), exactly the same as the one in this bug. Therefore, I'm not going to hold up landing this on fixing the intermittent failure, since it's a pre-existing issue.
Btw. I'm not able to reproduce either intermittent failure in a local opt build with: ./mach mochitest --setpref gfx.webrender.all=true --repeat=100 --run-until-failure --keep-open=false gfx/layers/apz/test/mochitest/test_group_hittest.html
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e1283be8744 Factor out some utility functions for testing APZ scrollbar hit testing. r=kats https://hg.mozilla.org/integration/autoland/rev/f7c315627ca4 APZ mochitest for hit test on scrollbar thumb of scrollframe inside float. r=kats
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: qe-verify+
I have reproduced this issue using Firefox 60.0a1 (2018.02.01) on Win 10 x64, with gfx.webrender.all pref set to true, using intel gpu. I can confirm this issue is fixed, I verified using Firefox 60.0b7 on Win 10 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.2.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: