Bug 94081

Summary: Implement DOM3 wheel event
Product: WebKit Reporter: sonny.piers <sonny.piers>
Component: UI EventsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, a.renevier, benjamin, buildbot, cdumez, cmarcelo, commit-queue, darin, d-r, eric.carlson, esprehn+autocc, fmalita, glenn, jer.noble, kangil.han, kondapallykalyan, masayuki, m.goleb+bugzilla, pdr, rbyers, rniwa, schenney, simon.fraser, syoichi, thorton, xidorn-webkit
Priority: P2 Keywords: BlinkMergeCandidate, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-wheelevents
Bug Depends on:    
Bug Blocks: 120597    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
darin: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch for landing
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion none

Description sonny.piers@gmail.com 2012-08-15 01:23:23 PDT
Mozilla and Microsoft have implemented wheel event, would be great to have it in Safari too.
Comment 1 Alexey Proskuryakov 2012-08-15 12:56:03 PDT
Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=719320

What are the differences between wheel and mousewheel?
Comment 2 Michał Gołębiowski-Owczarek 2013-01-11 04:16:07 PST
A good reference is here:
https://developer.mozilla.org/en-US/docs/Mozilla_event_reference/wheel
At this link there's also a proposed sort-of polyfill for this event.

Since this event is standardized by W3C:
http://www.w3.org/TR/DOM-Level-3-Events/#event-type-wheel
and since it works in both Firefox and IE >= 9, WebKit should implement it, too.
Comment 3 Michał Gołębiowski-Owczarek 2013-04-02 08:29:49 PDT
Note: IE supports the 'wheel' event only via addEventListener. There's no 'onwheel' attribute. Some tests might fail because of this.

This browser will just never stop surprising me.
Comment 4 Michał Gołębiowski-Owczarek 2013-04-02 19:15:36 PDT
Any progress here? It would be really useful to finally have a unified & standardized wheel event accross all major browsers and only WebKit is still waiting for an implementation...
Comment 5 Chris Dumez 2013-08-15 05:47:21 PDT
Corresponding Blink bug:
https://code.google.com/p/chromium/issues/detail?id=273395

I am interested in implementing this for WebKit and Blink. It is standard and already supported by Firefox and IE10. Also, the API is actually quite close to the non-standard 'mousewheel' event that WebKit implements so there is not much work needed. We would (finally) have a standard and cross-browser way of handling mouse wheel events.

I am planning to keep support for the non-standard 'mousewheel' for backward compatibility.

Does anyone have any concern?
Comment 6 Masayuki Nakano 2013-08-19 19:38:17 PDT
(In reply to comment #5)
> I am planning to keep support for the non-standard 'mousewheel' for backward compatibility.

For the compatibility:

1. "mousewheel" event should be fired *after* "wheel" event (it's the best way "mousewheel" fired from XP part automatically).
2. The default action should be performed after both "wheel" and "mousewheel" are fired and both of them are not consumed (their preventDefault() are not called).
3. "mousewheel" event should *not* be fired when the preceding "wheel" event is consumed.

And also, Firefox supports diagonal scroll (both deltaX and deltaY can be non-zero at an event) support only on Mac OS X.

Additionally, Firefox supports "onwheel" attribute and property of HTML elements. WebKit and Blink returns non-null for window.WheelEvent but they are not supporting "wheel" event. So, implementing "onwheel" on them are very helpful for Web application developers (they can use "onwheel" attribute support for checking "wheel" event support). This has not been standard yet, I filed a spec bug here:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=18542

FYI: Mozilla documents a lot of information about "mousewheel" and "wheel" event in MDN. I'm glad if they help you.

"mousewheel"
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/mousewheel
"MouseWheelEvent" interface
https://developer.mozilla.org/en-US/docs/Web/API/MouseWheelEvent
"wheel"
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/wheel
"WheelEvent" interface
https://developer.mozilla.org/en-US/docs/Web/API/WheelEvent
Comment 7 Chris Dumez 2013-08-20 23:34:52 PDT
corresponding Blink revision:
https://src.chromium.org/viewvc/blink?revision=156404&view=revision
Comment 8 Chris Dumez 2013-08-21 00:46:39 PDT
Created attachment 209254 [details]
WIP Patch
Comment 9 Chris Dumez 2013-08-21 01:35:52 PDT
Created attachment 209256 [details]
Patch
Comment 10 Masayuki Nakano 2013-08-21 03:26:17 PDT
Hmm, I'm not familiar with WebKit code. However. the patch looks like using same values for both deltaX/Y and wheelDeltaX/Y.

They must be really different values. For example, deltaY and wheelDeltaY have different sign.

And does wheelDeltaX/Y indicate scroll amount in pixels on *all* platforms?
See https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/mousewheel#wheelDelta.2C_wheelDeltaX_and_wheelDeltaY_value

You can use this testcase for checking the behavior on other browsers and WebKit (and/or Blink) with your patch.
https://bug719320.bugzilla.mozilla.org/attachment.cgi?id=589763
Comment 11 Chris Dumez 2013-08-21 03:49:50 PDT
(In reply to comment #10)
> Hmm, I'm not familiar with WebKit code. However. the patch looks like using same values for both deltaX/Y and wheelDeltaX/Y.
> 
> They must be really different values. For example, deltaY and wheelDeltaY have different sign.

You are right, the sign of deltaY is incorrect in my patch. I'll fix it.
Comment 12 Chris Dumez 2013-08-21 05:12:19 PDT
Created attachment 209264 [details]
Patch
Comment 13 Chris Dumez 2013-08-21 05:13:19 PDT
(In reply to comment #12)
> Created an attachment (id=209264) [details]
> Patch

The signs for deltaX / deltaY are now correct as per the specification and IE/FF.
Comment 14 Chris Dumez 2013-08-22 00:32:07 PDT
Comment on attachment 209264 [details]
Patch

I will make another iteration soon as we should report to real amount of pixels that is scrolled.
Comment 15 Chris Dumez 2013-08-22 03:34:03 PDT
Created attachment 209337 [details]
Patch
Comment 16 Chris Dumez 2013-08-22 08:01:24 PDT
Created attachment 209367 [details]
Patch
Comment 17 Chris Dumez 2013-08-25 04:32:47 PDT
Created attachment 209587 [details]
Patch

Reupload without change for mac ews.

Any feedback on this?
Comment 18 Chris Dumez 2013-08-25 04:59:38 PDT
Created attachment 209589 [details]
Patch
Comment 19 Build Bot 2013-08-25 06:11:20 PDT
Comment on attachment 209589 [details]
Patch

Attachment 209589 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1563269

New failing tests:
editing/style/5065910.html
Comment 20 Build Bot 2013-08-25 06:11:24 PDT
Created attachment 209591 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 21 Darin Adler 2013-08-26 09:44:44 PDT
Comment on attachment 209589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209589&action=review

Is there enough test coverage here? There are so many changes!

> Source/WebCore/dom/Document.idl:272
> -    [NotEnumerable] attribute EventListener onmousewheel;
> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.

I’m don’t think we should add these comments in the IDL files. We don’t do this elsewhere in other IDL files. And I’m not sure that people who need to know this is deprecated will be reading IDL files.

We should think about finding a useful place to put a comment about deprecation, where it will help someone make good decisions. Maybe somewhere we can put more than a single word, too.

> Source/WebCore/dom/Element.idl:188
> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.

Ditto.

> Source/WebCore/dom/EventTarget.cpp:164
> +static AtomicString legacyType(const Event* event)

The return type here could and should be const AtomicString& since these are all pre-allocated AtomicString objects.

> Source/WebCore/dom/EventTarget.cpp:185
> +    AtomicString legacyTypeName = legacyType(event);

Type here should be const AtomicString&.

> Source/WebCore/dom/EventTarget.cpp:200
> -    if (!prefixedTypeName.isEmpty()) {
> +    if (legacyTypeName == eventNames().webkitTransitionEndEvent) {

This code is now confusing. Before it made sense that the prefixing code was based on whether the name was prefixed. But now this is looking for a specific event name, so it at least needs a why comment. I suggest we add a function called isPrefixedEventType or something like that just so this code is readable, and then we won't need a comment.

> Source/WebCore/dom/WheelEvent.cpp:77
> +    , m_deltaX(-rawDelta.x())
> +    , m_deltaY(-rawDelta.y())

Old code did rounding, but this new code instead truncates. Is that better? Not clear why the raw delta here is floating point but elsewhere is integer.

> Source/WebCore/dom/WheelEvent.cpp:99
> -    // Normalize to the Windows 120 multiple
>      m_wheelDelta = IntPoint(rawDeltaX * TickMultiplier, rawDeltaY * TickMultiplier);

Might have been nicer to improve this comment rather than removing it. Also seems strange that the rawDeltaX/Y arguments are integers here, given that they get multiplied by 120.

> Source/WebCore/dom/WheelEvent.idl:36
> +    // Non-standard API

Comment is a good idea. Not sure that this is clear enough, though. Something that mentions this is leftover from older pre-standard versions might be useful; not sure.

> Source/WebCore/svg/SVGElementInstance.idl:71
> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.

Same thought about Deprecated comments in the IDL files.
Comment 22 Chris Dumez 2013-08-26 10:30:13 PDT
Comment on attachment 209589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209589&action=review

>> Source/WebCore/dom/Document.idl:272
>> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.
> 
> I’m don’t think we should add these comments in the IDL files. We don’t do this elsewhere in other IDL files. And I’m not sure that people who need to know this is deprecated will be reading IDL files.
> 
> We should think about finding a useful place to put a comment about deprecation, where it will help someone make good decisions. Maybe somewhere we can put more than a single word, too.

Ok, I will remove the comments about deprecation from the IDLs.

>> Source/WebCore/dom/EventTarget.cpp:164
>> +static AtomicString legacyType(const Event* event)
> 
> The return type here could and should be const AtomicString& since these are all pre-allocated AtomicString objects.

Ok, then I think I need to use emptyAtom instead of emptyString() below.

>> Source/WebCore/dom/EventTarget.cpp:185
>> +    AtomicString legacyTypeName = legacyType(event);
> 
> Type here should be const AtomicString&.

Ok

>> Source/WebCore/dom/EventTarget.cpp:200
>> +    if (legacyTypeName == eventNames().webkitTransitionEndEvent) {
> 
> This code is now confusing. Before it made sense that the prefixing code was based on whether the name was prefixed. But now this is looking for a specific event name, so it at least needs a why comment. I suggest we add a function called isPrefixedEventType or something like that just so this code is readable, and then we won't need a comment.

Ok, I will find a way to clean this up.

>> Source/WebCore/dom/WheelEvent.cpp:77
>> +    , m_deltaY(-rawDelta.y())
> 
> Old code did rounding, but this new code instead truncates. Is that better? Not clear why the raw delta here is floating point but elsewhere is integer.

We don't need rounding since m_delta* variables are double. The WheelEvent delta* members are double which is why I am using the same type. I kept IntPoint for wheelDelta because this matches the type of the wheelDelta attributes in the IDL.

>> Source/WebCore/dom/WheelEvent.cpp:99
>>      m_wheelDelta = IntPoint(rawDeltaX * TickMultiplier, rawDeltaY * TickMultiplier);
> 
> Might have been nicer to improve this comment rather than removing it. Also seems strange that the rawDeltaX/Y arguments are integers here, given that they get multiplied by 120.

Ok about the comment.
Regarding the int type, the arguments are integers, this initWheelEvent() function is exposed to JS and arguments have been using integers so far. Since this is a legacy function (that should not be used anymore), I am not sure it is worth updating it.

>> Source/WebCore/dom/WheelEvent.idl:36
>> +    // Non-standard API
> 
> Comment is a good idea. Not sure that this is clear enough, though. Something that mentions this is leftover from older pre-standard versions might be useful; not sure.

Ok, proposing: "Legacy MouseWheelEvent API replaced by standard WheelEvent API."
Comment 23 Chris Dumez 2013-08-26 12:28:58 PDT
Created attachment 209669 [details]
Patch

Darin, could you please take another look? (especially the EventTarget changes).
Comment 24 Chris Dumez 2013-08-26 12:31:05 PDT
Created attachment 209671 [details]
Patch

Fix typo.
Comment 25 Darin Adler 2013-08-26 21:42:07 PDT
Comment on attachment 209671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209671&action=review

Looks good.

> Source/WebCore/dom/EventTarget.h:37
> +#include "FeatureObserver.h"

It’s unpleasant to have to include this so widely. There’s no reason that shouldObserveLegacyType needs to be a static member function. It could just be a file internal function, to avoid the need to add this include.

> Source/WebCore/dom/EventTarget.h:157
> +        void setupLegacyTypeObserverIfNeeded(const AtomicString& legacyTypeName, LegacyTypeUse);

Two booleans would work better than the enum. The call site is basically turning two booleans into an enum.
Comment 26 Chris Dumez 2013-08-27 00:51:33 PDT
Created attachment 209719 [details]
Patch for landing
Comment 27 Build Bot 2013-08-27 02:31:02 PDT
Comment on attachment 209719 [details]
Patch for landing

Attachment 209719 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1606006

New failing tests:
fast/workers/termination-with-port-messages.html
Comment 28 Build Bot 2013-08-27 02:31:07 PDT
Created attachment 209737 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 29 Chris Dumez 2013-08-27 02:44:30 PDT
Comment on attachment 209719 [details]
Patch for landing

Test failure looks like a false positive.
Comment 30 WebKit Commit Bot 2013-08-27 03:08:01 PDT
Comment on attachment 209719 [details]
Patch for landing

Clearing flags on attachment: 209719

Committed r154673: <http://trac.webkit.org/changeset/154673>
Comment 31 WebKit Commit Bot 2013-08-27 03:08:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Tim Horton 2013-08-30 12:36:36 PDT
This broke PDFPlugin scrolling: https://bugs.webkit.org/show_bug.cgi?id=120542
Comment 33 Lucas Forschler 2019-02-06 09:19:07 PST
Mass move bugs into the DOM component.