Bug 129781 – Web Inspector: AXI: Expose checked/disabled/expanded/pressed/readonly/selected

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

[James Craig]

Web Inspector: AXI: Expose checked/selected/readonly/pressed/expanded

<rdar://problem/16245373>

[James Craig]

Created attachment 225976 [details]
working diff, still needs layout tests

[James Craig]

Created attachment 226051 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:285:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:286:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:304:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:305:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:306:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:318:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 6 in 11 files


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

[James Craig]

Created attachment 226055 [details]
patch

[Timothy Hatcher]

Comment on attachment 226055 [details]
patch

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

> Source/WebCore/inspector/protocol/DOM.json:62
> +                { "name": "checked", "type": "integer", "optional": true, "description": "Returns checked state if supported: unchecked (0), checked (1), or mixed (2)" },

This should be an enum not a number. That way the protocol is backwards compatible.

[James Craig]

Created attachment 226197 [details]
patch with review feedback

[James Craig]

Forgot to update the layout test. New patch coming shortly.

[James Craig]

Created attachment 226198 [details]
patch with review feedback (and layout test update)

[James Craig]

Created attachment 226207 [details]
patch including AccessibilityProperties::Checked::Enum

[James Craig]

Created attachment 226209 [details]
patch (removed unnecessary Inspector namespace for brevity)

[James Craig]

Created attachment 226331 [details]
patch with review feedback

[Joseph Pecoraro]

Comment on attachment 226331 [details]
patch with review feedback

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

Looks good, but a lot of style nits and comments. r=me, but feel free to put up another patch addressing these comments.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1441
> +                int checkValue = axObject->checkboxOrRadioValue(); // element using aria-checked

Comment style should be a sentence starting with a capital, ending with a period.

    // Element using aria-checked.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1446
> +                else if (axObject->isChecked()) // native checkbox

Ditto.

> Source/WebCore/inspector/protocol/DOM.json:64
> +                { "name": "checked", "type": "string", "optional": true, "enum": ["true", "false", "mixed"], "description": "Returns checked state if supported." },
> +                { "name": "disabled", "type": "boolean", "optional": true, "description": "Returns disabled state if supported." },
>                  { "name": "exists", "type": "boolean", "description": "Returns whether there is an existing AX object for the DOM node. If this is false, all the other properties will be default values." },

Now that I think about it, these comments all say "Returns ..." but they aren't returning anything. They are properties.

So:

    "Returns checked state if supported"

Can just be something like:

    "Checked state. Not available if not supported."

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:244
> +    _refreshAccessibility: (function(){

Nit: Although what you're doing here (running an anonymous function) is probably fine, I would recommend against it. Just go with the normal pattern.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:249
> +        var booleanValueToLocalizedStringIfTrue = function(property) {

Style: For each of these, change:

    var functionName = function() { ... }

To:

    function functionName() { ... }

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:270
> +            // make sure the current set of properties is available in the closure scope for the helper functions

Style: Same comment about comments being sentences. Capital and period. This applies to all comments in the patch.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:363
> +        var refreshAX = function(){

Style: Space between parens and opening brace: "function() {"

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:368
> +            domNode.accessibilityProperties(accessibilityPropertiesCallback.bind(this));

Oops. We should be feature checking if the backend supports DOMAgent.getAccessibilityPropertiesForNode before attempting to do any of this code at all, otherwise iOS 6 and iOS 7 will run this code and throw an exception. I'll file a separate bug on this.

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

Style: Unnecessary blank line.

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt:7
> +    label: 

There are no tests for label. Or if there are, label is always empty in the output. Might be good to include a test for it.

[chris fleizach]

Comment on attachment 226331 [details]
patch with review feedback

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1937
> +    return role == CheckBoxRole || role == MenuItemCheckboxRole || role == MenuItemRadioRole || role == RadioButtonRole;

this should be a switch statement

[James Craig]

Comment on attachment 226331 [details]
patch with review feedback

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

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:244
>> +    _refreshAccessibility: (function(){
> 
> Nit: Although what you're doing here (running an anonymous function) is probably fine, I would recommend against it. Just go with the normal pattern.

The normal pattern reinitializes the helper functions and callback every time the function is called. Setting up the private scope and returning the local refreshAX function is more efficient.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:249
>> +        var booleanValueToLocalizedStringIfTrue = function(property) {
> 
> Style: For each of these, change:
> 
>     var functionName = function() { ... }
> 
> To:
> 
>     function functionName() { ... }

Tied to the above comment, I think the var pattern is more common in modern JavaScript, because it more clearly indicates the scope of this var is limited to the closure.

>> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt:7
>> +    label: 
> 
> There are no tests for label. Or if there are, label is always empty in the output. Might be good to include a test for it.

There's a fix coming for this soon.
// FIXME: label will always come back as empty. Blocked by http://webkit.org/b/121134

[Joseph Pecoraro]

(In reply to comment #14)
> (From update of attachment 226331 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226331&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:244
> >> +    _refreshAccessibility: (function(){
> > 
> > Nit: Although what you're doing here (running an anonymous function) is probably fine, I would recommend against it. Just go with the normal pattern.
> 
> The normal pattern reinitializes the helper functions and callback every time the function is called. Setting up the private scope and returning the local refreshAX function is more efficient.

We do call Function.prototype.bind each time, but I'm not sure if it is that inefficient.

The tradeoff here is upfront execution (running the anonymous closure to create the final function). Maybe we can find a middle ground. For now you can keep what you're doing. I think we may even do it in a few places.

[James Craig]

Created attachment 226379 [details]
patch with review feedback

[James Craig]

Comment on attachment 226379 [details]
patch with review feedback

cq- b/c I'm going to make the last "function fName()" change Joe wanted.

[James Craig]

Created attachment 226380 [details]
patch with review feedback

[WebKit Commit Bot]

Comment on attachment 226380 [details]
patch with review feedback

Clearing flags on attachment: 226380

Committed r165430: <http://trac.webkit.org/changeset/165430>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.