1168182 - Consider capturing wheel events to the DOM element that the current wheel transaction started over

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Markus Stange [:mstange]]

At the moment, as you scroll over a page with the mouse wheel, the mouse moves over different elements of the page, and the wheel events are sent to whatever element is under the mouse at the time.

I think it would be a good idea to attach the wheel event target to the current wheel transaction, and then send all wheel events of the current transaction to that same DOM element.

(It's not clear what we should do if the element is removed from the document during the transaction - maybe send the event to the first ancestor that still is in the document?)

Doing this would avoid a few problems with APZ. One problem we have now is the following: If you start scrolling over an element that doesn't preventDefault() any wheel events, and then during the wheel transaction the mouse suddenly comes across an element that does preventDefault(), then some wheel events will still result in APZ scrolling until APZ is notified by the content thread that it needs to stop. Locking wheel events to the element that the scroll started over would work around this problem in most cases.

I'm going into a little more detail about this in bug 1013412 comment 5.

I brought up this idea in bug 1166871 comment 4 but I'm not sure if anybody understood what I meant.

(In reply to Olli Pettay [:smaug] from comment #7)
> Parent process could indeed capture wheel events in case wheel events are
> targeted to content process.

I don't really understand this. What does "capture" mean in this case, and how would the event flow work as a result?

> We do have the transaction model for wheel events so that during the
> transaction same thing is being
> scrolled,

Right. We scroll the same thing, but the DOM events might go to different elements during the scroll.

> but I'm not sure how that works with apz.

In APZ we have the concept of an InputBlock which mirrors the behavior of ESM wheel transactions, using timeouts and mouse moves to abort the current transaction, see the code around https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.cpp#303

> In fact, why doesn't that work in parent process already?

Not sure what this is referring to.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is e10s+apz bug, right? Since for non-apz there is the mousewheel transaction thing.

Comment 2

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

No, this is for the non-apz case. Consider this example: http://people.mozilla.org/~kgupta/bug/1168182.html. Put your mouse cursor over the scrollable div and use the wheel/trackpad to scroll. Note how the events are fired at whatever element happens to be under the cursor at the time. What markus is suggesting is that the events should always be targeted to the same element, just like touch events are.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Oh, but would that be web compatible?

Comment 4

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

That's what I was asking in bug 1166871 comment 5. I don't know the answer.

Comment 5

[Markus Stange [:mstange]]

Yeah, that is the big question. Can we try it out before we enable APZ and wait for regression reports?

I've been trying to come up with ideas for possible problems but wasn't very successful.
I think web content mostly uses wheel events for three purposes:
 (1) Implementing its own page scrolling
 (2) Implementing scrolling for specific scrollboxes
 (3) Zooming (e.g. maps)

In each of these cases, each wheel event that's used for custom scrolling / zooming should be preventDefault()ed. I have, however, seen cases of (2) and (3) where this was forgotten, maybe because it was only ever tested on pages where the page itself wasn't scrollable. As a result, you'd get double scrolling over the affected areas.

From now on I'm using "new behavior" to mean what would happen if we did what I suggest in this bug.

In case (1), the new behavior wouldn't be different from the old behavior because all wheel events on the page would be preventDefault()ed, so we'd never start a wheel transaction, and there wouldn't be any capturing.

In cases (2) and (3) if you start scrolling outside the affected area, the new behavior would be different because you'd just keep scrolling over the scrollbox and the scrollbox's wheel event handler wouldn't ever see any event.

In cases (2) and (3) with preventDefault() if you start scrolling inside the affected area, the new behavior won't be different because all wheel events are preventDefault()ed, so we don't start a wheel transaction, so we don't do any capturing.

In cases (2) and (3) *without* preventDefault() if you start scrolling inside the affected area, the new behavior will be very different because you'll keep zooming / scrolling the scrollbox even after the mouse has left the affected area due to page scrolling.

Comment 6

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

Masayuki, any thoughts on this?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to reply because I'm still not sure if this is right.

In strictly speaking, this is wrong. "wheel" event doesn't a scroll event. It notifies web apps of device operation by users. So, even if an element isn't scrollable, "wheel" events should also be fired on it.

If we lock wheel event target to a scrollable element, it must provide better UX for the users if any elements handle the event as a trigger to scroll a pseudo scrollable element even though it doesn't conform to UI Events (a.k.a. D3E) spec.

However, if wheel event handler may be used different purpose, the new behavior may break the web. For example, wheel event handlers may be implemented for moving character on games, counting up or down numbers, etc.

Anyway, the behavior shouldn't depend on e10s nor APZ. So, when you do this, I'd like you to change current behavior and enable it only  on non-release builds for waiting bug reports.

Comment 8

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

Note that we wouldn't actually move the target to a scrollable element (at least, that's not what I had in mind). If your mouse is positioned inside a scrollable element, the event target would still be the element under the cursor, and it would stay that same target for the duration of the transaction. The event target may be different from the containing scrollable element, which would be the thing that scrolls.

That being said we have another solution for bug 1166871 so I'm not convinced there's a need for this change now (or at least there's no driving force to get it done).

Comment 9

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> That being said we have another solution for bug 1166871 so I'm not
> convinced there's a need for this change now (or at least there's no driving
> force to get it done).

The driving force from my side is more about bug 1013412:

(In reply to Markus Stange [:mstange] from comment #0)
> Doing this would avoid a few problems with APZ. One problem we have now is
> the following: If you start scrolling over an element that doesn't
> preventDefault() any wheel events, and then during the wheel transaction the
> mouse suddenly comes across an element that does preventDefault(), then some
> wheel events will still result in APZ scrolling until APZ is notified by the
> content thread that it needs to stop.

Implementing this bug would reduce the chances of double-processing the event.

Oh, and I just thought of a user experience reason to implement this:

Once we have overscrolling on OS X, I hope the user will feel much more in control of the scrolled page than they do now, because the finger movements will always translate into a page movement until the fingers are lifted from the touchpad. If the page now suddenly interrupts this scroll gesture, for example by calling preventDefault() during overscroll (and presumably triggering a snap back animation?), this feeling of control is thwarted.

Comment 10

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #9)
> Oh, and I just thought of a user experience reason to implement this:
> 
> Once we have overscrolling on OS X, I hope the user will feel much more in
> control of the scrolled page than they do now, because the finger movements
> will always translate into a page movement until the fingers are lifted from
> the touchpad. If the page now suddenly interrupts this scroll gesture, for
> example by calling preventDefault() during overscroll (and presumably
> triggering a snap back animation?), this feeling of control is thwarted.

Actually, never mind this part. During overscroll, the wheel event position from the perspective on the page won't change, because the layout scroll position will just be the edge of the page (without overscroll), so the event target won't change during overscroll anyway, and we don't need this bug for that.

Comment 11

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

(In reply to Markus Stange [:mstange] from comment #9)
> The driving force from my side is more about bug 1013412:
> 
> (In reply to Markus Stange [:mstange] from comment #0)
> > Doing this would avoid a few problems with APZ. One problem we have now is
> > the following: If you start scrolling over an element that doesn't
> > preventDefault() any wheel events, and then during the wheel transaction the
> > mouse suddenly comes across an element that does preventDefault(), then some
> > wheel events will still result in APZ scrolling until APZ is notified by the
> > content thread that it needs to stop.
> 
> Implementing this bug would reduce the chances of double-processing the
> event.

Ok, fair enough. For completeness - it would reduce the chances, but wouldn't drop them to zero because a listener might not do preventDefault() on some invocations and might do it on later invocations and then we'd have the same issue.

Comment 12

[Masatoshi Kimura [:emk]]

At least the default handler should capture wheel transactions. For example,
http://www.gizmodo.jp/2016/03/batmangoogle.html
Once the mouse cursor is over the embedded Google street view, suddenly the wheel scroll will stop and street view will zoom out. This is very frustrating for me.
Also, the current behavior is inconsistent. If the embedded street view is replaced with a scrollable textarea, iframe, or whatever, the surrounding element will capture wheel events.

Comment 13

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug

• Create wheel transactions for wheel events handled by APZ.
• Group wheel events with the current wheel transaction, so that all
  wheel events in a wheel transaction are fired to the same element.

Comment 14

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug!

• Create wheel transactions for wheel events handled by APZ.
• Group wheel events with the current wheel transaction, so that all
  wheel events in a wheel transaction are fired to the same element.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ddeecc9100b
Bind wheel event targets to wheel transactions. r=masayuki,smaug

Comment 16

[Iulian Moraru]

Backed out for causing mochitest failures on test_wheel_scroll.html.
The failures won't appear on your push because it is affected by some build bustages that occurred on windows.

• Push with failures

• Failure log

• Backout link

[task 2023-02-22T19:02:26.499Z] 19:02:26     INFO - TEST-START | gfx/layers/apz/test/mochitest/test_wheel_scroll.html
[task 2023-02-22T19:02:26.527Z] 19:02:26     INFO - GECKO(3564) | WaitUntilApzStable: flushed APZ repaints in parent proc, waiting for callback...
[task 2023-02-22T19:02:26.542Z] 19:02:26     INFO - GECKO(3564) | WaitUntilApzStable: APZ flush done in parent proc
[task 2023-02-22T19:02:26.542Z] 19:02:26     INFO - GECKO(3564) | WaitUntilApzStable: got apz-flush-done in child proc
[task 2023-02-22T19:02:26.543Z] 19:02:26     INFO - GECKO(3564) | WaitUntilApzStable: done promiseFocus
[task 2023-02-22T19:02:26.585Z] 19:02:26     INFO - GECKO(3564) | WaitUntilApzStable: done promiseAllPaintsDone
[task 2023-02-22T19:02:26.593Z] 19:02:26     INFO - GECKO(3564) | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
[task 2023-02-22T19:02:26.626Z] 19:02:26     INFO - GECKO(3564) | PromiseApzRepaintsFlushed: APZ flush done
[task 2023-02-22T19:02:26.629Z] 19:02:26     INFO - GECKO(3564) | WaitUntilApzStable: all done
[task 2023-02-22T19:02:26.635Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.638Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.640Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.642Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.644Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.646Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.647Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:26.649Z] 19:02:26     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
<...>
[task 2023-02-22T19:02:27.164Z] 19:02:27     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:27.166Z] 19:02:27     INFO - GECKO(3564) | JavaScript error: , line 0: TypeError: can't access dead object
[task 2023-02-22T19:02:27.167Z] 19:02:27     INFO - TEST-INFO | started process screenshot
[task 2023-02-22T19:02:27.274Z] 19:02:27     INFO - TEST-INFO | screenshot: exit 0
[task 2023-02-22T19:02:27.282Z] 19:02:27     INFO - Buffered messages logged at 19:02:27
[task 2023-02-22T19:02:27.284Z] 19:02:27     INFO - TEST-PASS | gfx/layers/apz/test/mochitest/test_wheel_scroll.html | We should have scrolled down somewhat 
[task 2023-02-22T19:02:27.284Z] 19:02:27     INFO - Buffered messages finished
[task 2023-02-22T19:02:27.284Z] 19:02:27     INFO - TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_wheel_scroll.html | We should have scrolled to the bottom of the scrollframe - got 1190, expected 1469
[task 2023-02-22T19:02:27.284Z] 19:02:27     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:487:14
[task 2023-02-22T19:02:27.284Z] 19:02:27     INFO -     test@gfx/layers/apz/test/mochitest/test_wheel_scroll.html:89:5
[task 2023-02-22T19:02:27.285Z] 19:02:27     INFO - TEST-PASS | gfx/layers/apz/test/mochitest/test_wheel_scroll.html | The rotation should not have been adjusted 
[task 2023-02-22T19:02:27.285Z] 19:02:27     INFO - GECKO(3564) | MEMORY STAT | vsize 577MB | vsizeMaxContiguous 1806MB | residentFast 84MB | heapAllocated 11MB
[task 2023-02-22T19:02:27.286Z] 19:02:27     INFO - TEST-OK | gfx/layers/apz/test/mochitest/test_wheel_scroll.html | took 683ms
[task 2023-02-22T19:02:27.287Z] 19:02:27     INFO - TEST-START | gfx/layers/apz/test/mochitest/test_wheel_transactions.html

Comment 17

[Dan Robertson (:dlrobertson)]

Updated patch to fix the issue with the test.

Comment 18

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1168182 - Update webdriver recommended prefs. r=whimboo

Update wheel webdriver tests to work consistently with the addition of
wheel event groups.

Depends on D163484

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/658ee1bd76f6
Bind wheel event targets to wheel transactions. r=masayuki,smaug
https://hg.mozilla.org/integration/autoland/rev/cb782a3ace01
Update webdriver recommended prefs. r=whimboo,webdriver-reviewers,jdescottes

Comment 20

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/658ee1bd76f6
https://hg.mozilla.org/mozilla-central/rev/cb782a3ace01