1350224 - Support for loading ContextMenu in Launchpad

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[[e-mail]]

Support for loading ContextMenu in Launchpad

Use whatever Debugger.html is using

Comment 1

[Tim Nguyen :ntim]

I think we should support a shim of the menu API in devtools-sham-modules and make the debugger use the menu API. That's a win on both sides because the debugger will benefit from native context menu in the toolbox, and the netmonitor will be able to benefit from the debugger context menu when running in a webpage.

Comment 2

[Tim Nguyen :ntim]

Actually, devtools-core has a wrapper that detects which environment its in, and customizes the context menu as appropriate. https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/src/menu.js#L3

Comment 3

[Jason Laster [:jlast]]

We already have support for showing context menus in the launchpad :)

I've added some docs for how to use the API:
https://github.com/devtools-html/debugger.html/blob/master/docs/local-development.md#context-menus

Comment 4

[Ricky Chien [:rickychien] (inactive)]

devtools-launchpad hasn't support contextmenu for sub-menu. We should create a RP in devtools-core to fix that.

Comment 5

[Tim Nguyen :ntim]

(In reply to Ricky Chien [:rickychien] from comment #4)
> devtools-launchpad hasn't support contextmenu for sub-menu. We should create
> a RP in devtools-core to fix that.

I don't think it support the checkmark for checked items either (needed for the header context menu).

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1350224 - Support for loading ContextMenu in Launchpad

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

Comment 7

[Ricky Chien [:rickychien] (inactive)]

PR for supporting sub-menu and checkbox in devtools-core, please check https://github.com/devtools-html/devtools-core/pull/324

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

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

Comment 9

[[e-mail]]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

https://reviewboard.mozilla.org/r/128502/#review131192

Thanks for the patch! Overall looks good, please move menu.js into `utils/chrome` and send review again

::: devtools/client/netmonitor/src/utils/menu.js:7
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const Menu = require("devtools/client/framework/menu");

since this file wrap the usage of privilliged API., it is a good candidate to put into `utils/chrome`

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

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

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

https://reviewboard.mozilla.org/r/128502/#review131192

> since this file wrap the usage of privilliged API., it is a good candidate to put into `utils/chrome`

I believe all "devtools/client/framework/menu" and "devtools/client/framework/menu-item" can treat as a shim modules and that works perfectly in launchpad if we replace it by webpack alias. So I'd prefer to put it in utils/. I know that's tricky but this menu.js wrapper is supposed to be replaced elegantly by https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/src/components/shared/menu.js in someday when we are ready to bundle everyting in m-c.

This the way like we can switch the shim / sham module through webpack alias.

Comment 12

[[e-mail]]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

ok

Comment 13

[[e-mail]]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

https://reviewboard.mozilla.org/r/128502/#review131204

Comment 14

[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 0054681cbac1 -d 596ea55fa962: rebasing 388453:0054681cbac1 "Bug 1350224 - Support for loading ContextMenu in Launchpad r=gasolin" (tip)
merging devtools/client/netmonitor/src/request-list-context-menu.js
warning: conflicts while merging devtools/client/netmonitor/src/request-list-context-menu.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8856545 [details]
Bug 1350224 - Support for loading ContextMenu in Launchpad

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2044e263e1ba
Support for loading ContextMenu in Launchpad r=gasolin

Comment 17

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/2044e263e1ba

Comment 18

[Ciprian Georgiu, Desktop QA]

I verified this issue using latest Nightly 55.0a1 on Windows 10 x64. 

The context menu and sub-menus are properly loaded in the Netmonitor Launchpad, but I see an UI issue if context menu is open from the bottom page. It's displayed below the selected request and not above as it is in netmonitor (non-Launchpad version). You can see here a screenshot: http://imgur.com/a/pXkMU.

What do you think about this, Ricky?

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Thank you testing this very carefully! Technically, launchpad is running on browser content page (it's just a normal web page), so the element is unable to cross the browser chrome. 

As a result, this behavior is reasonable and correct!

btw, this kind of case could be enhanced by positioning the context menu to available space, so user can still see the entire menu even though triggering it at the corner.

Thanks!

Comment 20

[Ciprian Georgiu, Desktop QA]

Thanks for the clarification, Ricky! In this case, marking here as verified fixed.