1376291 - [Launchpad] Netmonitor Timings checkboxes are not accessible

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: Checkboxes.gif

[Affected versions]:
Firefox 56.0a1 (Build Id:20170625030210)

[Affected platforms]:
Windows 10 64bit
Mac 10.11.6
Ubuntu 16.04 64bit


[Steps to reproduce]:
1.Launch Firefox.
2.Open Netmonitor via launchpad.
3.Access the https://en.wikipedia.org/wiki/Main_Page webpage via Launchpad.
4.Right Click on any column.
5.Hover the Timings option.

[Expected result]:
The Timings options can be accessed.

[Actual result]:
The Timings options are not accessible.

[Regression range]:
This is reproducible using Launchpad 0.0.75.
I don't think that this is a regression.

[Additional Information]
This issue is not reproducible with Netmonitor opened from DevTools.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report!

This bug is part of devtools-core packages.

Reported here:
https://github.com/devtools-html/devtools-core/issues/471

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1376291 - Use devtools-contextmenu module for Launchpad

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

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

There is PR for context-menu fix:
https://github.com/devtools-html/devtools-core/issues/471

And patch is attached fixing the Network monitor panel

Needs testing yet (as soon as the PR lands).

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8885797 [details]
Bug 1376291 - Use devtools-contextmenu module for Launchpad

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

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8885797 [details]
Bug 1376291 - Use devtools-contextmenu module for Launchpad

https://reviewboard.mozilla.org/r/156584/#review162002

LGTM. I don't see any big issue. Please address following nits.

Thank you Honza!

::: devtools/client/netmonitor/package.json:14
(Diff revision 2)
>      "codemirror": "^5.24.2",
>      "devtools-config": "=0.0.12",
>      "devtools-launchpad": "=0.0.75",
>      "devtools-modules": "=0.0.27",
>      "devtools-source-editor": "=0.0.3",
> +    "devtools-contextmenu": "^0.0.2",

Are you install package through `yarn add devtools-contextmenu`?

`devtools-contextmenu` should be in alphabetical order and be placed after `devtools-config`.

Please using "=0.0.2" to fix the version for stability. And remember to run `yarn install` once you change it.

::: devtools/client/netmonitor/webpack.config.js:44
(Diff revision 2)
>      alias: {
>        "react": path.join(__dirname, "node_modules/react"),
>        "devtools/client/framework/devtools": path.join(__dirname, "../../client/shims/devtools"),
>        "devtools/client/framework/menu": "devtools-modules/src/menu",
>        "devtools/client/framework/menu-item": path.join(__dirname, "../../client/framework/menu-item"),
> +      "./utils/menu": path.join(__dirname, "devtools-contextmenu"),

nit: let's move this to the end (after "Services": "devtools-modules/src/Services").

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8885797 [details]
Bug 1376291 - Use devtools-contextmenu module for Launchpad

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

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #5)
> ::: devtools/client/netmonitor/package.json:14
> `devtools-contextmenu` should be in alphabetical order and be placed after
> `devtools-config`.
Done

> Please using "=0.0.2" to fix the version for stability. And remember to run
> `yarn install` once you change it.
Done

> nit: let's move this to the end (after "Services":
> "devtools-modules/src/Services").
Done


Thanks for the review Ricky!
Honza

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d149668f7570
Use devtools-contextmenu module for Launchpad r=rickychien

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Ricky: did you try the patch on your machine?

I am suddenly seeing an error and I am not sure if it's my wrong config or the patch isn't ready.

ERROR in ./src/request-list-context-menu.js
Module not found: Error: Cannot resolve module 'devtools-launchpad/src/components/shared/menu' in C:\src\mozilla.org\mozilla-central\devtools\client\netmonitor\src
 @ ./src/request-list-context-menu.js 22:21-73

ERROR in ./src/request-list-header-context-menu.js
Module not found: Error: Cannot resolve module 'devtools-launchpad/src/components/shared/menu' in C:\src\mozilla.org\mozilla-central\devtools\client\netmonitor\src
 @ ./src/request-list-header-context-menu.js 9:21-73


Honza

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d149668f7570

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Ah, Sorry it's my fault that I ran it on toolbox but not on Launchpad.

Webpack alias cannot support ./xxx/xxx pattern [1]. So the only way you can do for supporting both toolbox and launchpad is to change "./utils/menu" at [2][3] to "devtools/client/netmonitor/src/utils/menu" and then update the webpack alias in webpack.config.js to "devtools/client/netmonitor/src/utils/menu" (remember in alphabetical order) as well. 

[1] https://webpack.github.io/docs/configuration.html#resolve-alias
[2] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#22
[3] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-header-context-menu.js#9

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Please file a followup and I'll review carefully again. Thanks

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Follow up: bug 1380927

Let's continue discussion there.

Thanks,
Honza