Bug 94081 - Implement DOM3 wheel event
: Implement DOM3 wheel event
Status: RESOLVED FIXED
: WebKit
HTML Events
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dev.w3.org/2006/webapi/DOM-Lev...
: BlinkMergeCandidate, WebExposed
:
: 120597
  Show dependency treegraph
 
Reported: 2012-08-15 01:23 PST by
Modified: 2013-09-02 02:42 PST (History)


Attachments
WIP Patch (32.93 KB, patch)
2013-08-21 00:46 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.05 KB, patch)
2013-08-21 01:35 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.56 KB, patch)
2013-08-21 05:12 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.76 KB, patch)
2013-08-22 03:34 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.83 KB, patch)
2013-08-22 08:01 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.83 KB, patch)
2013-08-25 04:32 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (41.01 KB, patch)
2013-08-25 04:59 PST, Christophe Dumez
darin: review+
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (522.16 KB, application/zip)
2013-08-25 06:11 PST, Build Bot
no flags Details
Patch (40.79 KB, patch)
2013-08-26 12:28 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.80 KB, patch)
2013-08-26 12:31 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (40.33 KB, patch)
2013-08-27 00:51 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (546.46 KB, application/zip)
2013-08-27 02:31 PST, Build Bot
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-08-15 01:23:23 PST
Mozilla and Microsoft have implemented wheel event, would be great to have it in Safari too.
------- Comment #1 From 2012-08-15 12:56:03 PST -------
Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=719320

What are the differences between wheel and mousewheel?
------- Comment #2 From 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 From 2013-04-02 08:29:49 PST -------
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 From 2013-04-02 19:15:36 PST -------
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 From 2013-08-15 05:47:21 PST -------
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 From 2013-08-19 19:38:17 PST -------
(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 From 2013-08-20 23:34:52 PST -------
corresponding Blink revision:
https://src.chromium.org/viewvc/blink?revision=156404&view=revision
------- Comment #8 From 2013-08-21 00:46:39 PST -------
Created an attachment (id=209254) [details]
WIP Patch
------- Comment #9 From 2013-08-21 01:35:52 PST -------
Created an attachment (id=209256) [details]
Patch
------- Comment #10 From 2013-08-21 03:26:17 PST -------
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 From 2013-08-21 03:49:50 PST -------
(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 From 2013-08-21 05:12:19 PST -------
Created an attachment (id=209264) [details]
Patch
------- Comment #13 From 2013-08-21 05:13:19 PST -------
(In reply to comment #12)
> Created an attachment (id=209264) [details] [details]
> Patch

The signs for deltaX / deltaY are now correct as per the specification and IE/FF.
------- Comment #14 From 2013-08-22 00:32:07 PST -------
(From update of attachment 209264 [details])
I will make another iteration soon as we should report to real amount of pixels that is scrolled.
------- Comment #15 From 2013-08-22 03:34:03 PST -------
Created an attachment (id=209337) [details]
Patch
------- Comment #16 From 2013-08-22 08:01:24 PST -------
Created an attachment (id=209367) [details]
Patch
------- Comment #17 From 2013-08-25 04:32:47 PST -------
Created an attachment (id=209587) [details]
Patch

Reupload without change for mac ews.

Any feedback on this?
------- Comment #18 From 2013-08-25 04:59:38 PST -------
Created an attachment (id=209589) [details]
Patch
------- Comment #19 From 2013-08-25 06:11:20 PST -------
(From update of attachment 209589 [details])
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 From 2013-08-25 06:11:24 PST -------
Created an attachment (id=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 From 2013-08-26 09:44:44 PST -------
(From update of attachment 209589 [details])
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 From 2013-08-26 10:30:13 PST -------
(From update of attachment 209589 [details])
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 From 2013-08-26 12:28:58 PST -------
Created an attachment (id=209669) [details]
Patch

Darin, could you please take another look? (especially the EventTarget changes).
------- Comment #24 From 2013-08-26 12:31:05 PST -------
Created an attachment (id=209671) [details]
Patch

Fix typo.
------- Comment #25 From 2013-08-26 21:42:07 PST -------
(From update of attachment 209671 [details])
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 From 2013-08-27 00:51:33 PST -------
Created an attachment (id=209719) [details]
Patch for landing
------- Comment #27 From 2013-08-27 02:31:02 PST -------
(From update of attachment 209719 [details])
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 From 2013-08-27 02:31:07 PST -------
Created an attachment (id=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 From 2013-08-27 02:44:30 PST -------
(From update of attachment 209719 [details])
Test failure looks like a false positive.
------- Comment #30 From 2013-08-27 03:08:01 PST -------
(From update of attachment 209719 [details])
Clearing flags on attachment: 209719

Committed r154673: <http://trac.webkit.org/changeset/154673>
------- Comment #31 From 2013-08-27 03:08:08 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #32 From 2013-08-30 12:36:36 PST -------
This broke PDFPlugin scrolling: https://bugs.webkit.org/show_bug.cgi?id=120542