Bug 130726 – Web Inspector: AXI: clarify button roles (e.g. toggle or popup button)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[James Craig]

Web Inspector: AXI: clarify button roles (e.g. toggle or popup button)

AccessibilityObject::isButton()
AccessibilityObject::buttonRoleType() toggle or popup button

Examples for standard button and one with aria-haspopup and aria-pressed:
Role: button
Role: button (popup)
Role: button (toggle)

Not sure what it means to have a popup toggle button, but it'd be worth investigate what WebKit does in this scenario. 

Note: using div here b/c native  button element has no support for pressed state.
<div role="button" aria-haspopup="true" aria-pressed="true" tabindex="0">foo</div>

[Radar WebKit Bug Importer]

<rdar://problem/16420420>

[James Craig]

Probably needs a special case in AccessibilityObject::computedRoleString()

    if (role == ToggleButtonRole || role == PopupButtonRole )
        role = ButtonRole;

As well as some additional output string manipulation to the role section in DOMNodeDetailsSidebarPanel.js

[James Craig]

Example: http://files.paciellogroup.com/blogmisc/ARIA/togglebutton.html

[Aaron Chu]

Created attachment 292511 [details]
Patch

[James Craig]

Comment on attachment 292511 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2129
> +        { "popupbutton", PopUpButtonRole },

Unless you’ve accounted for this in the platform mappings, this will likely regress how these elements are exposed to the platform.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2151
> +        { "togglebutton", ToggleButtonRole },

Ditto

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:31
> +localizedStrings["%s (%s)"] = "%s (%s)";

I like this approach, but you should remove "%s (default)" and "%s (computed)" at the same time.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:34
>  localizedStrings["%s (hidden)"] = "%s (hidden)";

Possibly this one, too.

[James Craig]

Comment on attachment 292511 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:2129
>> +        { "popupbutton", PopUpButtonRole },
> 
> Unless you’ve accounted for this in the platform mappings, this will likely regress how these elements are exposed to the platform.

I was wrong about regressing existing elements, but after further review, this would allow 'popupbutton' as a token role value, (e.g. role="popupbutton"), which is not what you want.

I think you should rather add the following special case to AccessibilityObject::computedRoleString():

    if (role == PopUpButtonRole || role == ToggleButtonRole)
        role = ButtonRole;

Then the reverse map would return the "button" string, instead of allowing authors to use new role tokens.

An then you'd need some conditionals in the WebInspector UI to display the role differently depending on whether it had a pressed value "button (toggle, computed)" or had a popup "button (popup, default)"?

>> Source/WebCore/accessibility/AccessibilityObject.cpp:2151
>> +        { "togglebutton", ToggleButtonRole },
> 
> Ditto

Ditto. This would allow role="togglebutton".

> LayoutTests/accessibility/roles-computedRoleString-expected.txt:58
> +PASS: select:not([multiple]) -> popupbutton. 

This should return "button" not "popupbutton"

> LayoutTests/accessibility/roles-computedRoleString.html:85
> +<select data-role="popupbutton" class="ex" data-note=":not([multiple])">

Expected value for the Mac implementation should be "button" not "popupbutton", though it's possible the presentation of htlm:select may be different on other platforms.

> LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:26
> -    hierarchyLevel: 2
> +    hierarchyLevel: 1

This seems unrelated. If you can verify this also fails w/o your patch, check it in with the failure, and mark that test as skipped in TestExpectations.

> LayoutTests/platform/gtk/accessibility/roles-computedRoleString-expected.txt:-1
> -This tests that native elements and ARIA overrides result in the same ARIA computed role, regardless of platform.

You shouldn't need to remove this, as the test is skipped already for GTK.

webkit.org/b/163383 accessibility/roles-computedRoleString.html [ Failure ]

> LayoutTests/platform/mac/accessibility/roles-computedRoleString-expected.txt:-2
> -This tests that native elements and ARIA overrides result in the same ARIA computed role, regardless of platform.
> -

Copy your output to the platform-specific directory instead of removing it.

[Aaron Chu]

Created attachment 292781 [details]
Patch

[James Craig]

Comment on attachment 292781 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/DOM.json:94
> +                { "name": "isPopUpButton", "type": "boolean", "optional": true, "description": "Wheather an element is a popup button." },

Since this is flagging on @aria-haspopup, the property name should not be limited to popup buttons. Use hasPopUp or triggersPopUp or something similar, without referencing the "button" presentation.

Also, minor typo: "Wheather"

"Indicates whether an element triggers displaying a popup menu, dialog, etc."

> Source/WebCore/accessibility/AccessibilityObject.cpp:2201
>      if (role == HorizontalRuleRole)
>          role = SplitterRole;
> -    
> +    if (role == ToggleButtonRole || role == PopUpButtonRole)
> +        role = ButtonRole;

Minor nits:

1. Since these special case conditionals are all related, there's no need to add the extra line breaks between them.
2. I prefer alphabetical order if everything else is the same: the PopUpButtonRole check should precede ToggleButtonRole.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:503
> +                            buttonType = "toggle";

Please localize "toggle"

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:510
> +                            buttonType = (buttonType) ? buttonType + WebInspector.UIString(", ") + "popup" : "popup";

Please localize "popup"...

It may also be better to do a replacement string ("%s, %s") rather than concatenation.

> LayoutTests/accessibility/roles-computedRoleString-expected.txt:45
> +PASS: mark -> . 

Is this unrelated?

> LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:542
>  <div role="button" tabindex="0" aria-pressed="false">Not Pressed.</div>

You should add some more tests to this file for <select> and @aria-haspopup to expose your new API property.

> ManualTests/accessibility/inspector-to-expose-popup-toggle-button.html:8
> +<li>In the inspectr, you should be able to see the role values for each button. No button should have the role value of "No exact ARIA role match."</li>

