Bug 129781 - Web Inspector: AXI: Expose checked/disabled/expanded/pressed/readonly/selected
Summary: Web Inspector: AXI: Expose checked/disabled/expanded/pressed/readonly/selected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: James Craig
URL:
Keywords: InRadar
Depends on:
Blocks: 129779
  Show dependency treegraph
 
Reported: 2014-03-05 22:22 PST by James Craig
Modified: 2014-03-10 21:46 PDT (History)
13 users (show)

See Also:


Attachments
working diff, still needs layout tests (25.97 KB, patch)
2014-03-06 03:34 PST, James Craig
no flags Details | Formatted Diff | Diff
patch (33.94 KB, patch)
2014-03-06 15:37 PST, James Craig
no flags Details | Formatted Diff | Diff
patch (33.94 KB, patch)
2014-03-06 15:47 PST, James Craig
timothy: review-
Details | Formatted Diff | Diff
patch with review feedback (34.10 KB, patch)
2014-03-07 19:29 PST, James Craig
no flags Details | Formatted Diff | Diff
patch with review feedback (and layout test update) (34.09 KB, patch)
2014-03-07 19:40 PST, James Craig
no flags Details | Formatted Diff | Diff
patch including AccessibilityProperties::Checked::Enum (35.50 KB, patch)
2014-03-08 01:46 PST, James Craig
no flags Details | Formatted Diff | Diff
patch (removed unnecessary Inspector namespace for brevity) (35.45 KB, patch)
2014-03-08 01:57 PST, James Craig
no flags Details | Formatted Diff | Diff
patch with review feedback (36.16 KB, patch)
2014-03-10 13:28 PDT, James Craig
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
patch with review feedback (37.24 KB, patch)
2014-03-10 20:42 PDT, James Craig
joepeck: review+
jcraig: commit-queue-
Details | Formatted Diff | Diff
patch with review feedback (37.12 KB, patch)
2014-03-10 20:55 PDT, James Craig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2014-03-05 22:22:04 PST
Web Inspector: AXI: Expose checked/selected/readonly/pressed/expanded

<rdar://problem/16245373>
Comment 1 James Craig 2014-03-06 03:34:58 PST
Created attachment 225976 [details]
working diff, still needs layout tests
Comment 2 James Craig 2014-03-06 15:37:18 PST
Created attachment 226051 [details]
patch
Comment 3 WebKit Commit Bot 2014-03-06 15:42:15 PST
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.
Comment 4 James Craig 2014-03-06 15:47:40 PST
Created attachment 226055 [details]
patch
Comment 5 Timothy Hatcher 2014-03-07 13:51:55 PST
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.
Comment 6 James Craig 2014-03-07 19:29:53 PST
Created attachment 226197 [details]
patch with review feedback
Comment 7 James Craig 2014-03-07 19:34:37 PST
Forgot to update the layout test. New patch coming shortly.
Comment 8 James Craig 2014-03-07 19:40:34 PST
Created attachment 226198 [details]
patch with review feedback (and layout test update)
Comment 9 James Craig 2014-03-08 01:46:02 PST
Created attachment 226207 [details]
patch including AccessibilityProperties::Checked::Enum
Comment 10 James Craig 2014-03-08 01:57:24 PST
Created attachment 226209 [details]
patch (removed unnecessary Inspector namespace for brevity)
Comment 11 James Craig 2014-03-10 13:28:21 PDT
Created attachment 226331 [details]
patch with review feedback
Comment 12 Joseph Pecoraro 2014-03-10 14:02:05 PDT
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.
Comment 13 chris fleizach 2014-03-10 14:20:54 PDT
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
Comment 14 James Craig 2014-03-10 18:26:34 PDT
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
Comment 15 Joseph Pecoraro 2014-03-10 19:28:05 PDT
(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.
Comment 16 James Craig 2014-03-10 20:42:14 PDT
Created attachment 226379 [details]
patch with review feedback
Comment 17 James Craig 2014-03-10 20:46:56 PDT
Comment on attachment 226379 [details]
patch with review feedback

cq- b/c I'm going to make the last "function fName()" change Joe wanted.
Comment 18 James Craig 2014-03-10 20:55:51 PDT
Created attachment 226380 [details]
patch with review feedback
Comment 19 WebKit Commit Bot 2014-03-10 21:46:11 PDT
Comment on attachment 226380 [details]
patch with review feedback

Clearing flags on attachment: 226380

Committed r165430: <http://trac.webkit.org/changeset/165430>
Comment 20 WebKit Commit Bot 2014-03-10 21:46:15 PDT
All reviewed patches have been landed.  Closing bug.