Bug 94081 – Implement DOM3 wheel event

• Component: UI Events
• Status: RESOLVED FIXED

[[e-mail]]

Mozilla and Microsoft have implemented wheel event, would be great to have it in Safari too.

[Alexey Proskuryakov]

Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=719320

What are the differences between wheel and mousewheel?

[Michał Gołębiowski-Owczarek]

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.

[Michał Gołębiowski-Owczarek]

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.

[Michał Gołębiowski-Owczarek]

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

[Chris Dumez]

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?

[Masayuki Nakano]

(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

[Chris Dumez]

corresponding Blink revision:
https://src.chromium.org/viewvc/blink?revision=156404&view=revision

[Chris Dumez]

Created attachment 209254 [details]
WIP Patch

[Chris Dumez]

Created attachment 209256 [details]
Patch

[Masayuki Nakano]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 209264 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

Comment on attachment 209264 [details]
Patch

I will make another iteration soon as we should report to real amount of pixels that is scrolled.

[Chris Dumez]

Created attachment 209337 [details]
Patch

[Chris Dumez]

Created attachment 209367 [details]
Patch

[Chris Dumez]

Created attachment 209587 [details]
Patch

Reupload without change for mac ews.

Any feedback on this?

[Chris Dumez]

Created attachment 209589 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 209669 [details]
Patch

Darin, could you please take another look? (especially the EventTarget changes).

[Chris Dumez]

Created attachment 209671 [details]
Patch

Fix typo.

[Darin Adler]

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.

[Chris Dumez]

Created attachment 209719 [details]
Patch for landing

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

Comment on attachment 209719 [details]
Patch for landing

Test failure looks like a false positive.

[WebKit Commit Bot]

Comment on attachment 209719 [details]
Patch for landing

Clearing flags on attachment: 209719

Committed r154673: <http://trac.webkit.org/changeset/154673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

This broke PDFPlugin scrolling: https://bugs.webkit.org/show_bug.cgi?id=120542

[Lucas Forschler]

Mass move bugs into the DOM component.