Jan Alexander Steffens [:heftig] Reporter
	Description • 8 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 1 • 8 years ago

Confirmed. I'll look into it.

	Botond Ballo [:botond] Assignee
	Comment 2 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 3 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 4 • 8 years ago

I notice the problem doesn't happen for all effects, only some. For example, it doesn't happen for "Papercur" or "Fan".

	Botond Ballo [:botond] Assignee
	Comment 5 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 7 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 8 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 9 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/56438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56438/

	Botond Ballo [:botond] Assignee
	Comment 10 • 8 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 11 • 8 years ago

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".

	Botond Ballo [:botond] Assignee
	Comment 14 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 15 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/56444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56444/

	Botond Ballo [:botond] Assignee
	Comment 16 • 8 years ago

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/

	Botond Ballo [:botond] Assignee
	Comment 17 • 8 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 18 • 8 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 19 • 8 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 20 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/57086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57086/

	Botond Ballo [:botond] Assignee
	Comment 21 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/57088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57088/

	Botond Ballo [:botond] Assignee
	Comment 22 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/57090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57090/

	Botond Ballo [:botond] Assignee
	Comment 23 • 8 years ago

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/

	Botond Ballo [:botond] Assignee
	Comment 24 • 8 years ago

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/

	Botond Ballo [:botond] Assignee
	Comment 25 • 8 years ago

Tests, as promised.

Kats, a big kudos for the recent APZ mochitest refactorings which made these new ones almost a pleasure to write!

	Botond Ballo [:botond] Assignee
	Comment 26 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=104ac7205c58

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 27 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 28 • 8 years ago

Comment on attachment 8758972 [details]
Bug 1276107 - Add a test for scrolling an inactive subframe with perspective.

https://reviewboard.mozilla.org/r/57088/#review54036

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 29 • 8 years ago

Comment on attachment 8758973 [details]
Bug 1276107 - Add a test for scrolling an inactive subframe with z-index.

https://reviewboard.mozilla.org/r/57090/#review54038

	Botond Ballo [:botond] Assignee
	Comment 30 • 8 years ago

Thanks! Addressed review comments and landed.

	Pulsebot
	Comment 31 • 8 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 32 • 8 years ago

Backed out for often failing test_group_wheelevents.html with e10s:

https://hg.mozilla.org/integration/mozilla-inbound/rev/75fc78ba704e0d506c066001f7b9e8335ace5673
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce3db98c4f4524a169ab4ac0e1d9153174206f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7093be1d222f3270c94482cc8d7d23ee113dd390
https://hg.mozilla.org/integration/mozilla-inbound/rev/2529d7543c234bc34693e41621edb9bff7b80d37
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a84a262f3199840e9986d19589dc4d84a08cec0

E.g. many failures in this push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0836d0b9fd52f4d82364a5facc8821f237f4f4a4
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=29345871&repo=mozilla-inbound

Note that the Linux failure is different.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 33 • 8 years ago

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. :/

	Botond Ballo [:botond] Assignee
	Comment 34 • 8 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 35 • 8 years ago

Here's an attempt to avoid the footgun. Let's see if it's working:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9712f43677ce

	Botond Ballo [:botond] Assignee
	Comment 36 • 8 years ago

Attempt #2:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94d7687b9afc

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 37 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 38 • 8 years ago

Second one's looking much better. Will clean up and post for review.

	Botond Ballo [:botond] Assignee
	Comment 39 • 8 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 40 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/57706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57706/

	Botond Ballo [:botond] Assignee
	Comment 41 • 8 years ago

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/

	Botond Ballo [:botond] Assignee
	Comment 42 • 8 years ago

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/

	Botond Ballo [:botond] Assignee
	Comment 43 • 8 years ago

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/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 44 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 45 • 8 years ago

Rebased and landed.

	Pulsebot
	Comment 46 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 47 • 8 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/94409ada0653 for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=29586016&repo=mozilla-inbound

	Botond Ballo [:botond] Assignee
	Comment 48 • 8 years ago

Hmm. The test was passing on Try... [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=42d91e67f1d8

	Botond Ballo [:botond] Assignee
	Comment 49 • 8 years ago

(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().

	Pulsebot
	Comment 50 • 8 years ago

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

	Pulsebot
	Comment 51 • 8 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 52 • 8 years ago

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.

	Carsten Book [:Tomcat]
	Comment 53 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b205fa6baef5
https://hg.mozilla.org/mozilla-central/rev/87c033dd6fbb
https://hg.mozilla.org/mozilla-central/rev/83e2434714a9
https://hg.mozilla.org/mozilla-central/rev/a0020ce18631
https://hg.mozilla.org/mozilla-central/rev/3d2c6f9e9202
https://hg.mozilla.org/mozilla-central/rev/8c896f70f97b

	Botond Ballo [:botond] Assignee
	Comment 54 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 55 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 56 • 8 years ago

This would be good to verify in aurora once all the patches land.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 57 • 8 years ago

Tracking for 49, marking wontfix for 48, I think this is too risky for beta.

	Carsten Book [:Tomcat]
	Comment 58 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/b03055864252
https://hg.mozilla.org/releases/mozilla-aurora/rev/19ce047a3579
https://hg.mozilla.org/releases/mozilla-aurora/rev/9044530a9595
https://hg.mozilla.org/releases/mozilla-aurora/rev/db30e31b4b47
https://hg.mozilla.org/releases/mozilla-aurora/rev/2393f2456c18
https://hg.mozilla.org/releases/mozilla-aurora/rev/e579b0b53a20

	Petruta Horea [:phorea], Desktop QA
	Comment 59 • 8 years ago

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.