https://rawgit.com/w3c/aria/master/aria/aria.html#aria-current
<rdar://problem/21833925>
Created attachment 260300 [details] patch
Created attachment 260310 [details] patch
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
Created attachment 260894 [details] patch
Created attachment 260895 [details] patch
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"
(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
Created attachment 260897 [details] patch
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 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*.
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 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
Created attachment 261031 [details] patch Added periods to comments.
Comment on attachment 261031 [details] patch Clearing flags on attachment: 261031 Committed r189642: <http://trac.webkit.org/changeset/189642>
All reviewed patches have been landed. Closing bug.
Created attachment 261418 [details] example Added example for "aria-current".
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.
(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.
(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)
(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.
(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.