Bug 108455 – Implement WheelEvent::deltaMode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 185740 [details]
Patch

[WebKit Review Bot]

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.

[Kentaro Hara]

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.

[Adam Barth]

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 ?

[Adam Barth]

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?

[Kentaro Hara]

Created attachment 185895 [details]
Patch

[Kentaro Hara]

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

[WebKit Review Bot]

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.

[Adam Barth]

Comment on attachment 185895 [details]
Patch

Ok.  Would you be willing to double-check with the working group about the out-of-bounds case?

[Kentaro Hara]

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

[Kentaro Hara]

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

[Kentaro Hara]

Committed r141826: <http://trac.webkit.org/changeset/141826>