Originally posted by:captainbrosset see https://github.com/devtools-html/gecko-dev/issues/322 Similar to #321 : STR: - Open the console - Enter `document.body` Expected: next to the HTMLBodyElement output, there should be a little inspector icon. Hovering over this icon should display a tooltip: "Click to select the node in the inspector". Clicking on this icon should switch to the inspector panel and select the body element. Actual: nothing happens.

Comment on attachment 8846620 [details] Bug 1307940 - Add mochitest to test open-in-inspector icon in the console. https://reviewboard.mozilla.org/r/119638/#review121880 ::: devtools/client/webconsole/new-console-output/test/mochitest/browser.ini:41 (Diff revision 1) > [browser_webconsole_keyboard_accessibility.js] > [browser_webconsole_location_debugger_link.js] > [browser_webconsole_location_scratchpad_link.js] > [browser_webconsole_location_styleeditor_link.js] > [browser_webconsole_nodes_highlight.js] > +[browser_webconsole_nodes_select.js] Test file is missing, can you add it ?

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review121712 Works great Nicolas, thanks! Some nits + one issue I'd like to have your feedback on. Clearing review in the meantime. ::: devtools/client/themes/webconsole.css:807 (Diff revision 1) > > +/* > + * Open DOMNode in inspector button. Style need to be reset in the new > + * console since its style is already defined in reps.css . > + */ > +.webconsole-output-wrapper .open-inspector { I think it would be nice to move this right after the initial rules for .open-inspector (L598-603) ::: devtools/client/webconsole/new-console-output/components/grip-message-body.js:79 (Diff revision 1) > - mode: props.mode, > + mode: props.mode, > - }) > - ); > + useQuotes: typeof grip !== "string", > + style: styleObject, > + }; > + > + return Rep(repConfig); That's unrelated to your patch, but I guess we are passing unexpected props to specialized reps components. onDOMNodeMouse*, for instance, are only used for some Reps. ::: devtools/client/webconsole/new-console-output/components/message-container.js:78 (Diff revision 1) > const tableDataChanged = this.props.tableData !== nextProps.tableData; > const responseChanged = this.props.message.response !== nextProps.message.response; > const totalTimeChanged = this.props.message.totalTime !== nextProps.message.totalTime; > - return repeatChanged || openChanged || tableDataChanged || responseChanged || > - totalTimeChanged; > + > + // We need to update the message to show the inspect icon for node grips if : > + // - There are at least one node grip in the message nit: are -> is ::: devtools/client/webconsole/new-console-output/components/message-container.js:87 (Diff revision 1) > + const nextNodeGrips = getNodeGripsFromMessage(nextProps.message); > + const nextNodeFronts = nextProps.nodeFronts; > + > + const nodeFrontsChanged = ( > + nextNodeGrips.length > 0 > + && this.props.nodeFronts.count() !== nextNodeFronts.count() Couldn't we have a case where the count is the same because N nodes were added and N nodes were removed, but we should still update? ::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:121 (Diff revision 1) > + let onSelectInspector = this.toolbox.selectTool("inspector"); > + let onGripNodeToFront = gripToNodeFront(grip); > + > + return Promise.all([onGripNodeToFront, onSelectInspector]) > + .then(([front]) => { > + let onInspectorUpdated = new Promise(resolve => { I think we can replace this with let onInspectorUpdate = this.toolbox.inspector.once("inspector-updated"); ::: devtools/client/webconsole/new-console-output/selectors/messages.js:63 (Diff revision 1) > > function getCurrentGroup(state) { > return state.messages.currentGroup; > } > > +function getAllMessagesNodeFrontsByActorId(state) { I'm a bit confused with the naming of functions in this selector, but looking at the other getAll* methods here, this one should be named getAllNodesFrontsByActorId ::: devtools/client/webconsole/new-console-output/selectors/messages.js:186 (Diff revision 1) > > return messages; > } > > -exports.getAllMessages = getAllMessages; > -exports.getAllMessagesUiById = getAllMessagesUiById; > +module.exports = { > + getAllMessages: getAllMessages, Can we replace "getAllMessages: getAllMessages" by "getAllMessages" and so on? ::: devtools/client/webconsole/new-console-output/utils/messages.js:331 (Diff revision 1) > + getAttachedNodeFrontsFromGrip, > + getNodeGripsFromMessage, > + isGroupType, > + l10n, > + // Export for use in testing. > + getRepeatId: getRepeatId, "getRepeatId: getRepeatId" -> "getRepeatId"

