1419083 - Add a "Open in sidebar" context menu entry on ObjectInspector

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

This should work like in Bug 1419081

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

At this point, we should also remove the button we introduced in Bug 1419075

Comment 2

[Mike Park [:mparkms]]

Attachment: Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review commit: https://reviewboard.mozilla.org/r/205566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205566/

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

https://reviewboard.mozilla.org/r/205566/#review211328

This looks good, thanks Mike.
I have some comments about this, but it's working quite well
I only found a minor issue when the object is expanded and you click not directly on text : http://prntscr.com/hjra7m 
it does not enable the context menu entry

::: devtools/client/locales/en-US/webconsole.properties:224
(Diff revision 1)
>  # LOCALIZATION NOTE (webconsole.menu.openInVarView.label)
>  # Label used for a context-menu item displayed for object/variable logs. Clicking on it
>  # opens the webconsole variable view for the logged variable.
> -webconsole.menu.openInVarView.label=Open in Variables View
> +webconsole.menu.openInVarView.label=Open in sidebar
>  webconsole.menu.openInVarView.accesskey=V

the comment needs to be updated to not mention the variable view.

In fact, this label is still used in the browser console and browser toolbox, so we should create a new one.

if we need to update the content of a label at some point, we also need to rename the label name so the l10n team knows something has changed, and can work on new translations

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:128
(Diff revision 1)
> +          let { parameters } = getMessage(store.getState(), messageId);
> +          if (Array.isArray(parameters)) {
> +            for (let parameter of parameters) {
> +              if (parameter.actor === actor) {
> +                store.dispatch(actions.showObjectInSidebar(parameter));
> +                return;
> +              }
> +            }

i think we should only dispatch the action with the id, and in the action, retrieve the expected parameter.
the action would need to be a thunk, so you get access to the state (like https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/actions/filters.js#41-42). this way it will be easier to test that it retrieves the expected grip

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:7
(Diff revision 1)
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Test that the sidebar is hidden when the console is cleared.
>  

we may not need to have this file copiedd from webconsole_close_sidebar, since they are not really related

also, can this file be renamed to browser_webconsole_context_menu_object_in_sidebar ?
all our test involving context menus are prefixed the same so they are easier to spot.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:51
(Diff revision 1)
> -  } else {
> -    clearShortcut = WCUL10n.getStr("webconsole.clear.key");
> -  }
> -  synthesizeKeyShortcut(clearShortcut);
> -  await waitFor(() => findMessages(hud, "").length == 0);
> -  sidebar = hud.ui.document.querySelector(".sidebar");
> +  isnot(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +        "Sidebar is updated for {b:1}");
> +  sidebarText = hud.ui.document.querySelector(".sidebar-contents").textContent;
> +  ok(sidebarText.includes('"b":'), "Sidebar contents shown for {b:1}");
> +
> +  info("Ctrl-clicking on number");

invalid comment now

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:52
(Diff revision 1)
> -  ok(!sidebar, "Sidebar hidden after console.clear()");
> +  await showSidebar(hud, number, false);
> +  ok(hud.ui.document.querySelector(".sidebar"),
> +     "Sidebar is still shown after clicking on number");
> +  is(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +     "Sidebar is not updated after clicking on number");
> +
> +  info("Ctrl-clicking on string");
> +  await showSidebar(hud, string, false);
> +  ok(hud.ui.document.querySelector(".sidebar"),
> +     "Sidebar is still shown after clicking on string");
> +  is(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +     "Sidebar is not updated after clicking on string");
> +
> +  info("Ctrl-clicking on bool");
> +  await showSidebar(hud, bool, false);
> +  ok(hud.ui.document.querySelector(".sidebar"),
> +     "Sidebar is still shown after clicking on bool");
> +  is(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +     "Sidebar is not updated after clicking on bool");
> +
> +  info("Ctrl-clicking on null message");
> +  await showSidebar(hud, nullMessage, false);
> +  ok(hud.ui.document.querySelector(".sidebar"),
> +     "Sidebar is still shown after clicking on null message");
> +  is(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +     "Sidebar is not updated after clicking on null message");
> +
> +  info("Ctrl-clicking on undefined message");
> +  await showSidebar(hud, undefinedMsg, false);
> +  ok(hud.ui.document.querySelector(".sidebar"),
> +     "Sidebar is still shown after clicking on undefined message");
> +  is(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +     "Sidebar is not updated after clicking on undefined message");

for all of those, we can simply check that the context menu entry is disabled, with a isContextMenuEntryEnabled function.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:87
(Diff revision 1)
> +     "Sidebar is still shown after clicking on undefined message");
> +  is(hud.ui.document.querySelector(".sidebar-contents").textContent, sidebarText,
> +     "Sidebar is not updated after clicking on undefined message");
>  });
>  
> -async function showSidebar(hud) {
> +async function showSidebar(hud, node, expectMutation) {

we could rename the function to something that better conveys what it does (opening the context menu, and clicking on the out object in sidebar entry)

Comment 4

[Mike Park [:mparkms]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> I only found a minor issue when the object is expanded and you click not
> directly on text : http://prntscr.com/hjra7m 
> it does not enable the context menu entry

Do we want the context menu entry to be enabled if you click on an element of the object? Like this: https://i.imgur.com/N64NaUk.png

What about the case of an object inside an object? If we right click on the sub-object, it should get the grip for that object, right? I'm not sure if that's possible the way we have it now, since that sub-object doesn't have a message of its own.

Thinking about it, I think the easiest way that would solve all these edge cases is if we attached a context menu handler to ObjectInspector. We'd need to go to the reps bundle, but if we attached it to the ObjectInspector, it could just pass the grip directly, avoiding the need to iterate through messages. What do you think?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

ideally we want the context menu entry to be available on the whole .object-inspector element, including if you click somewhere inside the rectangle without element underneath.

We don't have to deal with object inside objects i think, only top objects.

And yes, maybe having a context menu on the objectInspector would be better, but this might be complex. We may handle this as part of https://github.com/devtools-html/devtools-core/issues/834.

Comment 6

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

https://reviewboard.mozilla.org/r/205566/#review211328

> i think we should only dispatch the action with the id, and in the action, retrieve the expected parameter.
> the action would need to be a thunk, so you get access to the state (like https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/actions/filters.js#41-42). this way it will be easier to test that it retrieves the expected grip

Do we still want to do this, if we're going to attach the context menu handler to the ObjectInspector? If we do that, I was imagining we'd get the grip from the object inspector directly and dispatch that.

Comment 7

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205566/diff/1-2/

Comment 8

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205566/diff/2-3/

Comment 9

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205566/diff/3-4/

Comment 10

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205566/diff/4-5/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

https://reviewboard.mozilla.org/r/205566/#review213038

This is looking good to me. I have only a few minor comment. Let's push this to TRY and land it if everything is fine.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_object_in_sidebar.js:6
(Diff revision 5)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test that the sidebar is hidden when the console is cleared.

Oooopsie

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_object_in_sidebar.js:16
(Diff revision 5)
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["devtools.webconsole.sidebarToggle", true]
> +  ]});

can we have only  `pushPref("devtools.webconsole.sidebarToggle", true)` ?

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:37
(Diff revision 5)
> + *        - {Boolean} sidebarTogglePref (optional) whether the sidebar should be
> + *            visible or not

i would have say "coulc be visible" instead of "should"

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:177
(Diff revision 5)
>        selection.selectAllChildren(webconsoleOutput);
>      },
>    }));
>  
> +  // Open object in sidebar.
> +  if (sidebarTogglePref) {

we should check for `openSidebar` I think, and then we could only pass openSidebar in new-console-output wrapper if sidebarTogglePref is set to true (and not pass it to the context menu).

Comment 12

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205566/diff/5-6/

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1b66f892f6d9 -d c4087b735507: rebasing 439078:1b66f892f6d9 "Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector. r=nchevobbe" (tip)
merging devtools/client/themes/webconsole.css
merging devtools/client/webconsole/new-console-output/components/SideBar.js
merging devtools/client/webconsole/new-console-output/test/mochitest/browser.ini
warning: conflicts while merging devtools/client/themes/webconsole.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/new-console-output/components/SideBar.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Mike Park [:mparkms]]

Comment on attachment 8934679 [details]
Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205566/diff/6-7/

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f533989d8f01 -d 49f68b1f4c2e: rebasing 439077:f533989d8f01 "Bug 1419083 - Add an "Open in sidebar" context menu entry on ObjectInspector. r=nchevobbe" (tip)
merging devtools/client/themes/webconsole.css
merging devtools/client/webconsole/new-console-output/components/SideBar.js
merging devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js
warning: conflicts while merging devtools/client/themes/webconsole.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/client/webconsole/new-console-output/components/SideBar.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/971f3f353ca8
Add an "Open in sidebar" context menu entry on ObjectInspector. r=nchevobbe

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/971f3f353ca8

Comment 18

[Francesco Lodolo [:flod]]

Is there a reason to pick a character not available in the label? This will show up as "Open in sidebar (V)" in any platform but macOS.

Comment 19

[Mike Park [:mparkms]]

"V" was the shortcut for "Open in Variable View", which had the equivalent functionality in the old version of the console. But if it would be better to change it, we can do that. Nicolas, what do you think?

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Yeah, it could be nice if we could have "S" I suppose, but it is already taken for Store as global variable. Another option would have been "o", but this is also already taken. I think it's okay for now, it's the same key than the one that used to exist for the same functionality.