Closed Bug 1276107 Opened 4 years ago Closed 4 years ago

First APZ scroll to subframe scrolls root frame instead

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 + verified
firefox50 --- verified

People

(Reporter: heftig, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(14 files)

211.42 KB, text/plain
Details
876 bytes, text/html
Details
55.63 KB, text/plain
Details
27.70 KB, text/plain
Details
36.54 KB, text/plain
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
909 bytes, text/html
Details
55.90 KB, text/plain
Details
36.71 KB, text/plain
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
http://lab.hakim.se/scroll-effects/
Ignore the fancy scroll effects; it also bugs with JavaScript disabled.

1. Ensure root frame is scrollable downwards.
2. Move the mouse over a subframe. (Its APZ minimap doesn't yet appear.)
3. Wheel-scroll down.

Root frame gets scrolled. (The subframe's APZ minimap now shows.)

4. Move mouse a bit.
5. Attempt to scroll the same subframe again.

The subframe now gets scrolled as expected.


If the initial attempt cannot scroll the root frame (i.e. it is at the end for that direction) the subframe gets scrolled, instantly, as expected.


Reproduced with:

FDE 48.0a2 (2016-05-23)
Nightly 49.0a1 (2016-05-26) b0096c5c727749ad3e79cbdf20d2e96bd179c213
Blocks: apz-desktop
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [gfx-noted]
Confirmed. I'll look into it.
Assignee: nobody → botond
Attached file Layer dump
There are a lot of layers on this page (see attached layer dump). It looks like every visible entry of every list gets one.

This isn't necessarily a problem in and of itself; just pointing it out, as it may have performance implications for the page.
It looks like the dispatch-to-content regions aren't being propagated to the subframes' layers, instead accumulating on the layer representing the root scroll frame's content, which is below the subframe layers. The latter layer is (correctly) never hit during hit testing, so we don't trigger the layerization mechanism at all.
I notice the problem doesn't happen for all effects, only some. For example, it doesn't happen for "Papercur" or "Fan".
Attached file Reduced testcase
Here's a reduced testcase that demonstrates the problem.

The key ingredients seem to be the perspective on the list, and the "transform: translateZ(0px)" on the list elements.
Attachment #8758048 - Attachment mime type: text/plain → text/html
By comparison, here's the display list dump for the reduced testcase with the "transform: translateZ(0px)" removed (which makes the bug go away).

The important difference is that here, the LayerEventRegions display item is on top of the display items representing the subframe's scrolled content. In the problematic display list, the LayerEventRegions display item is on the bottom.
This is a display list dump for a variant of the page where I removed the perspective, but left the transform in place. This works correctly as well. 

The reason this is interesting is that this display list still has the Transform display items, suggesting that it's the Perspective display items that are messing things up.
Attachment #8758063 - Flags: review?(matt.woodrow)
(Sorry, didn't mean to post that for review yet. Buggy MozReview.)

This patch fixes the reduced testcase, but not the original page, suggesting that there's more to the problem.
Attached file Reduced testcase #2
Here's a reduced testcase for a problem that still remains. The only difference here is that the scrolled content now additionally has "z-index:2".
Attachment #8758065 - Attachment mime type: text/plain → text/html
The remaining problem here seems to be that we are sorting display items by z index, and the items with z-index 2 end up on top of the event-regions item. Fix coming up.
Review commit: https://reviewboard.mozilla.org/r/56444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56444/
Attachment #8758063 - Attachment description: MozReview Request: [WIP] Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow → MozReview Request: Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow
Attachment #8758072 - Flags: review?(matt.woodrow)
Attachment #8758063 - Flags: review?(matt.woodrow)
Comment on attachment 8758063 [details]
MozReview Request: Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56438/diff/1-2/
These two fixes together make the original page behave properly.

I'll follow this up with two APZ mochitests that test exercise the dispatch-to-content mechanism for the respective page structures.
Attachment #8758063 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8758063 [details]
MozReview Request: Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow

https://reviewboard.mozilla.org/r/56438/#review53010
Comment on attachment 8758072 [details]
MozReview Request: Bug 1276107 - Ensure sorting display items by z-order doesn't cause event-regions items to end up below items they are supposed to cover. r=mattwoodrow

https://reviewboard.mozilla.org/r/56444/#review53012
Attachment #8758072 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8758063 [details]
MozReview Request: Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56438/diff/2-3/
Comment on attachment 8758072 [details]
MozReview Request: Bug 1276107 - Ensure sorting display items by z-order doesn't cause event-regions items to end up below items they are supposed to cover. r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56444/diff/1-2/
Tests, as promised.

Kats, a big kudos for the recent APZ mochitest refactorings which made these new ones almost a pleasure to write!
Attachment #8758971 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8758971 [details]
Bug 1276107 - Move scrollWheelOver() into apz_test_native_event_utils.js, renaming it to moveMouseAndScrollWheelOver().

https://reviewboard.mozilla.org/r/57086/#review54034

So one issue with scrollWheelOver is that if the mouse cursor is already at the target position, then the mouse-move event doesn't get dispatched on Windows [1]. I'm all in favour of making scrollWheelOver reusable but I think we should somehow robustify it against this problem, so that callers don't run into it. One idea I had was to record the last mouse position that we synthesize, and if it's the same as the destination, we just skip the move. However we'd need to add hooks to any synthesize function that moves the mouse. I wasn't terribly happy with that solution, but also couldn't come up with anything better, which is why I copied that function into a bunch of tests rather than putting it in the helper.

That being said, copying it around isn't helpful either, because the footgun still exists, so I'm in favour of putting it in the native_event_utils file, but please make sure to document the footgun. Also rename it to "moveMouseAndScrollWheelOver", because there are other "scrollWheelOver" functions in other tests and I'd like to avoid the name collision. Eventually we can refactor and unify all the different variants of this function.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/test/mochitest/helper_scroll_on_position_fixed.html?rev=1ac523b371f2&force=1#47
Comment on attachment 8758972 [details]
Bug 1276107 - Add a test for scrolling an inactive subframe with perspective.

https://reviewboard.mozilla.org/r/57088/#review54036
Attachment #8758972 - Flags: review?(bugmail.mozilla) → review+
Attachment #8758973 - Flags: review?(bugmail.mozilla) → review+
Thanks! Addressed review comments and landed.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79968b5fd84
Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df1bde7eafd
Ensure sorting display items by z-order doesn't cause event-regions items to end up below items they are supposed to cover. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/a09ad974b522
Move scrollWheelOver() into apz_test_native_event_utils.js, renaming it to moveMouseAndScrollWheelOver(). r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6211a8cc26
Add a test for scrolling an inactive subframe with perspective. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e57c225fd0
Add a test for scrolling an inactive subframe with z-index. r=kats
I'm guessing the windows failures are from the aforementioned footgun; the two new tests use the same coordinates so when the second test is run the mouse is already at the proper coordinates, left over from the first test. :/
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> I'm guessing the windows failures are from the aforementioned footgun; the
> two new tests use the same coordinates so when the second test is run the
> mouse is already at the proper coordinates, left over from the first test. :/

Good catch! I guess we should implement some footgun avoidance after all.
Flags: needinfo?(botond)
Here's an attempt to avoid the footgun. Let's see if it's working:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9712f43677ce
Another, perhaps better, option to avoid the footgun is to modify the wheel-synthesization functions in the widget code to move the mouse to provided coordinates before synthesizing the wheel. I'm not 100% sure if that would work though.
Second one's looking much better. Will clean up and post for review.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> Another, perhaps better, option to avoid the footgun is to modify the
> wheel-synthesization functions in the widget code to move the mouse to
> provided coordinates before synthesizing the wheel. I'm not 100% sure if
> that would work though.

Discussed this with Kats; this will be explored in a follow-up.
Review commit: https://reviewboard.mozilla.org/r/57706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57706/
Attachment #8758971 - Attachment description: MozReview Request: Bug 1276107 - Move scrollWheelOver() into apz_test_native_event_utils.js. r=kats → Bug 1276107 - Move scrollWheelOver() into apz_test_native_event_utils.js, renaming it to moveMouseAndScrollWheelOver().
Attachment #8758972 - Attachment description: MozReview Request: Bug 1276107 - Add a test for scrolling an inactive subframe with perspective. r=kats → Bug 1276107 - Add a test for scrolling an inactive subframe with perspective.
Attachment #8758973 - Attachment description: MozReview Request: Bug 1276107 - Add a test for scrolling an inactive subframe with z-index. r=kats → Bug 1276107 - Add a test for scrolling an inactive subframe with z-index.
Attachment #8759876 - Flags: review?(bugmail.mozilla)
Comment on attachment 8758971 [details]
Bug 1276107 - Move scrollWheelOver() into apz_test_native_event_utils.js, renaming it to moveMouseAndScrollWheelOver().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57086/diff/1-2/
Comment on attachment 8758972 [details]
Bug 1276107 - Add a test for scrolling an inactive subframe with perspective.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57088/diff/1-2/
Comment on attachment 8758973 [details]
Bug 1276107 - Add a test for scrolling an inactive subframe with z-index.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57090/diff/1-2/
Comment on attachment 8759876 [details]
Bug 1276107 - Avoid the footgun where, on Windows, when synthesizing a mouse move event, if the mouse is already at the target location the event is never dispatched.

https://reviewboard.mozilla.org/r/57706/#review54762

Looks ok. I'll try to do the follow-up change today which should clean this up.
Attachment #8759876 - Flags: review?(bugmail.mozilla) → review+
Rebased and landed.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7658c369fd33
Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f03149a7565
Ensure sorting display items by z-order doesn't cause event-regions items to end up below items they are supposed to cover. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8361d5c43d2
Move scrollWheelOver() into apz_test_native_event_utils.js, renaming it to moveMouseAndScrollWheelOver(). r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd06b8e0f6dc
Add a test for scrolling an inactive subframe with perspective. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15df83c29a6
Add a test for scrolling an inactive subframe with z-index. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd104ba0e8d
Avoid the footgun where, on Windows, when synthesizing a mouse move event, if the mouse is already at the target location the event is never dispatched. r=kats
Hmm. The test was passing on Try... [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=42d91e67f1d8
(In reply to Botond Ballo [:botond] from comment #48)
> Hmm. The test was passing on Try... [1].
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=42d91e67f1d8

Oh, it was a rebase error. My test code contained a new call to the function coordinatesRelativeToWindow(), which Kats meanwhile renamed to coordinatesRelativeToScreen().
Flags: needinfo?(botond)
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b205fa6baef5
Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c033dd6fbb
Ensure sorting display items by z-order doesn't cause event-regions items to end up below items they are supposed to cover. r=mattwoodrow
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e2434714a9
Move scrollWheelOver() into apz_test_native_event_utils.js, renaming it to moveMouseAndScrollWheelOver(). r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0020ce18631
Add a test for scrolling an inactive subframe with perspective. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2c6f9e9202
Add a test for scrolling an inactive subframe with z-index. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c896f70f97b
Avoid the footgun where, on Windows, when synthesizing a mouse move event, if the mouse is already at the target location the event is never dispatched. r=kats
All right, this time I pushed the fixes and the tests separately. That way, if the tests get backed out again for some reason, at least the fixes (hopefully) will not be backed out.
Comment on attachment 8758063 [details]
MozReview Request: Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow

Note: this uplift request applies to all six patches (2 fixes + 2 tests + 2 test refactoring patches).

Approval Request Comment
[Feature/regressing bug #]:
  APZ + event regions (bug 1090398 and related work)

[User impact if declined]:
  On pages that use certain CSS features such as perspective
  or z-index, an input event may scroll the wrong scrollable
  element.

[Describe test coverage new/current, TreeHerder]:
  The patches add two new mochitests that cover the affected
  scenario.

[Risks and why]:
  Moderate; the patch touches Layout code which is somewhat
  tricky

[String/UUID change made/needed]:
  None

I'm inclined not to request uplift to 48, but I'm willing to reconsider if someone makes a case.
Attachment #8758063 - Flags: approval-mozilla-aurora?
Comment on attachment 8758063 [details]
MozReview Request: Bug 1276107 - Ensure that the event-regions display item for a scrollable subframe ends up on top of perspective child items. r=mattwoodrow

Let's try this in aurora, maybe risky but good to fix subframe scrolling.
Attachment #8758063 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This would be good to verify in aurora once all the patches land.
Flags: qe-verify+
Tracking for 49, marking wontfix for 48, I think this is too risky for beta.
Verified as fixed using Firefox 49 beta 6 and latest Aurora 50.0a2 2016-08-24 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5.

If: - the scroll is initiated from the root frame, it will continue to scroll over the subframes too
- the scrollbar is situated on top or bottom of the sub frame, scrolling up or down needs an extra mouse wheel for the root frame to start scrolling

For those cases, the behavior is the same on latest builds and on 47.0.1 (with and without APZ) so this is the expected behavior.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.