Bug 158881 – Web Inspector: Implement Copy CSS Selector and Copy Xpath Selector context menus

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

[Nikita Vasilyev]

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

[Joseph Pecoraro]

I have this implemented, just writing tests.

[Joseph Pecoraro]

Created attachment 289095 [details]
[PATCH] Proposed Fix

[Matt Baker]

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`.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/206059>