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.
<rdar://problem/21248532>
Devin, take a look if you’re interested in this feature. If not, feel free to change it back to unassigned.
Created attachment 254809 [details] Patch
Created attachment 254818 [details] [Video] After the patch applied
First of all, this is really cool! Nitpick:
(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.
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.
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) ?
Created attachment 255034 [details] Patch
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?
(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.
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.
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.
Created attachment 255205 [details] Patch
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.
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);
Created attachment 255239 [details] Patch
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
Created attachment 255247 [details] Patch
Comment on attachment 255247 [details] Patch r=me!
Comment on attachment 255247 [details] Patch Clearing flags on attachment: 255247 Committed r185784: <http://trac.webkit.org/changeset/185784>
All reviewed patches have been landed. Closing bug.