1456852 - No context menu in the input field of the browser console with new frontend

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Oriol Brufau [:Oriol]]

1. Set devtools.chrome.enabled=true
2. Open browser console with new frontend
3. Right click the input area

Result: no context menu

Expected: context menu like this:

  ┌────────────┐
  │ Undo       │
  ├────────────┤
  │ Cut        │
  │ Copy       │
  │ Paste      │
  │ Delete     │
  ├────────────┤
  │ Select All │
  └────────────┘

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1456852 - Programatically create edit menu in the browser console input

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

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8990618 [details]
Bug 1456852 - Programatically create edit menu in the browser console input

https://reviewboard.mozilla.org/r/255676/#review262382


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/components/JSTerm.js:26
(Diff revision 1)
>  loader.lazyRequireGetter(this, "Telemetry", "devtools/client/shared/telemetry");
>  loader.lazyRequireGetter(this, "processScreenshot", "devtools/shared/webconsole/screenshot-helper");
>  
> +const Menu = require("devtools/client/framework/menu");
> +const MenuItem = require("devtools/client/framework/menu-item");
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

::: devtools/client/webconsole/components/JSTerm.js:1426
(Diff revision 1)
>            ref: node => {
>              this.inputNode = node;
>            },
>            onPaste: onPaste,
>            onDrop: onPaste,
> +          onContextMenu: function (e ) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

