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)
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)
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.
Comment 1•7 years ago
|
||
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ca489678fdedaff1c660dddf7ee62c966787b59a&tochange=5fc179e245d51289f1f7bcaba0c29189d3336fbf
Regressed by: Bug 1421380
Blocks: 1421380
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Unspecified → All
Updated•7 years ago
|
Assignee: nobody → bugmail
Updated•7 years ago
|
status-firefox58:
--- → unaffected
status-firefox59:
--- → unaffected
status-firefox60:
--- → affected
Updated•7 years ago
|
Blocks: stage-wr-nightly
Priority: -- → P1
Updated•7 years ago
|
Assignee | ||
Comment 3•7 years ago
|
||
Scrollbar dragging is not the only scrolling method that's affected: wheeling over the subframe area also scrolls the page.
Assignee | ||
Comment 4•7 years ago
|
||
Attached is a reduced testcase.
The issue seems to be related to use of the "float" property.
Assignee | ||
Updated•7 years ago
|
Attachment #8949885 -
Attachment mime type: text/plain → text/html
Assignee | ||
Updated•7 years ago
|
Summary: Scrollbar in the page doesn't work with webrender → Incorrect hit testing on page with floated div
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
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.
Assignee | ||
Comment 7•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
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 14•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 15•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
(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!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
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
Assignee | ||
Comment 22•7 years ago
|
||
(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.
Assignee | ||
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
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
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0e1283be8744
https://hg.mozilla.org/mozilla-central/rev/f7c315627ca4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify+
Comment 26•7 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•