1266478 - Migrate the inspector context menus to the new Menu API

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Once we have Bug 1257613 we can integrate this with the inspector

Comment 1

[Brian Grinstead [:bgrins]]

This is blocked on Bug 1211613 I think which is reorganizing the menu in the inspector

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: use-menu-api-inspector-WIP.patch

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: use-menu-api-inspector.patch

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1266478 - Integrate Menu API with inspector;

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

Comment 5

[Brian Grinstead [:bgrins]]

Patrick, this is really a 'feedback' request since there's going to need to be a number of test updates before attempting to land.  But I'd like initial review / usage notes if possible.  I hope this implementation is a lot easier to follow, since it centralizes all of the menu creation code into one place, and modifying the MenuItem properties is more convenient than modifying the DOM directly.

We'll want to block on the breadcrumb menu removal also as part of this (currently in Bug 1259812 but that part could be broken out into another patch).

Comment 6

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/50255/#review47937

::: devtools/client/inspector/inspector-panel.js:177
(Diff revision 1)
>      this.nodemenu = this.panelDoc.getElementById("inspector-node-popup");
>      this.lastNodemenuItem = this.nodemenu.lastChild;

I don't see these 2 variables used anywhere anymore. Can we remove them ?
If yes, I guess #inspector-node-popup in inspector.xul can be removed as well.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8748389 [details]
Bug 1266478 - Integrate Menu API with inspector;

https://reviewboard.mozilla.org/r/50255/#review48025

mozreview doesn't yet allow F+, so consider this an F+.
I've added a few peripheral comments below, but when it comes to the actual important changes: using Menu, MenuItem, moving away from the XUL code, I'm fine, I like this a lot.

::: devtools/client/inspector/inspector-panel.js:103
(Diff revision 1)
> +  this._target.actorHasMethod("domwalker", "duplicateNode").then(value => {
> +    this._supportsDuplicateNode = value;
> +  });
> +
> +  this._supportsScrollIntoView = false;
> +  this._target.actorHasMethod("domnode", "scrollIntoView").then(value => {
> +    this._supportsScrollIntoView = value;
> +  });
> +
> +  this._supportsResolveRelativeURL = false;
> +  this._target.actorHasMethod("inspector", "resolveRelativeURL").then(value => {
> +    this._supportsResolveRelativeURL = value;
> +  });

Please declare error handlers (.catch) for each of those.

Also, when adding a comment for a large block of code in a function, I usually think this is a clue for moving that block of code in its own function.

It's very likely that this section grows even more in the future, so it will be better in its own function.

::: devtools/client/inspector/inspector-panel.js
(Diff revision 1)
>      this.walker.on("new-root", this.onNewRoot);
>  
>      this.nodemenu = this.panelDoc.getElementById("inspector-node-popup");
>      this.lastNodemenuItem = this.nodemenu.lastChild;
> -    this.nodemenu.addEventListener("popupshowing", this._setupNodeMenu, true);
> -    this.nodemenu.addEventListener("popuphiding", this._resetNodeMenu, true);

This made me realize that we have 2 other functions in this file: showNodeMenu and hideNodeMenu that don't seem to be used anymore (hideNodeMenu is used in breadcrumbs, but since we don't have a menu there anymore, this usage should be removed).

Can you investigate a little bit more and confirm that these can be removed?

::: devtools/client/inspector/inspector-panel.js:740
(Diff revision 1)
> +      click: () => {
> +        this.editHTML();
> +      },

I know you don't need/want to return anything from this event handler, but it would look better on one line (same for the other ones below):

`click: () => this.editHTML()`

::: devtools/client/inspector/inspector-panel.js:917
(Diff revision 1)
> +    for (let menuitem of this._getNodeLinkMenuItems()) {
> +      menu.append(menuitem);
> +    }

Instead of added invisible items, I think we should just add what is visible. And I think the separator shouldn't be managed in `_getNodeLinkMenuItems` but here instead.
So `_getNodeLinkMenuItems` should return an array of 0, 1 or 2 elements depending on what should be visible. And here you would prepend a separator if the array has a non-zero length.