::: devtools/client/webconsole/components/JSTerm.js:1426
(Diff revision 1)
>            ref: node => {
>              this.inputNode = node;
>            },
>            onPaste: onPaste,
>            onDrop: onPaste,
> +          onContextMenu: function (e ) {

Error: There should be no spaces inside this paren. [eslint: space-in-parens]

::: devtools/client/webconsole/components/JSTerm.js:1427
(Diff revision 1)
>              this.inputNode = node;
>            },
>            onPaste: onPaste,
>            onDrop: onPaste,
> +          onContextMenu: function (e ) {
> +            var docshell = window.QueryInterface(Ci.nsIInterfaceRequestor)

Error: Unexpected var, use let or const instead. [eslint: mozilla/var-only-at-top-level]

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8990618 [details]
Bug 1456852 - Programatically create edit menu in the browser console input

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

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8990618 [details]
Bug 1456852 - Programatically create edit menu in the browser console input

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

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

https://reviewboard.mozilla.org/r/255674/#review263320

I have a few comment, but this already looks good

::: devtools/client/framework/menu.js:133
(Diff revision 4)
> -    if (item.submenu) {
> -      const menupopup = doc.createElementNS(XUL_NS, "menupopup");
> -      item.submenu._createMenuItems(menupopup);
> -
> -      const menu = doc.createElementNS(XUL_NS, "menu");
> +    function applyAttributesToNode(node) {
> +      if (item.l10nID) {
> +        node.setAttribute("data-l10n-id", item.l10nID);
> +      } else {
> +        node.setAttribute("label", item.label);
> -      menu.appendChild(menupopup);
> -      menu.setAttribute("label", item.label);
> -      if (item.disabled) {
> -        menu.setAttribute("disabled", "true");
> -      }
> -      if (item.accelerator) {
> +        if (item.accelerator) {
> -        menu.setAttribute("acceltext", item.accelerator);
> +          node.setAttribute("acceltext", item.accelerator);
> -      }
> +        }
> -      if (item.accesskey) {
> +        if (item.accesskey) {
> -        menu.setAttribute("accesskey", item.accesskey);
> +          node.setAttribute("accesskey", item.accesskey);
> +        }
> +      }
> +      if (item.type === "checkbox") {
> +        node.setAttribute("type", "checkbox");
> +      }
> +      if (item.type === "radio") {
> +        node.setAttribute("type", "radio");
> +      }
> +      if (item.disabled) {
> +        node.setAttribute("disabled", "true");
> +      }
> +      if (item.checked) {
> +        node.setAttribute("checked", "true");
>        }
>        if (item.id) {
> -        menu.id = item.id;
> +        node.id = item.id;
> +      }
> -      }
> +    }

do you think we could move this outside of createMenuItems so we don't create it each time, for each item in the menu ? It looks like it's easily doable by passing the item variable.

::: devtools/client/framework/test/browser_menu_api.js:108
(Diff revision 4)
>    ok(!menuItems[2].hasAttribute("checked"), "Doesn't have checked attr");
>  
>    is(menuItems[3].getAttribute("label"), MENU_ITEMS[3].label, "Correct label");
>    is(menuItems[3].getAttribute("disabled"), "true", "disabled attr menuitem");
>  
> +  is(menuItems[4].getAttribute("data-l10n-id"), MENU_ITEMS[4].l10nID, "Correct localization attribute");

could we also check textContent ?

::: devtools/client/webconsole/components/JSTerm.js:1332
(Diff revision 4)
> +    // The toolbox does it's own edit menu handling with
> +    // toolbox-textbox-context-popup and friends. For now, fall
> +    // back to use that if running inside the toolbox, but use our
> +    // own menu when running in the Browser Console.
> +    if (this.props.hud.isBrowserConsole &&
> +        Services.prefs.getBoolPref("devtools.browserconsole.html")) {

maybe add a comment with the bug number when we plan to make this the default ?

::: devtools/client/webconsole/components/JSTerm.js:1379
(Diff revision 4)
>        return dom.div({
>          className: "jsterm-input-container devtools-monospace",
>          key: "jsterm-container",
>          style: {direction: "ltr"},
>          "aria-live": "off",
> +        onContextMenu: this.onContextMenu.bind(this),

let's bind onContextMenu in the constructor, since we already do this for other JsTerm methods.

::: devtools/client/webconsole/index.html:12
(Diff revision 4)
> +    <link rel="localization" href="toolkit/main-window/editmenu.ftl"/>
> +    <link rel="stylesheet" href="chrome://global/skin/"/>
>      <link rel="stylesheet" href="chrome://devtools/skin/widgets.css"/>
>      <link rel="stylesheet" href="chrome://devtools/skin/webconsole.css"/>
>      <link rel="stylesheet" href="chrome://devtools/skin/components-frame.css"/>
>      <link rel="stylesheet" href="resource://devtools/client/shared/components/reps/reps.css"/>
>      <link rel="stylesheet" href="resource://devtools/client/shared/components/tabs/Tabs.css"/>
>      <link rel="stylesheet" href="resource://devtools/client/shared/components/tabs/TabBar.css"/>
>      <link rel="stylesheet" href="resource://devtools/client/shared/components/NotificationBox.css"/>
>      <link rel="stylesheet" href="chrome://devtools/content/netmonitor/src/assets/styles/httpi.css"/>
>  
> +    <script type="text/javascript" src="chrome://global/content/l10n.js"></script>

are those new resources needed only for the browser console ?
If yes, would there be a way to load them only if we're in the browser console instead ?

::: devtools/client/webconsole/test/mochitest/browser_console_context_menu_entries.js:14
(Diff revision 4)
> +  await pushPref("devtools.browserconsole.html", true);
> +  await pushPref("devtools.chrome.enabled", true);

Add comments to explain why those preferences are needed

::: devtools/client/webconsole/utils/context-menu.js:194
(Diff revision 4)
> + *
> + * You'll need to call menu.popup() yourself, this just returns the Menu instance.
> + *
> + * @returns {Menu}
> + */
> +function createEditContextMenu() {

This looks legit.
Do you think we could have a test to make sure the webconsole edit context menu and the browser console one are the same ?
The goal would be to be notified if at some point the context menu evolves so we need to modify our implementation.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> Comment on attachment 8990617 [details]
> Bug 1456852 - Programatically create the edit menu in the browser console
> input;
> 
> do you think we could move this outside of createMenuItems so we don't
> create it each time, for each item in the menu ? It looks like it's easily
> doable by passing the item variable.

Sure, makes sense.

> 
> ::: devtools/client/framework/test/browser_menu_api.js:108
> (Diff revision 4)
> >    ok(!menuItems[2].hasAttribute("checked"), "Doesn't have checked attr");
> >  
> >    is(menuItems[3].getAttribute("label"), MENU_ITEMS[3].label, "Correct label");
> >    is(menuItems[3].getAttribute("disabled"), "true", "disabled attr menuitem");
> >  
> > +  is(menuItems[4].getAttribute("data-l10n-id"), MENU_ITEMS[4].l10nID, "Correct localization attribute");
> 
> could we also check textContent ?

In this case we haven't registered any localizations for the test document, so we are taking it on faith that if we set "data-l10n-id" then fluent will do its thing. Then actual consumers of the menu (like browser_console_context_menu_entries.js should check the strings.

> ::: devtools/client/webconsole/components/JSTerm.js:1332
> (Diff revision 4)
> > +    // The toolbox does it's own edit menu handling with
> > +    // toolbox-textbox-context-popup and friends. For now, fall
> > +    // back to use that if running inside the toolbox, but use our
> > +    // own menu when running in the Browser Console.
> > +    if (this.props.hud.isBrowserConsole &&
> > +        Services.prefs.getBoolPref("devtools.browserconsole.html")) {
> 
> maybe add a comment with the bug number when we plan to make this the
> default ?

Yep - Bug 1476097.

> ::: devtools/client/webconsole/index.html:12
> (Diff revision 4)
> > +    <link rel="localization" href="toolkit/main-window/editmenu.ftl"/>
> > +    <link rel="stylesheet" href="chrome://global/skin/"/>
> >      <link rel="stylesheet" href="chrome://devtools/skin/widgets.css"/>
> >      <link rel="stylesheet" href="chrome://devtools/skin/webconsole.css"/>
> >      <link rel="stylesheet" href="chrome://devtools/skin/components-frame.css"/>
> >      <link rel="stylesheet" href="resource://devtools/client/shared/components/reps/reps.css"/>
> >      <link rel="stylesheet" href="resource://devtools/client/shared/components/tabs/Tabs.css"/>
> >      <link rel="stylesheet" href="resource://devtools/client/shared/components/tabs/TabBar.css"/>
> >      <link rel="stylesheet" href="resource://devtools/client/shared/components/NotificationBox.css"/>
> >      <link rel="stylesheet" href="chrome://devtools/content/netmonitor/src/assets/styles/httpi.css"/>
> >  
> > +    <script type="text/javascript" src="chrome://global/content/l10n.js"></script>
> 
> are those new resources needed only for the browser console ?
> If yes, would there be a way to load them only if we're in the browser
> console instead ?

Yes, although the script will not be required after Bug 1455649 (and the localization link needs to be in the markup until that bug lands as well).

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> ::: devtools/client/webconsole/utils/context-menu.js:194
> (Diff revision 4)
> > + *
> > + * You'll need to call menu.popup() yourself, this just returns the Menu instance.
> > + *
> > + * @returns {Menu}
> > + */
> > +function createEditContextMenu() {
> 
> This looks legit.
> Do you think we could have a test to make sure the webconsole edit context
> menu and the browser console one are the same ?
> The goal would be to be notified if at some point the context menu evolves
> so we need to modify our implementation.

I'd like to wait until Bug 1476097 to do that. The Edit menu is unlikely to change in either place. It's pretty standardized and we already have many copies of it across the tree. Also browser_console_context_menu_entries.js should ensure we don't regress the new one.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

https://reviewboard.mozilla.org/r/255674/#review264184


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/framework/menu.js:201
(Diff revision 5)
> +  }
> +  if (item.id) {
> +    node.id = item.id;
> +  }
> +}
> +

Error: More than 1 blank line not allowed. [eslint: no-multiple-empty-lines]

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8990617 [details]
Bug 1456852 - Programatically create the edit menu in the browser console input;

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

Comment 18

[Brian Grinstead [:bgrins]]

Try and talos look good so far: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9ef6b24ea591656d04bcb6f2fcfe759f82fd12a
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d4f1dc3c351aa8f21d3302cc68694e98d611ba98&newProject=try&newRevision=8216e06d45ed1a9eed4335c7a7fdc073aac977ae&framework=1&filter=damp

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99d1ca47faea
Programatically create the edit menu in the browser console input;r=nchevobbe

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/99d1ca47faea

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta backport?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Is this something we should consider for Beta backport?

No, I don't think so. The textbox is only visible when chrome debugging is enabled, and this fix depends on a few DOM and localization changes that I don't think are worth uplifting for this feature.

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

Thanks Brian.