Closed Bug 1307940 Opened 8 years ago Closed 7 years ago

There's no icon next to DOMNodes to select them in the inspector

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.4 - May 1
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: linclark, Assigned: nchevobbe)

References

Details

(Whiteboard: [console-html])

Attachments

(3 files, 1 obsolete file)

Bug 1307940 - Add icon next to ElementNodeRep to select node in inspector;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307940 - Add mochitest to test open-in-inspector icon in the console.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307940 - Add toolbox-dependent method to serviceContainer only if toolbox is available.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
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.
Priority: -- → P2
Whiteboard: new-console
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
Flags: qe-verify+
Whiteboard: new-console → [new-console]
QA Contact: iulia.cristescu
Iteration: --- → 53.5 - Jan 23
Priority: P2 → P1
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → P2
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 ?
Attachment #8846620 - Flags: review?(jdescottes)
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"
Attachment #8846619 - Flags: review?(jdescottes)
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!
Attachment #8846621 - Flags: review?(jdescottes) → review+
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
Attachment #8846619 - Flags: review?(jdescottes) → review+
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
Attachment #8846620 - Flags: review?(jdescottes) → review+
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
Depends on: 1357341
Whiteboard: [new-console] → [console-html]
Attachment #8846621 - Attachment is obsolete: true
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)?
Attachment #8846619 - Flags: review?(bgrinstead) → review+
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.
Attachment #8846620 - Flags: review?(bgrinstead) → review+
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
Attachment #8860847 - Flags: review?(bgrinstead) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: