Bug 129943 – Web Inspector: AXI: Expose Accessibility Tree parent of the selected node

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

[James Craig]

Web Inspector: AXI: Expose AX parent and children of the selected node

[James Craig]

Created attachment 226487 [details]
partial patch (AX parent working, no childnodes or test case yet)

[Joseph Pecoraro]

The approach looks good to me.

[James Craig]

Since the patch for AX Parent is starting to get larger than anticipated, and the approach for AX Children is undecided, I'm going to split these and just use this bug for the work on AX Parent.

[James Craig]

Created attachment 226567 [details]
patch v2

Version 2. Not sure I like how the axParentNodeId is exposed in the LayoutTest. I think it'll probably be better to result those to something more useful.

[James Craig]

s/result/resolve/

[James Craig]

Created attachment 226585 [details]
patch (need to resolve conflicts with TOT)

[James Craig]

Created attachment 226586 [details]
patch

[Joseph Pecoraro]

Comment on attachment 226586 [details]
patch

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

r=me

> Source/WebCore/inspector/protocol/DOM.json:63
> +                { "name": "axParentNodeId", "$ref": "NodeId", "optional": true, "description": "<code>DOMNode</code> of the accessibility tree parent object id if available." },

This reads weird. "of the AX tree parent object id". Maybe we can drop the "id" at the end?

> Source/WebInspectorUI/ChangeLog:15
> +        * UserInterface/Models/DOMNode.js: AX PArent Support and adding role to DOMNode (will be exposed as AX Parent link and in overlays).

Typo: "PArent"

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:59
> +    title += WebInspector.roleSelectorForNode(node);

Neat. A bit of a tough sell displaying CSS selectors that we don't support yet, but I think its fine.

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:83
> +WebInspector.linkifyNodeReference = function(node, optDisplayAsRole)
> +{
> +    var selector = WebInspector.displayNameForNode(node);
> +    var role = node.computedRole();
> +    var displayName = optDisplayAsRole && role ? role : selector;
>      var link = document.createElement("span");
>      link.appendChild(document.createTextNode(displayName));
> +    link.setAttribute("role", "link");
>      link.className = "node-link";
> -    link.title = displayName;
> +    link.title = selector;

I think the optDisplayAsRole can be done at the call site. I'm not sure it is needed on this utility function. This function returns the link, and we can change the link textContent as needed after it returns. What do you think?

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:284
> +                var axParentNode = "";
> +                if (accessibilityProperties.axParentNodeId !== undefined)
> +                    axParentNode = WebInspector.linkifyNodeReference(WebInspector.domTreeManager.nodeForId(accessibilityProperties.axParentNodeId), true);

How about naming this "axParentNodeLink". You can initialize it to null. Then down below you can do:

    ...Row.value = axParentNodeLink || "";

This way we won't ever mix types (string versus DOMNode) in the variable.

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode.html:125
> +        outerHTML = outerHTML.replace(";base64,R0lGODlhEAARAJECAOHh4UpKSgAAAAAAACH5BAEAAAIALAAAAAAQABEAAAIllB8Zx63b4otSUWcvyuz5D4biSD7AiZroWSXa5r7CJNOvra1RAQA7", "...")

Hehe, rather specific. Might be easier to use a regex:

    outerHTML = outerHTML.replace(/;base64,.*?"/, "\"");

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode.html:132
> +        for (key in response.result.properties) {

Oops. Missing a var here?

    for (var key in ...)

Otherwise "key" gets accidentally leaked into the global scope.

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode.html:136
> +            else if (key === "axParentNodeId")

No need for the "else" here, the previous "if" was a return/break/continue.

[Timothy Hatcher]

Comment on attachment 226586 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:59
>> +    title += WebInspector.roleSelectorForNode(node);
> 
> Neat. A bit of a tough sell displaying CSS selectors that we don't support yet, but I think its fine.

Yeah I am not sure about this. I think this will confuse developers and cause more truncation in the DOM navigation bar because of the longer strings. Lets revert this or make it optional with an argument for some cases.

>> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:83
>> +    link.title = selector;
> 
> I think the optDisplayAsRole can be done at the call site. I'm not sure it is needed on this utility function. This function returns the link, and we can change the link textContent as needed after it returns. What do you think?

I agree, it can be done after the link is made. (IIRC, we replace the text of the links in other places.)

Also optDisplayAsRole should not have the "opt" prefix or it should be spelled out. But we usually don't call out optional arguments.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:284
>> +                    axParentNode = WebInspector.linkifyNodeReference(WebInspector.domTreeManager.nodeForId(accessibilityProperties.axParentNodeId), true);
> 
> How about naming this "axParentNodeLink". You can initialize it to null. Then down below you can do:
> 
>     ...Row.value = axParentNodeLink || "";
> 
> This way we won't ever mix types (string versus DOMNode) in the variable.

Which the DFG JIT hates…

[James Craig]

Comment on attachment 226586 [details]
patch

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

>>> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:59
>>> +    title += WebInspector.roleSelectorForNode(node);
>> 
>> Neat. A bit of a tough sell displaying CSS selectors that we don't support yet, but I think its fine.
> 
> Yeah I am not sure about this. I think this will confuse developers and cause more truncation in the DOM navigation bar because of the longer strings. Lets revert this or make it optional with an argument for some cases.

I'll just modify the title along with the text content when I get the link back.

[James Craig]

Created attachment 226610 [details]
patch with review feedback

[WebKit Commit Bot]

Comment on attachment 226610 [details]
patch with review feedback

Rejecting attachment 226610 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 226610, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
at 327 (offset 19 lines).
Hunk #18 succeeded at 335 (offset 19 lines).
Hunk #19 succeeded at 344 (offset 19 lines).
4 out of 19 hunks FAILED -- saving rejects to file LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt.rej
patching file LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6532272299704320

[James Craig]

Ah right. Conflicts in the test expectation from bug 129779.

[James Craig]

Created attachment 226668 [details]
patch merges conflicts with TOT

[WebKit Commit Bot]

Comment on attachment 226668 [details]
patch merges conflicts with TOT

Clearing flags on attachment: 226668

Committed r165633: <http://trac.webkit.org/changeset/165633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

inspector-protocol/dom/getAccessibilityPropertiesForNode.html fails on Mavericks (at least):

http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r165633%20(4257)/inspector-protocol/dom/getAccessibilityPropertiesForNode-pretty-diff.html

[James Craig]

Bug 130259. Web Inspector: AXI: Suppress axParentNodeId entirely in getAccessibilityPropertiesForNode.html b/c the int value isn't consistent enough for a layout test.

[James Craig]

<rdar://problem/16322354>

[Jer Noble]

This test has been very flakey since it was added:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&revision=165633&tests=inspector-protocol%2Fdom%2FgetAccessibilityPropertiesForNode.html

[Jer Noble]

Once again, beat to the punch by Alexey. :)