m. typo

[Blaze Burg]

Comment on attachment 292781 [details]
Patch

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

I'd like to get you familiar with writing inspector tests before we start adding a lot more code, so please redo the manual test and address the other small issues.

> Source/JavaScriptCore/ChangeLog:9
> +        Exposing isPopUpButton() to the inspector to determine if a button is a

"Add the isPopupButton flag to the AccessibilityProperties type."

> Source/WebCore/ChangeLog:12
> +        Added special case logic to make sure PopUppButtonRole and ToggleButtonRole to user ButtonRole as role.

Nit: PopUpButtonRole

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1783
>              level = hierarchicalLevel ? hierarchicalLevel : headingLevel;

it looks like hierarchicalLevel should be returning an Optional<T> type, but this doesn't need to be fixed here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1784
> +            

Extra newlines not necessary here.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:494
> +                var hasPopup = accessibilityProperties.isPopupButton;

Nit: let

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:510
> +                            buttonType = (buttonType) ? buttonType + WebInspector.UIString(", ") + "popup" : "popup";

This will not be easily localizable. It's easier for localizers if we have one localizable string per combination. To do this, you should add the types to an array and sort them, then add a string for each possible value.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:520
> +                        var extraRoleInfo = buttonType + WebInspector.UIString(", ") + roleType;

Nit: you should use let instead of var and hoist the variable definitions up a bit.

Since we can't attach descriptions to localizable string keys currently, the localizers will have no idea what ", " is supposed to do.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:523
> +                    if (extraRoleInfo) {

Nit: braces not needed here.

> LayoutTests/accessibility/axi-to-expose-popup-and-toggle-button.html:17
> +    var popupbutton = document.createElement('button');

Nit: use let in new JS code.

> ManualTests/accessibility/inspector-to-expose-popup-toggle-button.html:5
> +<p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=130726">Bug 130726: Web Inspector: AXI: clarify button roles (e.g. toggle or popup button)</a>.</p>

This should be a normal Inspector test. You could have the same page content, then in a test case you can request accessibility properties of the button elements and verify they have the right flags. You should look at a recently-authored inspector test to see how to use AsyncTestSuite. Let me know if you need more pointers.

> ManualTests/accessibility/inspector-to-expose-popup-toggle-button.html:7
> +<li>Open Accessibility Inspector and inspector each buttons.</li>

Nit: inspect each button

> ManualTests/accessibility/inspector-to-expose-popup-toggle-button.html:8
> +<li>In the inspectr, you should be able to see the role values for each button. No button should have the role value of "No exact ARIA role match."</li>

Nit: inspector

[Aaron Chu]

Comment on attachment 292781 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/DOM.json:94
>> +                { "name": "isPopUpButton", "type": "boolean", "optional": true, "description": "Wheather an element is a popup button." },
> 
> Since this is flagging on @aria-haspopup, the property name should not be limited to popup buttons. Use hasPopUp or triggersPopUp or something similar, without referencing the "button" presentation.
> 
> Also, minor typo: "Wheather"
> 
> "Indicates whether an element triggers displaying a popup menu, dialog, etc."

this property is not just flagging @aria-haspopup, it is actually derived from "axObject->isPopUpButton() || axObject->ariaHasPopup();" and so it is also checking the role of the element.

[Aaron Chu]

Created attachment 293799 [details]
Patch

[Blaze Burg]

Comment on attachment 293799 [details]
Patch

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

r=me. Please fix problems as noted and upload a clean patch for landing via commit queue.

> Source/WebCore/ChangeLog:11
> +        Added special case logic to make sure PopUppButtonRole and ToggleButtonRole to user ButtonRole as role.

Typo: PopUpButtonRole

> Source/JavaScriptCore/inspector/protocol/DOM.json:101
> +                { "name": "isPopUpButton", "type": "boolean", "optional": true, "description": "Wheather an element is a popup button." },

Typo: Wheather

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:511
> +                        if (pressed !== "") {

Nit: just if (pressed) is sufficient here, and braces not needed for single line then

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:517
> +                        // a popup button, we only include "popup"

Nit: end sentence with period.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:519
> +                            let buttonTypeString

Not sure what this declaration is for.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:530
> +                        role = WebInspector.UIString("%s (%s, %s)").format(role, buttonType, roleType);

Please add a FIXME that we need to add localizer notes to these UIStrings. This is blocked on https://bugs.webkit.org/show_bug.cgi?id=153962.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:534
> +                    }

Sidebar: this function is massive and messy and not testable as-is. In the longer term, we should move most of this logic into a Model class and write tests there.

[Aaron Chu]

Created attachment 294086 [details]
Patch

[Timothy Hatcher]

Comment on attachment 294086 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:509
> +

Remove blank line here.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:519
> +                        if (hasPopup) {
> +                            buttonType = (buttonType) ? buttonTypePopupToggleString : buttonTypePopupString;
> +                        }

Style: Drop the braces. Also drop the () around (buttonType).

[Aaron Chu]

Created attachment 294169 [details]
Patch

[Build Bot]

Comment on attachment 294169 [details]
Patch

Attachment 294169 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2487108

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/transaction-create_in_versionchange.htm
imported/w3c/web-platform-tests/IndexedDB/transaction-abort-index-metadata-revert.html

[Build Bot]

Created attachment 294335 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

Comment on attachment 294169 [details]
Patch

r=me again. The EWS failures are unrelated. Let me know if you need me to land the patch if commit-queue has trouble.

[WebKit Commit Bot]

Comment on attachment 294169 [details]
Patch

Clearing flags on attachment: 294169

Committed r208540: <http://trac.webkit.org/changeset/208540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.