Bug 146012 - AX: ARIA 1.1 @aria-current
Summary: AX: ARIA 1.1 @aria-current
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-16 02:09 PDT by James Craig
Modified: 2016-01-25 10:19 PST (History)
13 users (show)

See Also:


Attachments
patch (28.35 KB, patch)
2015-08-31 11:38 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (25.86 KB, patch)
2015-08-31 12:54 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (35.26 KB, patch)
2015-09-09 17:47 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (35.26 KB, patch)
2015-09-09 17:54 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (35.06 KB, patch)
2015-09-09 18:24 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (34.82 KB, patch)
2015-09-11 16:11 PDT, Nan Wang
cfleizach: review+
Details | Formatted Diff | Diff
patch (34.82 KB, patch)
2015-09-11 16:50 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
example (1.76 KB, text/html)
2015-09-17 14:02 PDT, Nan Wang
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Radar WebKit Bug Importer 2015-07-15 06:28:59 PDT
<rdar://problem/21833925>
Comment 2 Nan Wang 2015-08-31 11:38:52 PDT
Created attachment 260300 [details]
patch
Comment 3 Nan Wang 2015-08-31 12:54:07 PDT
Created attachment 260310 [details]
patch
Comment 4 chris fleizach 2015-09-09 14:53:14 PDT
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
Comment 5 Nan Wang 2015-09-09 17:47:28 PDT
Created attachment 260894 [details]
patch
Comment 6 Nan Wang 2015-09-09 17:54:43 PDT
Created attachment 260895 [details]
patch
Comment 7 chris fleizach 2015-09-09 18:07:55 PDT
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"
Comment 8 Nan Wang 2015-09-09 18:09:10 PDT
(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
Comment 9 Nan Wang 2015-09-09 18:24:17 PDT
Created attachment 260897 [details]
patch
Comment 10 Nan Wang 2015-09-10 14:20:27 PDT
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.
Comment 11 Darin Adler 2015-09-11 09:38:55 PDT
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*.
Comment 12 Nan Wang 2015-09-11 16:11:15 PDT
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.
Comment 13 chris fleizach 2015-09-11 16:44:54 PDT
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
Comment 14 Nan Wang 2015-09-11 16:50:45 PDT
Created attachment 261031 [details]
patch

Added periods to comments.
Comment 15 WebKit Commit Bot 2015-09-11 17:47:32 PDT
Comment on attachment 261031 [details]
patch

Clearing flags on attachment: 261031

Committed r189642: <http://trac.webkit.org/changeset/189642>
Comment 16 WebKit Commit Bot 2015-09-11 17:47:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Nan Wang 2015-09-17 14:02:21 PDT
Created attachment 261418 [details]
example

Added example for "aria-current".
Comment 18 Joseph Scheuhammer 2016-01-15 10:57:18 PST
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.
Comment 19 Nan Wang 2016-01-15 11:33:54 PST
(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.
Comment 20 Joseph Scheuhammer 2016-01-18 11:31:19 PST
(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)
Comment 21 Nan Wang 2016-01-20 10:07:44 PST
(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.
Comment 22 Joseph Scheuhammer 2016-01-25 10:19:51 PST
(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.