1278287 - Migrate the style-inspector-menu module (context menus for Rules and Computed view) to the new Menu API

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

Bug 1266478 will convert the main inspector context menu, but the computed and rules views have a different place where menus are created (style-inspector-menu)

Comment 1

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

Not finished, but I wanted a preliminary review before continuing, to see if I've missed anything major.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8762216 [details] [diff] [review]
style-inspector-menu.patch

Review of attachment 8762216 [details] [diff] [review]:
-----------------------------------------------------------------

Good start, see comment in line

::: devtools/client/inspector/shared/style-inspector-menu.js
@@ +74,5 @@
>        // so we need to save the node ourselves.
>        this.styleDocument.popupNode = event.explicitOriginalTarget;
>        this.styleWindow.focus();
> +      this.menu.popup(event.screenX, event.screenY, this.inspector._toolbox);
> +      this._updateMenuItems();

I'd basically remove the _updateMenuItems function and fold it's logic into the _createContextMenuItems function, then call _createContextMenuItems() before calling menu.popup.  The idea being that we recreate the menu from scratch with the correct menuitem state each time before a popup instead of previously, when the menu items were created once, assigned as instance variables, and then modified during the popup event.

This means we can stuff all of the menu creation logic into a single function and remove the DOM modification code.  In this case, the removeContextMenuItems function will also not be needed.

Comment 3

[Maximillian von Briesen [:moby]]

There's a lot of code in _updateMenuItems as well as _updateCopyMenuItems. Instead of removing those functions, can I just call them from _createContextMenuItems?

Comment 4

[Maximillian von Briesen [:moby]]

Also, since you want to create a new menu each time, shouldn't I call _createContextMenu rather than _createContextMenuItems before the popup? Otherwise I'd be appending items to the menu that already exists.

Comment 5

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8764042 [details] [diff] [review]
style-inspector-menu.patch

Review of attachment 8764042 [details] [diff] [review]:
-----------------------------------------------------------------

We discussed in person - this is on the right path.  Tests still need updating.  Also, please make _createContextMenu return a Menu, instead of attaching it onto the instance

Comment 7

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ccfe8611197

Comment 8

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

fixed eslint errors

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3067f7923fb0

Comment 9

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

eslint fixes... again
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a013dd24d84c

Comment 10

[Maximillian von Briesen [:moby]]

Comment on attachment 8765006 [details] [diff] [review]
style-inspector-menu.patch

Review of attachment 8765006 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is ready for review now. I don't see anything notable on the try push.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8765006 [details] [diff] [review]
style-inspector-menu.patch

Review of attachment 8765006 [details] [diff] [review]:
-----------------------------------------------------------------

Looking great!  Two more tests that need updating and weren't in our try push b/c they are part of the cliboard suite: browser_styleinspector_context-menu-copy-color_02.js and browser_styleinspector_context-menu-copy-urls.js

Comment 12

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

Fixed those two tests. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f811b637bdc

Comment 13

[Brian Grinstead [:bgrins]]

ES lint failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f811b637bdc&selectedJob=23128734

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8766457 [details] [diff] [review]
style-inspector-menu.patch

See Comment 13

Comment 15

[Maximillian von Briesen [:moby]]

Attachment: style-inspector-menu.patch

Fix eslint problems. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=488203173774

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8766518 [details] [diff] [review]
style-inspector-menu.patch

Review of attachment 8766518 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4e695d4a3a84
Migrate the style-inspector-menu module; r=bgrins

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e695d4a3a84

Comment 19

[Ada [:adalucinet]]

Found some potential issues while verifying this fix using latest Nightly 50.0a1, across platforms [1], as it follows:
1. In Computed view, ‘Show MDN Docs’ option via the context menu remains displayed when scrolling up/down.
2. In Computed view, after clicking on ‘Visit MDN page’ link, the tooltip remains visible; Shouldn’t it fade away on click?
3. With Light and Firebug Themes, ‘Visit MDN page’ link has the same color before and after clicking on it. Shouldn't there be a difference between both states (clicked/not clicked) like with the Dark theme? 
4. [Mac OS X only] - there is no hand cursor at mouse over ‘Visit MDN page’ link. 
4.1. Not reproducible with the Dark theme.
5. bug 1285229 is also applicable for both Rules and Computed context menus - the context menu remains displayed when repeatedly right clicking on any elements

All the mentioned inconsistencies are available in this - https://goo.gl/mcbYF4 - screen recording.
 
Maximillian, any ideas? Looking forward for your input regarding the above! Thanks!

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

Comment 20

[Maximillian von Briesen [:moby]]

I don't think I am hiding the popup properly in the new patch. I'll see if I can fix these issues. Thanks for the help, Alexandra.

Comment 21

[Brian Grinstead [:bgrins]]

The MDN Docs popup seems unrelated to the context menu used to open it - I don't think the things from Comment 19 have to do with the work to convert the context menu. Alexandra, did the behavior here change as a result of the bug, or do you see the same behavior in an earlier build?  I believe I see the same thing in Dev Edition.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #19)
> 5. bug 1285229 is also applicable for both Rules and Computed context menus
> - the context menu remains displayed when repeatedly right clicking on any
> elements

I believe the fix in Bug 1285229 will also solve it here

Comment 23

[Ada [:adalucinet]]

(In reply to Brian Grinstead [:bgrins] from comment #21)
> Alexandra, did the behavior here change as a result of the
> bug, or do you see the same behavior in an earlier build?  I believe I see
> the same thing in Dev Edition.

None of the mentioned issues are recent regressions - also reproducible with latest Developer Edition 49.0a2, across platforms. Should I file separate reports?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > Alexandra, did the behavior here change as a result of the
> > bug, or do you see the same behavior in an earlier build?  I believe I see
> > the same thing in Dev Edition.
> 
> None of the mentioned issues are recent regressions - also reproducible with
> latest Developer Edition 49.0a2, across platforms. Should I file separate
> reports?

Yes please, file issues in the inspector component.  Also note that bug 1283454 is in progress and modifying the mdn tooltip, although I doubt it will affect most of these issues.

Comment 25

[Ada [:adalucinet]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> Yes please, file issues in the inspector component.  Also note that bug
> 1283454 is in progress and modifying the mdn tooltip, although I doubt it
> will affect most of these issues.
Thank you, Brian!

> 1. In Computed view, ‘Show MDN Docs’ option via the context menu remains displayed when scrolling up/down.
There was already a report on this matter - commented in bug 938097

> 2. In Computed view, after clicking on ‘Visit MDN page’ link, the tooltip remains visible
> 3. With Light and Firebug Themes, ‘Visit MDN page’ link has the same color before and after clicking on it.
Logged bug 1286521

> 4. [Mac OS X only] - there is no hand cursor at mouse over ‘Visit MDN page’ link.
Weird fact - this issue is no longer reproducible under Mac OS X 10.11.1 nor 10.9.5, using latest 50.0a1; tried even with the Nightly build used in comment 19, but without success. 

Based on this output, marking here accordingly.