Bug 145658 – Web Inspector: Hovering over CSS selectors in Styles panel show highlight matching elements on the web page

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 254300 [details]
[Video] Firefox DevTools: Highlight matching elements

Firefox DevTools' have an icon right next to CSS selectors to highlight all matching elements.

I think Web Inspector should do it too, but instead of adding the icon, we should do the highlighting when text cursor is on the selector.

[Radar WebKit Bug Importer]

<rdar://problem/21248532>

[Nikita Vasilyev]

Devin, take a look if you’re interested in this feature. If not, feel free to change it back to unassigned.

[Devin Rousso]

Created attachment 254809 [details]
Patch

[Nikita Vasilyev]

Created attachment 254818 [details]
[Video] After the patch applied

[Nikita Vasilyev]

First of all, this is really cool!

Nitpick:

[Nikita Vasilyev]

(accidentally sent my comment unfinished)

I think it would be better to highlight on mouse over/out of the entire CSS selector, not just the icon.

I’m not a reviewer yet and I’m not familiar about the backend part, so I cannot review that.

[Joseph Pecoraro]

Comment on attachment 254809 [details]
Patch

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

We should probably look into testing this.

Take a look at:
LayoutTests/inspector/dom/highlight-shape-outside.html

And we can chat about how to write a test for this.

> Source/JavaScriptCore/inspector/protocol/DOM.json:334
> +            "name": "highlightSelectorAll",

We may want to just name this highlightSelector. The all will be implied here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1084
> +    RefPtr<NodeList> nodes = m_document->querySelectorAll(selectors, ec);

There are some limitations to this implementation.

    • a selector "a:visited" will match elements not returned by document.querySelectorAll("a:visited")
        - this is a privacy requirement. Instead of using querySelectorAll we should be able to  extend this to the SelectorMatcher machinery. Probably in a separate patch.

    • document requires that you run the selector in the correct frame. Is this the main frame? A sub-frame?
        - for instance, if you are inspecting a Node in an <iframe> in the DOMTree, and then hover a selector "div" it will highlight the divs in the main frame, instead of the subframe
        - we are assuming the main frame. We may want to include a "frameId" in the protocol method and pass the frameId matching the selected DOMNode in the DOMTree.

These cases would both benefit greatly from tests. But can also be FIXMEs to improve later.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1094
> +    m_overlay->highlightMultipleNodes(nodes, *highlightConfig);

Lets rename this. "MultipleNodes" => "NodeList".

> Source/WebCore/inspector/InspectorOverlay.cpp:255
> +    m_highlightNodeList = nodes;

I wonder if highlighting a node | nodeList should be exclusive. If so we should clear the other when setting one of them.

What do you think? Is there ever a time where we would want to highlight both an individual node and a list of nodes?

> Source/WebCore/inspector/InspectorOverlay.cpp:259
> +
> +

Style: Too many newlines!

