Bug 146012 – AX: ARIA 1.1 @aria-current

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

https://rawgit.com/w3c/aria/master/aria/aria.html#aria-current

[Radar WebKit Bug Importer]

<rdar://problem/21833925>

[Nan Wang]

Created attachment 260300 [details]
patch

[Nan Wang]

Created attachment 260310 [details]
patch

[chris fleizach]

Comment on attachment 260310 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Updated inspector to support aria-current.

we  should add a WebINspector layout test for these changes too

> Source/WebCore/accessibility/AccessibilityObject.cpp:1834
> +    String currentStateValue = stripLeadingAndTrailingHTMLSpaces(getAttribute(aria_currentAttr));

we should have a unit test within our test that confirms extra white space is removed correctly

thinks also like

"   page "

> Source/WebCore/accessibility/AccessibilityObject.h:439
> +enum AccessibilityCurrentState { CurrentFalse, CurrentTrue, CurrentPage, CurrentStep, CurrentLocation, CurrentDate, CurrentTime };

I think "current" by itself is too vague to be shortened on its own. Maybe we should either call this

ARIACurrent or
ContainerCurrentItem

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2884
> +        switch (currentState) {

we can make this switch (m_object->currentState()) {

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:352
> +                if (accessibilityProperties.current === DOMAgent.AccessibilityPropertiesCurrent.True)

seems like we should use a switch statement here

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:353
> +                    current = WebInspector.UIString("Yes");

i think this should be "True" instead of Yes, because this is not a binary (yes/no) property but rather a multi-value thing where "true" is different from 'yes"

> LayoutTests/ChangeLog:8
> +        * accessibility/aria-current-expected.txt: Added.

you probably need to add Skip to other TestExpectations and make bugs for Win and GTK to add support to query for aria-current

[Nan Wang]

Created attachment 260894 [details]
patch

[Nan Wang]

Created attachment 260895 [details]
patch

[chris fleizach]

Comment on attachment 260895 [details]
patch

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

> Source/JavaScriptCore/inspector/protocol/DOM.json:83
> +                { "name": "ARIACurrent", "type": "string", "optional": true, "enum": ["true", "false", "page", "step", "location", "date", "time"], "description": "Current item within a container or set of related elements." },

do we name any other properties with ARIA as the prefix in the node inspector?
if not, we might as well leave just this string as "current"... i think internally and with the platform API we can leave it ARIACurrent, but node inspector can probably stick with "current"

[Nan Wang]

(In reply to comment #7)
> Comment on attachment 260895 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260895&action=review
> 
> > Source/JavaScriptCore/inspector/protocol/DOM.json:83
> > +                { "name": "ARIACurrent", "type": "string", "optional": true, "enum": ["true", "false", "page", "step", "location", "date", "time"], "description": "Current item within a container or set of related elements." },
> 
> do we name any other properties with ARIA as the prefix in the node
> inspector?
> if not, we might as well leave just this string as "current"... i think
> internally and with the platform API we can leave it ARIACurrent, but node
> inspector can probably stick with "current"

Okay, will make the change

[Nan Wang]

Created attachment 260897 [details]
patch

[Nan Wang]

Comment on attachment 260897 [details]
patch

Obsolete the patch since we are going for another approach that the current item object should also be exposed to its parent.

[Darin Adler]

Comment on attachment 260897 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1830
> +    String falseValue = ASCIILiteral("false");
> +    String pageValue = ASCIILiteral("page");
> +    String stepValue = ASCIILiteral("step");
> +    String locationValue = ASCIILiteral("location");
> +    String dateValue = ASCIILiteral("date");
> +    String timeValue = ASCIILiteral("time");

Putting these into local String variables is not helpful. We actually miss the opportunity to get the optimized version of == and waste time every time this function is called, allocating and deallocating 6 extra strings. I suggest just comparing directly with the string literals, or using locals of type const char[] or const char*.

[Nan Wang]

Created attachment 261023 [details]
patch

The exposing current item to parent approach has some complex edge cases that need to be taken care of. 
Will go for the original proposal for now.

[chris fleizach]

Comment on attachment 261023 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1825
> +    // aria-current can return false (default), true, page, step, location, date or time

end this line with a period because it's a full sentence

> Source/WebCore/accessibility/AccessibilityObject.cpp:1843
> +    // Any value not included in the list of allowed values should be treated as "true"

ditto about period

[Nan Wang]

Created attachment 261031 [details]
patch

Added periods to comments.

[WebKit Commit Bot]

Comment on attachment 261031 [details]
patch

Clearing flags on attachment: 261031

Committed r189642: <http://trac.webkit.org/changeset/189642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nan Wang]

Created attachment 261418 [details]
example

Added example for "aria-current".

[Joseph Scheuhammer]

Question:  do the changes here incur a notification?  I've looked over the code, and don't see one, but I may have missed it.

The other platforms -- UIA, IA2, ATK-AT-SPI -- do provide an event when the value of aria-current changes.  See:

http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#event-aria-current

Can someone confirm what AXAPI does in this regard?  Thanks.

[Nan Wang]

(In reply to comment #18)
> Question:  do the changes here incur a notification?  I've looked over the
> code, and don't see one, but I may have missed it.
> 
> The other platforms -- UIA, IA2, ATK-AT-SPI -- do provide an event when the
> value of aria-current changes.  See:
> 
> http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#event-aria-current
> 
> Can someone confirm what AXAPI does in this regard?  Thanks.

There's no specific notification set up for aria-current changes. But it will incur the AXAriaAttributeChanged notification by default.

[Joseph Scheuhammer]

(In reply to comment #19)
> (In reply to comment #18)
> > Question:  do the changes here incur a notification?
> > ...
> > 
> 
> There's no specific notification set up for aria-current changes. But it
> will incur the AXAriaAttributeChanged notification by default.

Thanks.

I assume that AXAriaAttributeChanged is a general notification of when any aria-* attribute changes.

Is the recommendation that the mapping specification explicitly state that AXAriaAttributeChanged is the appropriate notification in the case of changes to aria-current?

(By "mapping specification", I mean these entries:
http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#event-aria-current)

[Nan Wang]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Question:  do the changes here incur a notification?
> > > ...
> > > 
> > 
> > There's no specific notification set up for aria-current changes. But it
> > will incur the AXAriaAttributeChanged notification by default.
> 
> Thanks.
> 
> I assume that AXAriaAttributeChanged is a general notification of when any
> aria-* attribute changes.
> 
> Is the recommendation that the mapping specification explicitly state that
> AXAriaAttributeChanged is the appropriate notification in the case of
> changes to aria-current?
> 
> (By "mapping specification", I mean these entries:
> http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#event-aria-current)

There's no explicit statement for that but it's appropriate to add that notification to almost all of the attributes listed. 

Many of the entries just list "Not Mapped", however, "No Notification" is a more accurate phrasing.

[Joseph Scheuhammer]

(In reply to comment #21)
> 
> > ... 
> > Thanks.
> > 
> > I assume that AXAriaAttributeChanged is a general notification of when any
> > aria-* attribute changes.
> > 
> > Is the recommendation that the mapping specification explicitly state that
> > AXAriaAttributeChanged is the appropriate notification in the case of
> > changes to aria-current?
> > 
> > (By "mapping specification", I mean these entries:
> > http://rawgit.com/w3c/aria/master/core-aam/core-aam.html#event-aria-current)
> 
> There's no explicit statement for that but it's appropriate to add that
> notification to almost all of the attributes listed. 
> 
> Many of the entries just list "Not Mapped", however, "No Notification" is a
> more accurate phrasing.


The phrase, "Not Mapped" is also used for the other platforms, e.g., IAccessible2.  There "No Event" is the more accurate phrase.  It might be a good idea to change them both.

For now, I'm going add "No Notification" for aria-current changes for AXAPI.

Thanks again.