Created attachment 281573 [details] [Image] Chrome DevTools vs Web Inspector There should be a context menu in the DOM tree outline to copy a uniq CSS selector and Xpath. Chrome DevTools and Firefox DevTools both have this. rdar://problem/8181156
I have this implemented, just writing tests.
Created attachment 289095 [details] [PATCH] Proposed Fix
Comment on attachment 289095 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=289095&action=review Very nice tests! r=me with some nits. > LayoutTests/inspector/dom/domutilities-csspath.html:40 > + InspectorTest.expectEqual(WebInspector.cssPath(node), "#id-test", "Element with id should have simple selector '#outer'."); '#outer' -> '#id-test' > LayoutTests/inspector/dom/domutilities-csspath.html:145 > + documentNode = docNode; Nit: all other inspector tests use `documentNode` or `node` as the argument name here. I think it should just be `node`, rather than arbitrarily abbreviating just to make it unique. > LayoutTests/inspector/dom/domutilities-csspath.html:180 > + <div class="alpha"></div> Should we reduce the test DOM further? Some of the divs aren't necessary to test the UniqueTagName, NonUniqueTagName, and NonUniqueClassName cases. > LayoutTests/inspector/dom/domutilities-xpath.html:101 > + WebInspector.domTreeManager.requestDocument((docNode) => { Nit: `docNode` -> `node` > Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:141 > + return classAttribute ? classAttribute.trim().split(/\s+/) : []; This shares some logic with DOMNode.escapedClassSelector. It seems like this could be moved into DOMNode as a classNames getter, which DOMNode.escapedClassSelector could be implemented on top of. There are a handful of other places in the UI where we call DOMNode.gettAtribute passing "class", and do some work on the returned string. These would also benefit from a DOMNode.classNames getter. > Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:150 > + for (let i = 0; i < siblings.length; ++i) { This can just be a for...of since the counter isn't used in the body of the loop. > Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:289 > + for (let i = 0; i < siblings.length; ++i) { Nit: loop counter not used in body, change to for...of. > Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:290 > + if (isSimiliarNode(node, siblings[i])) { Nit: invert condition and `continue`.
Comment on attachment 289095 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=289095&action=review Thanks for the review. Good comments. >> LayoutTests/inspector/dom/domutilities-csspath.html:40 >> + InspectorTest.expectEqual(WebInspector.cssPath(node), "#id-test", "Element with id should have simple selector '#outer'."); > > '#outer' -> '#id-test' Good catch! >> LayoutTests/inspector/dom/domutilities-csspath.html:145 >> + documentNode = docNode; > > Nit: all other inspector tests use `documentNode` or `node` as the argument name here. I think it should just be `node`, rather than arbitrarily abbreviating just to make it unique. I like this suggestion! >> LayoutTests/inspector/dom/domutilities-csspath.html:180 >> + <div class="alpha"></div> > > Should we reduce the test DOM further? Some of the divs aren't necessary to test the UniqueTagName, NonUniqueTagName, and NonUniqueClassName cases. I think as long as its not going overboard the excess is fine. That said, it might be useful to test where something matches the last child and not the middle like I'm doing here. But the dump test kinda handles that. >> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:141 >> + return classAttribute ? classAttribute.trim().split(/\s+/) : []; > > This shares some logic with DOMNode.escapedClassSelector. It seems like this could be moved into DOMNode as a classNames getter, which DOMNode.escapedClassSelector could be implemented on top of. > > There are a handful of other places in the UI where we call DOMNode.gettAtribute passing "class", and do some work on the returned string. These would also benefit from a DOMNode.classNames getter. Good point, can be done in a follow-up.
<https://trac.webkit.org/changeset/206059>