> Source/WebCore/inspector/InspectorOverlay.cpp:805
> +    if (m_highlightNode || m_highlightNodeList) {

If we made them exclusive, then we would want to change the logic below.

> Source/WebCore/inspector/InspectorOverlay.h:78
>  enum class HighlightType {
>      Node, // Provides 4 quads: margin, border, padding, content.
> +    NodeList, // Provides a list of nodes
>      Rects, // Provides a list of quads.
>  };

This reminds me. This won't yet work on iOS because it does its highlighting in a different way. Would you be interested in implementing this for iOS?

See:
Source/WebKit2/UIProcess/WKInspectorHighlightView.mm

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:308
> +        if (this._style.ownerRule)
> +            DOMAgent.highlightSelectorAll.invoke({selectors: this._style.ownerRule.selectorText, highlightConfig});
> +        else
> +            DOMAgent.highlightNode.invoke({nodeId: this._style.node.id, highlightConfig});

When inspecting an iOS 8 backend, the backend will not support highlightSelectorAll. This would be a runtime exception trying to call DOMAgent.highlightSelectorAll and it doesn't exist.

We should only trigger this if the backend supports it. Also we should avoid using "invoke" unless it is strictly necessary. Here it actually is necessary, but only because of iOS 6. We should add a comment about that. So I'd suggest:

    if (!this._style.ownerRule) {
      // COMPATIBILITY (iOS 6): Order of parameters changed in iOS 7.
      DOMAgent.highlightNode.invoke(...);
      return;
    }

    if (DOMAgent.highlightSelectorAll)
        DOMAgent.highlightSelectorAll(...) // NOTE: no need for invoke here.

[Nikita Vasilyev]

Comment on attachment 254809 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlayPage.js:500
> +        for (var i = 0; i < highlight.fragments.length; ++i)

for (var fragment of highlight.fragments)
?

> Source/WebCore/inspector/InspectorOverlayPage.js:512
> +            for (var i = 0; i < highlight.fragments.length; ++i)

for (var fragment of highlight.fragments)
?

[Devin Rousso]

Created attachment 255034 [details]
Patch

[Darin Adler]

Comment on attachment 255034 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.h:146
> +    virtual void highlightSelector(ErrorString&, const String& frameId, const RefPtr<Inspector::InspectorObject>&& highlightConfig, const String& selectors) override;

What’s the deal with the type const RefPtr<Inspector::InspectorObject>&&? That type doesn’t make sense; can’t be used to pass ownership. Should just be InspectorObject&. I see it used a lot here. Is this some crazy problem with our bindings generator?

[Joseph Pecoraro]

(In reply to comment #10)
> Comment on attachment 255034 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255034&action=review
> 
> > Source/WebCore/inspector/InspectorDOMAgent.h:146
> > +    virtual void highlightSelector(ErrorString&, const String& frameId, const RefPtr<Inspector::InspectorObject>&& highlightConfig, const String& selectors) override;
> 
> What’s the deal with the type const RefPtr<Inspector::InspectorObject>&&?
> That type doesn’t make sense; can’t be used to pass ownership. Should just
> be InspectorObject&. I see it used a lot here. Is this some crazy problem
> with our bindings generator?

This is a generated signature from the inspector code generator. It seems that can be improved in this case.

[Brian Burg]

Comment on attachment 255034 [details]
Patch

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

Overall looks pretty good! I don't have an updated checkout, would prefer if another reviewer could do some manual testing (or I'll do it later). This is a great page to test regions etc. http://webplatform.adobe.com/Demo-for-National-Geographic-Forest-Giant/browser/src/

I am curious about what will happen when highlighting selectors that aren't actually live in any stylesheet. For example, a selector could be inside of a media query, should it actually be highlighted? A way to get a "search list" of which nodes matched (or indication that nothing matched) would be cool for another patch.

> Source/WebCore/inspector/InspectorController.cpp:327
> +RefPtr<Inspector::InspectorArray> InspectorController::buildObjectForHighlightedNode() const

why the change to untyped objects?

RefPtr<Inspector::Protocol::Array<Inspector::Protocol::OverlayTypes::NodeHighlightData>>

> Source/WebCore/inspector/InspectorController.h:117
> +    WEBCORE_EXPORT RefPtr<Inspector::InspectorArray> buildObjectForHighlightedNode() const;

as above

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1094
> +    RefPtr<NodeList> nodes = document->querySelectorAll(selectors, ec);

should the string be called selectorList or selectorsString? Without some suffix to hint at the type, I was assuming the variable was an array.

>>> Source/WebCore/inspector/InspectorDOMAgent.h:146
>>> +    virtual void highlightSelector(ErrorString&, const String& frameId, const RefPtr<Inspector::InspectorObject>&& highlightConfig, const String& selectors) override;
>> 
>> What’s the deal with the type const RefPtr<Inspector::InspectorObject>&&? That type doesn’t make sense; can’t be used to pass ownership. Should just be InspectorObject&. I see it used a lot here. Is this some crazy problem with our bindings generator?
> 
> This is a generated signature from the inspector code generator. It seems that can be improved in this case.

Filed a cleanup bug: https://bugs.webkit.org/show_bug.cgi?id=146091

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:80
> +        <iframe class="class-one" src="data:text/html;charset=utf-8,%3Cbody%3E%3Cdiv%20id=%22id-one%22%3E%3C/div%3E%3Cdiv%3E%3C/div%3E%3C/body%3E"></iframe>

This is weird, do other tests do this? I think it might be better to set innerHTML inside runTest(), or use a helper .html page.

[Joseph Pecoraro]

Comment on attachment 255034 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/DOM.json:338
> +                { "name": "frameId", "type": "string", "description": "Identifier of the frame which will be searched using the selector." },
> +                { "name": "highlightConfig", "$ref": "HighlightConfig", "description": "A descriptor for the highlight appearance." },
> +                { "name": "selectors", "type": "string", "description": "A CSS selector for finding matching nodes to highlight." }

I feel like a better order might be: highlightConfig, selectors, frameId. You could imagine frameId would be optional, and assume the main frame, but we can keep it required.

>> Source/WebCore/inspector/InspectorController.cpp:327
>> -RefPtr<Inspector::Protocol::OverlayTypes::NodeHighlightData> InspectorController::buildObjectForHighlightedNode() const
>> +RefPtr<Inspector::InspectorArray> InspectorController::buildObjectForHighlightedNode() const
> 
> why the change to untyped objects?
> 
> RefPtr<Inspector::Protocol::Array<Inspector::Protocol::OverlayTypes::NodeHighlightData>>

Thought technically true, a better signature would be:

    Ref<Inspector::Protocol::Array<Inspector::Protocol::OverlayTypes::NodeHighlightData>> buildObjectForHighlightedNodes() const

Usage  of this array is very similar to InspectorArray but it type checks what you put into it. Also, Ref instead of RefPtr clarifies ownership, here we just have one that we pass around.

    auto highlights = Inspector::Protocol::Array<Inspector::Protocol::OverlayTypes::NodeHighlightData>::create();
    ...
    highlights->addItem(nodeHighlightData);
    ....
    return WTF::move(highlights);

Also this may be returning list, so the name suggestion "ForHighlightedNodes" is clearer at the call site.

> Source/WebCore/inspector/InspectorOverlay.h:77
> +    NodeList, // Provides a list of nodes

Style: Comment should end in a period.

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:31
> +    var testcases = [
> +    {
> +        'selector' : 'div',
> +        'frameId' : '0.1'
> +    },
> +    {
> +        'selector' : '.class-one',
> +        'frameId' : '0.1'
> +    },
> +    {
> +        'selector' : '#id-one',
> +        'frameId' : '0.1'
> +    },
> +    {
> +        'selector' : 'iframe.class-one',
> +        'frameId' : '0.1'
> +    },
> +    {
> +        'selector' : 'div',
> +        'frameId' : '0.3'
> +    },
> +    {
> +        'selector' : '#id-one',
> +        'frameId' : '0.3'
> +    },
> +    ];

This would read better for me compact:

    var testcases = [
        {frameId: "0.1", selector: "div"},
        {frameId: "0.1", selector: ".class-one"},
        {frameId: "0.1", selector: "#id-one"},
        // ...
    };

It would be nice to include a non-simple selector, such as:

    "div, p"

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:62
> +            InspectorTest.log("SELECTOR: " + testcase.selector);

Nit: "\nSELECTOR:" would produce slightly nicer output.

[Devin Rousso]

Created attachment 255205 [details]
Patch

[Brian Burg]

Comment on attachment 255205 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        * inspector/InspectorOverlayPage.js: Now expects an array as its parameter and loops through it to highlight each node given.  If the parameter array contains more than one element, do not draw the textbox containing info on that node.

usually we try to hard wrap lines at a reasonable column number, like 100 or something. Adding a blank line after a big comment also improves readability.

[Joseph Pecoraro]

Comment on attachment 255205 [details]
Patch

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

r=me, some nits to improve the patch just a bit but this looks good to me!

> Source/JavaScriptCore/inspector/protocol/DOM.json:338
> +                { "name": "frameId", "type": "string", "optional": true, "description": "Identifier of the frame which will be searched using the selector." }

Since this is optional the description should clarify what action will be taken if this is absent.

    "... the selector. If not provided, the main frame will be searched."

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1099
> +    ExceptionCode ec = 0;
> +    RefPtr<NodeList> nodes = document->querySelectorAll(selectorString, ec);

Would be nice to file a bug and have a FIXME about the :visited case.

    // FIXME: <https://webkit.org/b/??????> Web Inspector: DOM.highlightSelector should work for "a:visited"

> Source/WebCore/testing/Internals.cpp:930
> +    auto object = document->page()->inspectorController().buildObjectForHighlightedNodes();
> +    return object->toJSONString();

Would be better to rename "object" to "array", or better yet just make this one line.

    return document->page()->inspectorController().buildObjectForHighlightedNodes()->toJSONString();

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:7
> +        {'frameId': '0.1', 'selector': 'div'},

We should test the error / edge cases as well. Things like an invalid selector / missing frame.

But at the very least it would be good to test a selector that matches no elements, like ".no-elements".

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:34
> +            InspectorTest.assert(!error, "When evaluating code, an exception was thrown:" + wasThrown);

The intent here is to assert on "!wasThrown".

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:46
> +            InspectorTest.log("\nSELECTOR: " + testcase.selector);
> +            InspectorTest.log(JSON.stringify(payload, null, 2));

Purely because the JSON payload is so big it would be nice to also output the size of the list:

    InspectorTest.log("\nSELECTOR: " + testcase.selector);
    InspectorTest.log("\nFOUND: " + payload.length);

[Devin Rousso]

Created attachment 255239 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 255239 [details]
Patch

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

Again, nearly flawless. Lets tweak just one last time for the test name and the tab changes.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:59
> +        this._codeMirror.enableCSSTabNewline();

Unrelated to this change.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:636
> +    CodeMirror.defineExtension("enableCSSTabNewline", function() {
> +        if (this.somethingSelected()) this.indentSelection("add");
> +        else this.execCommand("insertTab");
> +    });

Unrelated to this change.

> LayoutTests/ChangeLog:10
> +        * inspector/dom/highlight-multiple-shapes-expected.txt: Added.
> +        * inspector/dom/highlight-multiple-shapes-iframe.html: Added.
> +        * inspector/dom/highlight-multiple-shapes.html: Added.

Gosh, sorry I didn't notice this before. Lets rename this to match the protocol method you are testing.

    inspector/dom/highlightSelector.html

With that, the patch looks perfect!

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:12
> +        {'frameId': '0.2', 'selector': 'div'}, // Non-existing frame ID.

Since this expects to fail, you may want to do something like:

  {frameId: '0.2', selector: 'div', error: true},

> LayoutTests/inspector/dom/highlight-multiple-shapes.html:30
> +        function highlightSelectorCallback(error) {
> +            InspectorTest.assert(!error, "Error in callback for DOMAgent.highlightSelector: " + error);

And instead of having the output include an ASSERT here (which looks wrong) you can:

    if (errorExpected) {
        InspectorTest.assert(error);
        InspectorTest.log("PASS: Expected error: " + error);
        callback();
        return;
    }

    ... assert no error, normal path

[Devin Rousso]

Created attachment 255247 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 255247 [details]
Patch

r=me!

[WebKit Commit Bot]

Comment on attachment 255247 [details]
Patch

Clearing flags on attachment: 255247

Committed r185784: <http://trac.webkit.org/changeset/185784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.