[webkit-reviews] review granted: [Bug 129943] Web Inspector: AXI: Expose Accessibility Tree parent of the selected node : [Attachment 226586] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted James Craig
<jcraig at apple.com>'s request for review:
Bug 129943: Web Inspector: AXI: Expose Accessibility Tree parent of the
selected node
https://bugs.webkit.org/show_bug.cgi?id=129943

Attachment 226586: patch
https://bugs.webkit.org/attachment.cgi?id=226586&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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(accessi
bilityProperties.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,R0lGODlhEAARAJECAOHh4UpKSgAAAAAAACH5BAEAAAIALAAAAAAQ
ABEAAAIllB8Zx63b4otSUWcvyuz5D4biSD7AiZroWSXa5r7CJNOvra1RAQA7", "...")

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.