Web Inspector: AXI: Expose AX parent and children of the selected node
Created attachment 226487 [details] partial patch (AX parent working, no childnodes or test case yet)
The approach looks good to me.
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.
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.
s/result/resolve/
Created attachment 226585 [details] patch (need to resolve conflicts with TOT)
Created attachment 226586 [details] patch
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.
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…
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.
Created attachment 226610 [details] patch with review feedback
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
Ah right. Conflicts in the test expectation from bug 129779.
Created attachment 226668 [details] patch merges conflicts with TOT
Comment on attachment 226668 [details] patch merges conflicts with TOT Clearing flags on attachment: 226668 Committed r165633: <http://trac.webkit.org/changeset/165633>
All reviewed patches have been landed. Closing bug.
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
Bug 130259. Web Inspector: AXI: Suppress axParentNodeId entirely in getAccessibilityPropertiesForNode.html b/c the int value isn't consistent enough for a layout test.
<rdar://problem/16322354>
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
Once again, beat to the punch by Alexey. :)