::: devtools/client/inspector/inspector-panel.js:999
(Diff revision 1)
> -  _setupAttributeMenu: function(isEditableElement) {
> -    let addAttribute = this.panelDoc.getElementById("node-menu-add-attribute");
> +  _getAttributesSubmenu: function(isEditableElement) {
> +    let attributesSubmenu = new Menu();
> -    let editAttribute = this.panelDoc.getElementById("node-menu-edit-attribute");
> -    let removeAttribute = this.panelDoc.getElementById("node-menu-remove-attribute");
>      let nodeInfo = this.nodeMenuTriggerInfo;
> +    let attributeWasClicked = isEditableElement && nodeInfo &&

For parity with other boolean names: `wasAttributeClicked`, or even `isAttributeClicked`.

::: devtools/client/inspector/inspector-panel.js:1014
(Diff revision 1)
> -    // Enable "Edit Attribute" and "Remove Attribute" only on attribute click
> -    if (isEditableElement && nodeInfo && nodeInfo.type === "attribute") {
> -      editAttribute.removeAttribute("disabled");
> -      editAttribute.setAttribute("label",
> -        strings.formatStringFromName(
> -          "inspector.menu.editAttribute.label", [`"${nodeInfo.name}"`], 1));
> +      },
> +    }));
> +    attributesSubmenu.append(new MenuItem({
> +      id: "node-menu-edit-attribute",
> +      label: strings.formatStringFromName("inspectorEditAttribute.label",
> +                 [attributeWasClicked ? `"${nodeInfo.name}"` : ''], 1),

Please use double-quotes only "" instead of ''

::: devtools/client/inspector/inspector.xul:41
(Diff revision 1)
>        modifiers="accel"
>        command="nodeSearchCommand"/>
>    </keyset>
>  
>    <popupset id="inspectorPopupSet">
>      <!-- Used by the Markup Panel, the Highlighter and the Breadcrumbs -->

While you're changing code around here, this comment needs updating. The highlighter and breadcrumbs don't use this menu anymore.

::: devtools/client/locales/en-US/inspector.properties:117
(Diff revision 1)
> -inspector.menu.editAttribute.label=Edit Attribute %S
> +inspectorEditAttribute.label=Edit Attribute %S
> +inspectorEditAttribute.accesskey=E
>  
>  # LOCALIZATION NOTE (inspector.menu.removeAttribute.label): This is the label of a
>  # sub-menu "Attribute" in the inspector contextual-menu that appears
>  # when the user right-clicks on the attribute of a node in the inspector,
>  # and that allows to remove this attribute.
> -inspector.menu.removeAttribute.label=Remove Attribute %S
> +inspectorRemoveAttribute.label=Remove Attribute %S

Why did you change these key names? I'm guessing for consistency reasons, right? If so, please also change the key names in the LOCALIZATION NOTE above.

::: devtools/client/locales/en-US/inspector.properties:146
(Diff revision 1)
> +# LOCALIZATION NOTE (inspectorHTMLEdit.label): This is the label shown
> +# in the inspector contextual-menu for the item that lets users edit the
> +# (outer) HTML of the current node
> +inspectorHTMLEdit.label=Edit As HTML
> +inspectorHTMLEdit.accesskey=E

This string and the next ones below are straight out moved from a dtd file. Maybe it's worth needinfo'ing :flod on the bug to let him know about that. Maybe they can move the translations too, instead of re-translating.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #7)
> ::: devtools/client/locales/en-US/inspector.properties:146
> (Diff revision 1)
> > +# LOCALIZATION NOTE (inspectorHTMLEdit.label): This is the label shown
> > +# in the inspector contextual-menu for the item that lets users edit the
> > +# (outer) HTML of the current node
> > +inspectorHTMLEdit.label=Edit As HTML
> > +inspectorHTMLEdit.accesskey=E
> 
> This string and the next ones below are straight out moved from a dtd file.
> Maybe it's worth needinfo'ing :flod on the bug to let him know about that.
> Maybe they can move the translations too, instead of re-translating.

Hi :flod, I have a patch here that is going to be moving a bunch of strings from a .dtd file into a .properties file, here: https://reviewboard.mozilla.org/r/50255/diff/1#3.  Is there anything I can do to make this migration easier for localization, since the string values aren't changing?

Comment 9

[Francesco Lodolo [:flod]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Hi :flod, I have a patch here that is going to be moving a bunch of strings
> from a .dtd file into a .properties file, here:
> https://reviewboard.mozilla.org/r/50255/diff/1#3.  Is there anything I can
> do to make this migration easier for localization, since the string values
> aren't changing?

Sadly no, since most teams work on external tools, so creating a script that moves strings around wouldn't help.
The only thing I can do is to add this changeset to the report we send at the beginning of the cycle.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #7)
> ::: devtools/client/inspector/inspector-panel.js
> (Diff revision 1)
> >      this.walker.on("new-root", this.onNewRoot);
> >  
> >      this.nodemenu = this.panelDoc.getElementById("inspector-node-popup");
> >      this.lastNodemenuItem = this.nodemenu.lastChild;
> > -    this.nodemenu.addEventListener("popupshowing", this._setupNodeMenu, true);
> > -    this.nodemenu.addEventListener("popuphiding", this._resetNodeMenu, true);
> 
> This made me realize that we have 2 other functions in this file:
> showNodeMenu and hideNodeMenu that don't seem to be used anymore
> (hideNodeMenu is used in breadcrumbs, but since we don't have a menu there
> anymore, this usage should be removed).
> 
> Can you investigate a little bit more and confirm that these can be removed?

Yes I think they can be removed - they are gone now.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8748389 [details]
Bug 1266478 - Integrate Menu API with inspector;

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

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8748389 [details]
Bug 1266478 - Integrate Menu API with inspector;

Clearing review - just stashing initial test improvements and a rebase here

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8748389 [details]
Bug 1266478 - Integrate Menu API with inspector;

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

Comment 14

[Brian Grinstead [:bgrins]]

Whew, lots of tests there.  I think the tests are generally easier to follow now (since new menu api is sync, and asserting things on the MenuItem objects is a little easier than asserting menuitem DOM nodes).  But please have a look, there were a lot of updates and I might have missed some stuff.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4256086e3e9d

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #7)
> Instead of added invisible items, I think we should just add what is
> visible. And I think the separator shouldn't be managed in
> `_getNodeLinkMenuItems` but here instead.
> So `_getNodeLinkMenuItems` should return an array of 0, 1 or 2 elements
> depending on what should be visible. And here you would prepend a separator
> if the array has a non-zero length.

FYI I made this change in v3, but then switched it back in v4 because adding the invisible items made it easier to test

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8748389 [details]
Bug 1266478 - Integrate Menu API with inspector;

https://reviewboard.mozilla.org/r/50255/#review55928

Looks good. One question about a potential race condition in tests.

::: devtools/client/inspector/inspector-panel.js:112
(Diff revision 3)
>    this.addNode = this.addNode.bind(this);
>    this.addNodeButton = doc.getElementById("inspector-element-add-button");
>    this.addNodeButton.addEventListener("click", this.addNode);
>  
>    this._target.on("will-navigate", this._onBeforeNavigate);
> +  this._detectActorFeatures();

So, this calls various async things on the server, so there's a chance that if you open the menu qickly enough, you won't get the right menu items enabled because the server hasn't yet responded.
I doubt this will ever happen in production, but can this happen in slow tests?

::: devtools/client/inspector/inspector-panel.js:175
(Diff revision 3)
> +   */
> +  _detectActorFeatures: function () {
> +    this._supportsDuplicateNode = false;
> +    this._target.actorHasMethod("domwalker", "duplicateNode").then(value => {
> +      this._supportsDuplicateNode = value;
> +    }).catch();

Should be `.catch(e => console.error(e));` rather than an empty catch.
Same below.

::: devtools/client/inspector/test/head.js:816
(Diff revision 3)
> + * @return An array of MenuItems
> + */
> +function openContextMenuAndGetAllItems(inspector, options) {
> +  let menu = inspector._openMenu(options);
> +
> +  // Flatten all menu items into a single array to ake searching through it easier

s/ake/make

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #16)
> Comment on attachment 8748389 [details]
> Bug 1266478 - Integrate Menu API with inspector;
> 
> https://reviewboard.mozilla.org/r/50255/#review55928
> 
> Looks good. One question about a potential race condition in tests.
> 
> ::: devtools/client/inspector/inspector-panel.js:112
> (Diff revision 3)
> >    this.addNode = this.addNode.bind(this);
> >    this.addNodeButton = doc.getElementById("inspector-element-add-button");
> >    this.addNodeButton.addEventListener("click", this.addNode);
> >  
> >    this._target.on("will-navigate", this._onBeforeNavigate);
> > +  this._detectActorFeatures();
> 
> So, this calls various async things on the server, so there's a chance that
> if you open the menu qickly enough, you won't get the right menu items
> enabled because the server hasn't yet responded.
> I doubt this will ever happen in production, but can this happen in slow
> tests?

Hm, I guess that's possible.  Seems unlikely because we wait for an inspector update but I have a workaround for this that I'll push up to reviewboard and then land where we can emit an event once all the features are detected and yield on it if needed.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8748389 [details]
Bug 1266478 - Integrate Menu API with inspector;

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

Comment 19

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1042f8e66508

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fb3a694a5c43
Integrate Menu API with inspector;r=pbro

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/618a83361367
Eslint fixes followup;r=me

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb3a694a5c43
https://hg.mozilla.org/mozilla-central/rev/618a83361367

Comment 23

[Ada [:adalucinet]]

Verified with latest Nightly 50.0a1, across platforms [1], and found some inconsistencies: 
1. No shortcut key available for Attributes option → screenshot: https://i.imgur.com/9IAtLLI.png
* with the latest 49.0a2 the shortcut is displayed (https://i.imgur.com/NpBzM2v.png)
2. [Windows 10 64-bit _only_] when fast right clicking on any element from the HTML tree, the context menu remains displayed several times 
* screenshot: https://i.imgur.com/iFvYyB2.png

Brian, any ideas? Should I file new reports for both mentioned issues? Thanks in advance!

[1] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #23)
> Verified with latest Nightly 50.0a1, across platforms [1], and found some
> inconsistencies: 
> 1. No shortcut key available for Attributes option → screenshot:
> https://i.imgur.com/9IAtLLI.png
> * with the latest 49.0a2 the shortcut is displayed
> (https://i.imgur.com/NpBzM2v.png)
> 2. [Windows 10 64-bit _only_] when fast right clicking on any element from
> the HTML tree, the context menu remains displayed several times 
> * screenshot: https://i.imgur.com/iFvYyB2.png
> 
> Brian, any ideas? Should I file new reports for both mentioned issues?
> Thanks in advance!
> 
> [1] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit

Thank you, and yes, please file issues for both of those

Comment 25

[Ada [:adalucinet]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> Thank you, and yes, please file issues for both of those
Sure thing!
Logged bug 1285229 and bug 1285225. 
Marking here accordingly.