Bug 108455 - Implement WheelEvent::deltaMode
Summary: Implement WheelEvent::deltaMode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2013-01-31 04:00 PST by Kentaro Hara
Modified: 2013-02-04 16:20 PST (History)
4 users (show)

See Also:


Attachments
Patch (38.27 KB, patch)
2013-01-31 04:08 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (38.49 KB, patch)
2013-01-31 16:44 PST, Kentaro Hara
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-01-31 04:00:39 PST
Per the spec, WheelEvent::deltaMode should return DOM_DELTA_PIXEL or DOM_DELTA_LINE or DOM_DELTA_PAGE.

Spec: http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents
https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-wheelevent
Comment 1 Kentaro Hara 2013-01-31 04:08:03 PST
Created attachment 185740 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-31 04:10:11 PST
Attachment 185740 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/events/constructors/wheel-event-constructor-expected.txt', u'LayoutTests/fast/events/constructors/wheel-event-constructor.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/WheelEvent.cpp', u'Source/WebCore/dom/WheelEvent.h', u'Source/WebCore/dom/WheelEvent.idl', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebInputEventConversion.cpp']" exit_code: 1
Source/WebCore/dom/WheelEvent.h:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/dom/WheelEvent.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/dom/WheelEvent.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kentaro Hara 2013-01-31 04:13:55 PST
Comment on attachment 185740 [details]
Patch

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

>> Source/WebCore/dom/WheelEvent.h:46
>> +        DOM_DELTA_PIXEL = 0,
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

This is OK. We conventionally use the same constant name as the Web IDL spec.
Comment 4 Adam Barth 2013-01-31 10:33:41 PST
Comment on attachment 185740 [details]
Patch

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

> Source/WebCore/dom/WheelEvent.idl:28
> +    const unsigned long              DOM_DELTA_PIXEL = 0x00;
> +    const unsigned long              DOM_DELTA_LINE  = 0x01;
> +    const unsigned long              DOM_DELTA_PAGE  = 0x02;
> +

Are these constants on the WheelEvent constructor or on the instances?

> Source/WebCore/page/EventHandler.cpp:256
>      switch (deltaMode) {

Should we add a default case to this switch now that the type of deltaMode is unsigned rather than WheelEvent::DeltaMode ?
Comment 5 Adam Barth 2013-01-31 10:36:09 PST
The spec says:

---8<---
Initializes the deltaMode attribute on the WheelEvent object to the enumerated values 0, 1, or 2, which represent the amount of pixels scrolled (DOM_DELTA_PIXEL), lines scrolled (DOM_DELTA_LINE), or pages scrolled (DOM_DELTA_PAGE) if the rotation of the wheel would have resulted in scrolling.
--->8---

Does that mean the constructor should enforce that deltaMode is only initialized to 0, 1, or 2?  If the caller supplies the value 42, what is supposed to happen?
Comment 6 Kentaro Hara 2013-01-31 16:44:02 PST
Created attachment 185895 [details]
Patch
Comment 7 Kentaro Hara 2013-01-31 16:44:19 PST
(In reply to comment #4)
> > Source/WebCore/dom/WheelEvent.idl:28
> > +    const unsigned long              DOM_DELTA_PIXEL = 0x00;
> > +    const unsigned long              DOM_DELTA_LINE  = 0x01;
> > +    const unsigned long              DOM_DELTA_PAGE  = 0x02;
> > +
> 
> Are these constants on the WheelEvent constructor or on the instances?

Both. The spec requires both (http://www.w3.org/TR/WebIDL/#es-constants). The WebKit implementation is conformant with the spec.

> > Source/WebCore/page/EventHandler.cpp:256
> >      switch (deltaMode) {
> 
> Should we add a default case to this switch now that the type of deltaMode is unsigned rather than WheelEvent::DeltaMode ?

Done.

> The spec says:
> 
> ---8<---
> Initializes the deltaMode attribute on the WheelEvent object to the enumerated values 0, 1, or 2, which represent the amount of pixels scrolled (DOM_DELTA_PIXEL), lines scrolled (DOM_DELTA_LINE), or pages scrolled (DOM_DELTA_PAGE) if the rotation of the wheel would have resulted in scrolling.
> --->8---
> 
> Does that mean the constructor should enforce that deltaMode is only initialized to 0, 1, or 2?  If the caller supplies the value 42, what is supposed to happen?

Good point. As far as I read the spec, I couldn't find any explicit description about that. Given that the type of deltaMode is unsigned, I think the behavior should follow the spec of normal unsigned attributes. If so, assigning 42 will result in 42.

(c.f. If deltaMode is speced as enum, invalid value assignment will be simply ignored. http://www.w3.org/TR/WebIDL/#idl-enums)
Comment 8 WebKit Review Bot 2013-01-31 16:47:52 PST
Attachment 185895 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/events/constructors/wheel-event-constructor-expected.txt', u'LayoutTests/fast/events/constructors/wheel-event-constructor.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/WheelEvent.cpp', u'Source/WebCore/dom/WheelEvent.h', u'Source/WebCore/dom/WheelEvent.idl', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebInputEventConversion.cpp']" exit_code: 1
Source/WebCore/dom/WheelEvent.h:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/dom/WheelEvent.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/dom/WheelEvent.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Adam Barth 2013-02-01 00:21:50 PST
Comment on attachment 185895 [details]
Patch

Ok.  Would you be willing to double-check with the working group about the out-of-bounds case?
Comment 10 Kentaro Hara 2013-02-01 00:34:41 PST
(In reply to comment #9)
> (From update of attachment 185895 [details])
> Ok.  Would you be willing to double-check with the working group about the out-of-bounds case?

Sure. Will do before landing.
Comment 11 Kentaro Hara 2013-02-04 15:50:39 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 185895 [details] [details])
> > Ok.  Would you be willing to double-check with the working group about the out-of-bounds case?
> 
> Sure. Will do before landing.

I discussed with spec authors. In conclusion, we can set out-of-bound values:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=14053#c6

Will land the patch soon.
Comment 12 Kentaro Hara 2013-02-04 15:56:51 PST
Committed r141826: <http://trac.webkit.org/changeset/141826>