Comment on attachment 8846621 [details] Bug 1307940 - Add mocha tests to test logged node grips handling; https://reviewboard.mozilla.org/r/119640/#review121986 Looks great, thanks!

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review121712 > I think it would be nice to move this right after the initial rules for .open-inspector (L598-603) I won't be opposed to this, but early on this refactor, we chose to have the the specific rules for the new console at the bottom of the file, so we can clean up more easily when we ditch the old console. I'd like to keep it that way until we actually enable this on every channel > That's unrelated to your patch, but I guess we are passing unexpected props to specialized reps components. onDOMNodeMouse*, for instance, are only used for some Reps. That is true, at some point I was removing them in Rep utils function, but 2 things bothered me : - it added an overhead to the project (the `removedUnusedProps` function would to be call on all the Reps) - I couldn't find any thing stating that it was a bad thing to pass unused props to a React component. I discussed it briefly with Jason that didn't find it problematic either. Do you think it has a negative impact, can create bugs or something ? > Couldn't we have a case where the count is the same because N nodes were added and N nodes were removed, but we should still update? I don't think so, we never remove nodeFronts (unless we remove the message itself) > Can we replace "getAllMessages: getAllMessages" by "getAllMessages" and so on? yep, looks better

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review121712 > I think we can replace this with > > let onInspectorUpdate = this.toolbox.inspector.once("inspector-updated"); I can't get it to work like that. The function called (to my surprise) is http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/event/core.js#69 , which does not return a promise (and throws an error because the listener is null). I need to switch to use the inspector I get when doing `toolbox.selectTool("inspector")`, on which once returns a promise.

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review122406 ::: devtools/client/webconsole/new-console-output/components/message-container.js:47 (Diff revision 2) > serviceContainer: PropTypes.object.isRequired, > autoscroll: PropTypes.bool.isRequired, > indent: PropTypes.number.isRequired, > tableData: PropTypes.object, > + dispatch: PropTypes.func.isRequired, > + nodeFronts: PropTypes.array.isRequired, getting a warning with a debug build here: Warning: Failed prop type: Invalid prop `nodeFronts` of type `object` supplied to `MessageContainer`, expected `array`. in MessageContainer (created by ConsoleOutput) in ConsoleOutput (created by Connect(ConsoleOutput)) in Connect(ConsoleOutput) in div in Provider I guess nodeFronts is an immutable map? Same issue in console-output.js

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review121712 > I won't be opposed to this, but early on this refactor, we chose to have the the specific rules for the new console at the bottom of the file, so we can clean up more easily when we ditch the old console. I'd like to keep it that way until we actually enable this on every channel That's fine! > That is true, at some point I was removing them in Rep utils function, but 2 things bothered me : > - it added an overhead to the project (the `removedUnusedProps` function would to be call on all the Reps) > - I couldn't find any thing stating that it was a bad thing to pass unused props to a React component. I discussed it briefly with Jason that didn't find it problematic either. > > Do you think it has a negative impact, can create bugs or something ? I thought this could lead to warnings for React in dev mode, but I was wrong! > I can't get it to work like that. The function called (to my surprise) is http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/event/core.js#69 , which does not return a promise (and throws an error because the listener is null). > I need to switch to use the inspector I get when doing `toolbox.selectTool("inspector")`, on which once returns a promise. Oh right, toolbox.inspector is the inspector front. Sad :(

Comment on attachment 8846620 [details] Bug 1307940 - Add mochitest to test open-in-inspector icon in the console. https://reviewboard.mozilla.org/r/119638/#review122452

