Closed Bug 1168182 Opened 9 years ago Closed 2 years ago

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

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
113 Branch
Tracking Flags:
Tracking Status
firefox41 --- wontfix
firefox113 --- fixed

People

(Reporter: mstange, Assigned: dlrobertson)

References

(Regressed 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug
(deleted), text/x-phabricator-request
Details
Bug 1168182 - Update webdriver recommended prefs. r=whimboo
(deleted), text/x-phabricator-request
Details
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.
This is e10s+apz bug, right? Since for non-apz there is the mousewheel transaction thing.
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.
Oh, but would that be web compatible?
That's what I was asking in bug 1166871 comment 5. I don't know the answer.
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.
Masayuki, any thoughts on this?
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
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).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(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.
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.
Component: Event Handling → User events and focus handling
Severity: normal → S3
Assignee: nobody → drobertson
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1796235
  • 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.
  • 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.
Attachment #9306280 - Attachment is obsolete: true
Attachment #9306052 - Attachment description: WIP: Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug! → Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug!
Attachment #9306052 - Attachment description: Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug! → Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki!,smaug!
Pushed by drobertson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2ddeecc9100b Bind wheel event targets to wheel transactions. r=masayuki,smaug

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.

[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
Flags: needinfo?(drobertson)

Updated patch to fix the issue with the test.

Flags: needinfo?(drobertson)
Attachment #9306052 - Attachment description: Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki!,smaug! → Bug 1168182 - Bind wheel event targets to wheel transactions. r=masayuki,smaug

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

Depends on D163484

Blocks: 1821733
Attachment #9321924 - Attachment description: Bug 1168182 - Update webdriver tests for wheel event groups. r=whimboo → Bug 1168182 - Update webdriver recommended prefs. r=whimboo
Pushed by drobertson@mozilla.com: 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

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

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox113: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
Regressions: 1823660
Duplicate of this bug: 1778200
Duplicate of this bug: 1746967
Duplicate of this bug: 1746304
Duplicate of this bug: 1614358
Duplicate of this bug: 1796235
Regressions: 1830194
Regressions: 1830758
Blocks: 1823700
Regressions: 1851378
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: