Bug 130264 – Web Inspector: AXI: Expose Accessibility Tree children of the selected node

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

[James Craig]

Web Inspector: AXI: Expose Accessibility Tree children of the selected node

[James Craig]

<rdar://problem/16331398>

[Radar WebKit Bug Importer]

<rdar://problem/16331416>

[James Craig]

Created attachment 226813 [details]
partial patch (AX children functional; still needs work and UI tweaking)

[James Craig]

<rdar://problem/16331398>

[Joseph Pecoraro]

Comment on attachment 226813 [details]
partial patch (AX children functional; still needs work and UI tweaking)

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1427
> +    RefPtr<Inspector::TypeBuilder::Array<int>> axChildNodeIds = Inspector::TypeBuilder::Array<int>::create();

Since this actually allocates an array, lets only do it if we need to. You can declare the RefPtr here, but don't call Array<>::create yet.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1453
> +            for (const auto& axChildNode : axObject->children()) {

You can move the array creation right here above this for loop.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:283
> +            // FIXME: This UI is weird and should probably change.
> +            // The role reference looks like a tagname because it's...
> +            // ..sometimes unavailable and we have to show the tagname.
> +            // Perhaps just show the whole selector with :role(foo)?

That sounds fine.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:303
> +                    var childNodes = accessibilityProperties.axChildNodeIds;
> +                    for (var i = 0, c = childNodes.length; i < c; i++) {
> +                        var axChildNodeId = childNodes[i];

We use for..of loops in the inspector:
<https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of>

This code can be written as:

    for (var axChildNodeId of accessibilityProperties.axChildNodeIds) {
        ...
    }

This eliminates the need for variables "i" and "c".

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:314
> +                        for (var i = 0, c = axChildNodeLinks.length; i < c; i++) {

Likewise.

> Source/WebInspectorUI/UserInterface/Views/Main.css:263
> +    line-height: 1.2; /* [sic] unitless multiplier */

Why "[sic]"? Seems just the comment "unitless multiplier" would fine. Do you have a screenshot of what this UI looks like?

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode.html:135
> +        outerHTML = outerHTML.replace(/ class=\"ex\"/g, ""); // remove any duplicated, unnecessary class attributes

You can get rid of the escape sequences now in the regex. the \" can just be "

[James Craig]

Created attachment 227140 [details]
partial patch

[Timothy Hatcher]

Comment on attachment 227140 [details]
partial patch

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

> Source/WebCore/inspector/protocol/DOM.json:64
> +                { "name": "axChildNodeIds", "type": "array", "items": { "$ref": "NodeId" }, "optional": true, "description": "Array of <code>DOMNode</code> ids of the accessibility tree children if available." },
>                  { "name": "axParentNodeId", "$ref": "NodeId", "optional": true, "description": "<code>DOMNode</code> id of the accessibility tree parent object if available." },

"ax" prefix here seems a little redundant. It can be assume since they are in the "AccessibilityProperties" structure. The description explains them too.

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:73
> +WebInspector.linkifyAxNodeReference = function(node)

Lets spell out Accessibility.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:284
> +                var axChildNodeLinks = [];

Can we spell out accessibility? We do in other places.

> Source/WebInspectorUI/UserInterface/Views/Main.css:263
> +    line-height: 1.2; /* [sic] unitless multiplier */

Lets remove the comment.

[James Craig]

Comment on attachment 227140 [details]
partial patch

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

>> Source/WebCore/inspector/protocol/DOM.json:64
>>                  { "name": "axParentNodeId", "$ref": "NodeId", "optional": true, "description": "<code>DOMNode</code> id of the accessibility tree parent object if available." },
> 
> "ax" prefix here seems a little redundant. It can be assume since they are in the "AccessibilityProperties" structure. The description explains them too.

But these nodes have both child nodes in the DOM tree and child nodes in the AX tree. Given that "childNode" means "DOM childNode" everywhere else in the Inspector and inspector-protocol, I wanted the property name to be more explicit about this distinction.

>> Source/WebInspectorUI/UserInterface/Views/Main.css:263
>> +    line-height: 1.2; /* [sic] unitless multiplier */
> 
> Lets remove the comment.

I can remove it, but WebKit engineers misuse line-height all over the place, including using pixel units, even in the Web Inspector. It's so much harder to fix it after it's been done incorrectly, so I've gotten in the habit of using that comment everywhere I use line-height so well-intentioned but misguided engineers don't come behind me and break it.

[James Craig]

Comment on attachment 227140 [details]
partial patch

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

>>> Source/WebInspectorUI/UserInterface/Views/Main.css:263
>>> +    line-height: 1.2; /* [sic] unitless multiplier */
>> 
>> Lets remove the comment.
> 
> I can remove it, but WebKit engineers misuse line-height all over the place, including using pixel units, even in the Web Inspector. It's so much harder to fix it after it's been done incorrectly, so I've gotten in the habit of using that comment everywhere I use line-height so well-intentioned but misguided engineers don't come behind me and break it.

I haven't had a lot of sleep lately. Sorry for sounding like a pompous jackass in that last comment. ;-)

[James Craig]

Created attachment 227393 [details]
screen shot

(In reply to comment #5)
> Do you have a screenshot of what this UI looks like?

Attaching screen shot showing current UI. (Mouse is not visible, but I was hovering over one of the listitem children.)

[James Craig]

Created attachment 227400 [details]
patch with review feedback

I the left "ax" prefix in the protocol for protocol clarity. Since the DOM children and AX children can be different, I wanted the protocol to be very clear that this was NOT referencing DOM children. If you feel really strongly about this, I'd prefer to change it as a second bug, since the axParentNodeId and axChildNodeIds should be changed at the same time.

I expanded "ax" to "accessibility" as requested in the JavaScript files, which feels overly verbose to me in that scenario. Are you worried about "AX" being unclear? I think it's a pretty well-known prefix for us. After forking WebKit, Blink engineers even gutted most of the long "accessibilityFoo" names in WebCore in favor of "axFoo" to reduce verbosity.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/inspector/InspectorDOMAgent.h:235:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:81:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 2 in 12 files


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

[James Craig]

Created attachment 227402 [details]
patch

See previous comments re: "ax" prefix.

[Build Bot]

Comment on attachment 227402 [details]
patch

Attachment 227402 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5675638656598016

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html

[Build Bot]

Created attachment 227409 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Timothy Hatcher]

(In reply to comment #8)
> (From update of attachment 227140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227140&action=review
> 
> >> Source/WebCore/inspector/protocol/DOM.json:64
> >>                  { "name": "axParentNodeId", "$ref": "NodeId", "optional": true, "description": "<code>DOMNode</code> id of the accessibility tree parent object if available." },
> > 
> > "ax" prefix here seems a little redundant. It can be assume since they are in the "AccessibilityProperties" structure. The description explains them too.
> 
> But these nodes have both child nodes in the DOM tree and child nodes in the AX tree. Given that "childNode" means "DOM childNode" everywhere else in the Inspector and inspector-protocol, I wanted the property name to be more explicit about this distinction.

I don't think it will be confusing in the code, given that we are accessing these properties on an object with accessibility or ax in the variable name. Lets drop the ax prefixes in another patch.

> >> Source/WebInspectorUI/UserInterface/Views/Main.css:263
> >> +    line-height: 1.2; /* [sic] unitless multiplier */
> > 
> > Lets remove the comment.
> 
> I can remove it, but WebKit engineers misuse line-height all over the place, including using pixel units, even in the Web Inspector. It's so much harder to fix it after it's been done incorrectly, so I've gotten in the habit of using that comment everywhere I use line-height so well-intentioned but misguided engineers don't come behind me and break it.

Using length units is still valid and has its purposes. I use pixels mostly to be very explicit with how high I want the line to be. Otherwise I use em, but those em cases probably should be unitless.

[Timothy Hatcher]

(In reply to comment #11)
> Created an attachment (id=227400) [details]
> patch with review feedback
> 
> I the left "ax" prefix in the protocol for protocol clarity. Since the DOM children and AX children can be different, I wanted the protocol to be very clear that this was NOT referencing DOM children. If you feel really strongly about this, I'd prefer to change it as a second bug, since the axParentNodeId and axChildNodeIds should be changed at the same time.
> 
> I expanded "ax" to "accessibility" as requested in the JavaScript files, which feels overly verbose to me in that scenario. Are you worried about "AX" being unclear? I think it's a pretty well-known prefix for us. After forking WebKit, Blink engineers even gutted most of the long "accessibilityFoo" names in WebCore in favor of "axFoo" to reduce verbosity.

The Blink argument didn't help your case. :) Google loves terse names and wild abbreviations. We fought with them for years over this in WebKit.

Many of the cases you are prefixing with ax or accessibility likely can just drop the prefix altogether. Especially code that is already localized in a function or class dealing with AccessibilityProperties or accessibility things.

[James Craig]

(In reply to comment #16)

> Using length units is still valid and has its purposes. I use pixels mostly to be very explicit with how high I want the line to be. Otherwise I use em, but those em cases probably should be unitless.

It is technically valid, but the only time it's not problematic is if you're in a context where you can't resize your fonts (the current version of the Web Inspector is one of these contexts) or if you only render elements with no text (such as icons), and even then the icons would be better as resizable SVG icons sized in ems or rems. IndieUI and CSS WGs are currently discussing a "user-font-size" media feature for use with responsive layouts, which could allow large raster icons based on font-size in addition to SVG.

One of the (long-term) goals of the Web Inspector could be to have an interface where the text is resizable. A line-height defined in pixels will prevent this from working as intended. You can resize your fonts in Xcode, and other IDEs on OS X; Web developers should be able to do this in the Inspector, too.

[James Craig]

Bug 130598: Web Inspector: AXI: drop ax prefix from parentNodeId and childNodeIds in the protocol

[James Craig]

*** Bug 130598 has been marked as a duplicate of this bug. ***

[James Craig]

After touching all the rest for this patch, I thought it was kind of silly to have a separate patch for the protocol. Doing it here with the rest of the suggested changes.

[James Craig]

Created attachment 227461 [details]
patch with review feedback

The previous test failure seems unrelated.

[Timothy Hatcher]

(In reply to comment #18)
> (In reply to comment #16)
> 
> One of the (long-term) goals of the Web Inspector could be to have an interface where the text is resizable. A line-height defined in pixels will prevent this from working as intended. You can resize your fonts in Xcode, and other IDEs on OS X; Web developers should be able to do this in the Inspector, too.

We do plan to support resizing the fonts in the text editor, but making the font in the UI resizable is a non-goal, unless the system as a whole starts doing it. We don't currently use line-height in the text editor styles.

[WebKit Commit Bot]

Comment on attachment 227461 [details]
patch with review feedback

Clearing flags on attachment: 227461

Committed r166087: <http://trac.webkit.org/changeset/166087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.