Please, don't mind the old reviews, we do not need all the work we were doing since the icon is handled directly by the Reps (shown when grip.preview.isConnected is true).

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review135454 This looks good to me. See the comments inline and also one UX note (that I assume is a change for reps). If I log document.body then move my mouse slowly from the tag name over to the icon then keep going to the right, the element stays highlighted in the page (even after my mouse leaves the inspect icon). Let me know if you can't reproduce that and I can show a screencast. ::: devtools/client/themes/webconsole.css:803 (Diff revision 3) > .message.startGroup .icon, > .message.startGroupCollapsed .icon { > display: none; > } > > +/* Why can't we use the reps style instead? Or is this unsetting properties set by webconsole.css? ::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:105 (Diff revision 3) > return this.toolbox && this.toolbox.highlighterUtils > ? this.toolbox.highlighterUtils.unhighlight(forceHide) > : null; > }, > + openNodeInInspector: async (grip) => { > + if (!this.toolbox) { Ideally if there was no toolbox we won't show the icon at all. Doesn't hurt to have the check here but maybe we can also not set onInspectIconClick in that case. Or is serviceContainer already implying we have a toolbox (i.e. not browser console)?

Comment on attachment 8846620 [details] Bug 1307940 - Add mochitest to test open-in-inspector icon in the console. https://reviewboard.mozilla.org/r/119638/#review135460 ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_nodes_select.js:50 (Diff revision 3) > + "inspector to be selected"); > + let onInspectorSelected = toolbox.once("inspector-selected"); > + let onInspectorUpdated = inspector.once("inspector-updated"); > + let onNewNode = toolbox.selection.once("new-node-front"); > + > + EventUtils.synthesizeMouseAtCenter( I've been using openInInspectorIcon.click() instead of EventUtils because it's shorter and IIRC it better handles nodes scrolled out of the viewport.

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review135636 ::: devtools/client/themes/webconsole.css:803 (Diff revision 3) > .message.startGroup .icon, > .message.startGroupCollapsed .icon { > display: none; > } > > +/* Yes, we already have a rule for the class in the file, for the old frontend : http://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#598

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review135454 > Why can't we use the reps style instead? Or is this unsetting properties set by webconsole.css? Yes, we already have a rule for the class in the file, for the old frontend : http://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#598

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review135454 > Ideally if there was no toolbox we won't show the icon at all. Doesn't hurt to have the check here but maybe we can also not set onInspectIconClick in that case. Or is serviceContainer already implying we have a toolbox (i.e. not browser console)? No, we always set serviceContainer, with or without toolbox, I'll do the check

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review135454 I saw that a couple of times, even in the inspector, and I think this is related to how the highlighter utils work. We can see http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-highlighter-utils.js#224-256 that highlightDomValueGrip is async, and it might occurs that you mouseleave before the highlighter is set, which causes the highlighter to stay highlighted. However, I can't reproduce it easily on my machine, maybe the bug you're seeing is not caused by the highlighter ?

Comment on attachment 8846619 [details] Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector; https://reviewboard.mozilla.org/r/119636/#review135454 I filed https://bugzil.la/1358983 for investigating that bug. I will investigate also on the Reps side, since I see a flicker of the higlighter when going from the tagname to the icon

Comment on attachment 8860847 [details] Bug 1307940 - Add toolbox-dependent method to serviceContainer only if toolbox is available. https://reviewboard.mozilla.org/r/132840/#review135838

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d366721c625a Add icon next to ElementNodeRep to select node in inspector; r=bgrins,jdescottes https://hg.mozilla.org/integration/autoland/rev/417076404106 Add mochitest to test open-in-inspector icon in the console. r=bgrins,jdescottes https://hg.mozilla.org/integration/autoland/rev/84368a2785c2 Add toolbox-dependent method to serviceContainer only if toolbox is available. r=bgrins

https://hg.mozilla.org/mozilla-central/rev/d366721c625a https://hg.mozilla.org/mozilla-central/rev/417076404106 https://hg.mozilla.org/mozilla-central/rev/84368a2785c2

Managed to reproduce the issue on 55.0a1 (2017-03-13). I can confirm it is verified fixed on 55.0a1 (2017-04-26), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.