Bug 117306 – IndieUI: Support UIValueChangeRequestEvent

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Accessibility

[chris fleizach]

Support the UIValueChangeRequestEvent

https://dvcs.w3.org/hg/IndieUI/raw-file/default/src/indie-ui-events.html#dictionary-uivaluechangerequesteventinit-members

[chris fleizach]

Created attachment 205303 [details]
patch

[WebKit Commit Bot]

Attachment 205303 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/global-constructors-attributes-expected.txt', u'LayoutTests/indieui/uivaluechangerequest-expected.txt', u'LayoutTests/indieui/uivaluechangerequest.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.cpp', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.idl', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityNodeObject.h', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/dom/EventNames.in', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.cpp:49:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.cpp:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:44:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:45:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:57:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 205303 [details]
patch

Attachment 205303 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/878076

[Early Warning System Bot]

Comment on attachment 205303 [details]
patch

Attachment 205303 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/856396

[chris fleizach]

Created attachment 205307 [details]
patch

I think I have to ignore the style complaint about the all caps constants, since those are the names specified in the spec

[WebKit Commit Bot]

Attachment 205307 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/global-constructors-attributes-expected.txt', u'LayoutTests/indieui/uivaluechangerequest-expected.txt', u'LayoutTests/indieui/uivaluechangerequest.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.cpp', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.idl', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityNodeObject.h', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/dom/EventNames.in', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:44:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:45:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 9 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 205307 [details]
patch

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

> Source/WebCore/ChangeLog:10
> +        (Example, allow a screen reader user to change the value of a custom slider).

It seems like this line can fit on the previous line.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1019
> +    DEFINE_STATIC_LOCAL(const AtomicString, noAction, ());

Why don't we use nullAtom or emptyAtom instead?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1041
> +        uiActionValue.split(' ', uiActions);

The spec says it's a list of "a set of whitespace-separated tokens". I'm actually surprised that uiactions DOM property isn't DOMTokenList.
Perhaps we should give that feedback to the standards body.  Regardless, we should probably use DOMTokenList's algorithm to split the tokens here.
We also need a test that uses non-space whitespaces.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1061
> +        UIValueChangeRequestEventInit eventInit;
> +        eventInit.changeType = increase ? UIValueChangeRequestEvent::INCREMENT : UIValueChangeRequestEvent::DECREMENT;
> +        eventInit.receiver = receiver;

Can we add a constructor to do this?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1065
> +        RefPtr<UIValueChangeRequestEvent> event = UIValueChangeRequestEvent::create(eventNames().valuechangerequestEvent, eventInit);
> +
> +        node()->dispatchUIRequestEvent(event);

You can do both of these things in single line.

> Source/WebCore/testing/Internals.cpp:2065
> +    UIValueChangeRequestEventInit eventInit;
> +    eventInit.changeType = changeType;
> +    eventInit.receiver = receiver;

Ditto.

> Source/WebCore/testing/Internals.cpp:2069
> +    RefPtr<UIValueChangeRequestEvent> event = UIValueChangeRequestEvent::create(eventNames().valuechangerequestEvent, eventInit);
> +    
> +    target->dispatchUIRequestEvent(event);

Ditto.

[chris fleizach]

Comment on attachment 205307 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1041
>> +        uiActionValue.split(' ', uiActions);
> 
> The spec says it's a list of "a set of whitespace-separated tokens". I'm actually surprised that uiactions DOM property isn't DOMTokenList.
> Perhaps we should give that feedback to the standards body.  Regardless, we should probably use DOMTokenList's algorithm to split the tokens here.
> We also need a test that uses non-space whitespaces.

I'll inform the committee about this. I'll file another bug about testing uiactions with non space whitespaces

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1061
>> +        eventInit.receiver = receiver;
> 
> Can we add a constructor to do this?

Yes. Will add some new eventInit constructors

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1065
>> +        node()->dispatchUIRequestEvent(event);
> 
> You can do both of these things in single line.

If i do that I think the event will be leaked. I'm relying on the static allocation to dealloc the event

[chris fleizach]

(In reply to comment #8)
> (From update of attachment 205307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205307&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1041
> >> +        uiActionValue.split(' ', uiActions);
> > 
> > The spec says it's a list of "a set of whitespace-separated tokens". I'm actually surprised that uiactions DOM property isn't DOMTokenList.
> > Perhaps we should give that feedback to the standards body.  Regardless, we should probably use DOMTokenList's algorithm to split the tokens here.
> > We also need a test that uses non-space whitespaces.
> 
> I'll inform the committee about this. I'll file another bug about testing uiactions with non space whitespaces

https://bugs.webkit.org/show_bug.cgi?id=118270

> 
> >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1061
> >> +        eventInit.receiver = receiver;
> > 
> > Can we add a constructor to do this?
> 
> Yes. Will add some new eventInit constructors
> 
> >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1065
> >> +        node()->dispatchUIRequestEvent(event);
> > 
> > You can do both of these things in single line.
> 
> If i do that I think the event will be leaked. I'm relying on the static allocation to dealloc the event

[James Craig]

INDIEUI-ACTION-60: uiactions attr value should be a DOMTokenList
https://www.w3.org/WAI/IndieUI/track/actions/60

[chris fleizach]

Created attachment 206341 [details]
patch

This should address Ryosuke's comments

[WebKit Commit Bot]

Attachment 206341 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/global-constructors-attributes-expected.txt', u'LayoutTests/indieui/uivaluechangerequest-expected.txt', u'LayoutTests/indieui/uivaluechangerequest.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/Modules/indieui/UIRequestEvent.cpp', u'Source/WebCore/Modules/indieui/UIRequestEvent.h', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.cpp', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h', u'Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.idl', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityNodeObject.h', u'Source/WebCore/dom/EventNames.h', u'Source/WebCore/dom/EventNames.in', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl']" exit_code: 1
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:45:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/Modules/indieui/UIValueChangeRequestEvent.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 9 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Craig]

FYI, the uiactions IDL attribute is now a DOMTokenList, not a DOMString.
The uiactions content attribute is still a string, of course.

https://dvcs.w3.org/hg/IndieUI/rev/3ab9f0b62e4d

[James Craig]

…which means you'll need to update this test at the very least:
LayoutTests/indieui/uiactions.html

[chris fleizach]

(In reply to comment #14)
> …which means you'll need to update this test at the very least:
> LayoutTests/indieui/uiactions.html

That change will need to come in a different patch, and which point this test will get updated yes

[Radar WebKit Bug Importer]

<rdar://problem/14407328>

[chris fleizach]

(In reply to comment #7)
> (From update of attachment 205307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205307&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        (Example, allow a screen reader user to change the value of a custom slider).
> 
> It seems like this line can fit on the previous line.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1019
> > +    DEFINE_STATIC_LOCAL(const AtomicString, noAction, ());
> 
> Why don't we use nullAtom or emptyAtom instead?
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1041
> > +        uiActionValue.split(' ', uiActions);
> 
> The spec says it's a list of "a set of whitespace-separated tokens". I'm actually surprised that uiactions DOM property isn't DOMTokenList.
> Perhaps we should give that feedback to the standards body.  Regardless, we should probably use DOMTokenList's algorithm to split the tokens here.
> We also need a test that uses non-space whitespaces.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1061
> > +        UIValueChangeRequestEventInit eventInit;
> > +        eventInit.changeType = increase ? UIValueChangeRequestEvent::INCREMENT : UIValueChangeRequestEvent::DECREMENT;
> > +        eventInit.receiver = receiver;
> 
> Can we add a constructor to do this?
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1065
> > +        RefPtr<UIValueChangeRequestEvent> event = UIValueChangeRequestEvent::create(eventNames().valuechangerequestEvent, eventInit);
> > +
> > +        node()->dispatchUIRequestEvent(event);
> 
> You can do both of these things in single line.
> 
> > Source/WebCore/testing/Internals.cpp:2065
> > +    UIValueChangeRequestEventInit eventInit;
> > +    eventInit.changeType = changeType;
> > +    eventInit.receiver = receiver;
> 
> Ditto.
> 
> > Source/WebCore/testing/Internals.cpp:2069
> > +    RefPtr<UIValueChangeRequestEvent> event = UIValueChangeRequestEvent::create(eventNames().valuechangerequestEvent, eventInit);
> > +    
> > +    target->dispatchUIRequestEvent(event);
> 
> Ditto.

Hi Ryosuke, could you take a look again?

[Brady Eidson]

Comment on attachment 206341 [details]
patch

Assuming that patches for review since 2013 are stale, r-

[James Craig]

Closing. This approach/spec is no longer being pursued.