1355833 - Fix broken context menu actions on browser

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

HTML menu.js runs on launchpad is different from m-c, even though we've supported context-menu and sub-menu in launchpad but onClick actions in context menu are still broken.

Broken menu actions are like Copy URL, Copy Post Data, Copy as cURL...etc. We should fix them.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1355833 - Fix broken context menu actions

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

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 4

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

I am seeing some issues when testing this patch


1) TypeError: node.getBoxQuads is not a function
Only in Chrome. It's because "getBoxQuads" is Firefox specific API
https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-modules/client/shared/widgets/tooltip/HTMLTooltip.js#L180

Probably not worth fixing now since we have other priorities but, it would be nice to have these things documented somewhere.

2) FileSaver.saveAs
In both Firefox and Chrome

Honza

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

https://reviewboard.mozilla.org/r/129482/#review132480

::: devtools/client/netmonitor/src/request-list-context-menu.js:57
(Diff revision 5)
>  
>      copySubmenu.push({
>        id: "request-list-context-copy-url",
>        label: L10N.getStr("netmonitor.context.copyUrl"),
>        accesskey: L10N.getStr("netmonitor.context.copyUrl.accesskey"),
> -      visible: !!selectedRequest,
> +      disabled: !selectedRequest,

I think hiding is a better UX than disabling. Can we do that ?

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 9

[Ricky Chien [:rickychien] (inactive)]

It's weird that the clipboard API [1] doesn't work in browser launchpad when calling it within promise or async function like copyPostData [2] and copyImageAsDataUri [3]. I suspect that is platform issue.

[1] http://searchfox.org/mozilla-central/source/devtools/shared/platform/content/clipboard.js#11-20
[2] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#235-267
[3] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#324-331

Comment 10

[Ricky Chien [:rickychien] (inactive)]

https://jsfiddle.net/kx2pu7pn/2/ here is an example show that we can copy text in a separate non-user generated event handler. 

console throws "document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler.", so the behavior has been blocked by browser.

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 12

[Ricky Chien [:rickychien] (inactive)]

In order to deal with clipboard issue on launchpad (see comment 9 and comment 10), I removed async function and promise within click event callback. That means we will rely on long string data which fetches from netmonitor-controller. From user aspect, normal user is unlikely to trigger a copy action as quick as long string data is unavailable. So IMO the removal of getLongString  API would be safe (however, we still need to add extra waiting API in mochitest).

Here is a documentation said that why we get this warning when executing copy command in asynchronous context (setTimeout, promise, async await..etc) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Interact_with_the_clipboard

Comment 13

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

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

https://reviewboard.mozilla.org/r/129482/#review132620

Thanks Ricky for working on this!

Honza

::: devtools/client/netmonitor/src/request-list-context-menu.js:59
(Diff revision 1)
>  
>      copySubmenu.push({
>        id: "request-list-context-copy-url",
>        label: L10N.getStr("netmonitor.context.copyUrl"),
>        accesskey: L10N.getStr("netmonitor.context.copyUrl.accesskey"),
> -      visible: !!selectedRequest,
> +      disabled: !selectedRequest,

Ah, so I didn't understand properly on IRC. I thought that we disabled action that are not supported in Launchpad.

In this case, I vote for keeping the UI as it was (i.e. hiding instead of disabling).

But, if you like this approach we can talk to Blake and ask for his opinion.

::: devtools/client/netmonitor/src/request-list-context-menu.js:127
(Diff revision 1)
>        accesskey: L10N.getStr("netmonitor.context.copyImageAsDataUri.accesskey"),
> -      visible: !!(selectedRequest &&
> +      disabled: !(selectedRequest &&
>                 selectedRequest.responseContent &&
>                 selectedRequest.responseContent.content.mimeType.includes("image/")),
>        click: () => this.copyImageAsDataUri(),
>      });

I couldn't make this work on my machine. Copy Image As Data URI never put anything into the clipboard. Anyone can repro that?

Comment 14

[[e-mail]]

As we talked yesterday I prefer keep the menu invisible instead of disable. Some menu item does not make sense (Open in debugger) for unsupported data type, so we better hide it.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

https://reviewboard.mozilla.org/r/129482/#review132620

> Ah, so I didn't understand properly on IRC. I thought that we disabled action that are not supported in Launchpad.
> 
> In this case, I vote for keeping the UI as it was (i.e. hiding instead of disabling).
> 
> But, if you like this approach we can talk to Blake and ask for his opinion.

It's ok to revert it back. Fixed in my latest patch!

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

https://reviewboard.mozilla.org/r/129482/#review132620

> I couldn't make this work on my machine. Copy Image As Data URI never put anything into the clipboard. Anyone can repro that?

I don't see this issue on my machine. I verifed that in www.google.com and `copy images as data uri` works great as expected.

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

https://reviewboard.mozilla.org/r/129482/#review132480

> I think hiding is a better UX than disabling. Can we do that ?

Fixed in my latest patch! There is a devtools-core PR for fixing unmatched menu item when menu is invisible. See https://github.com/devtools-html/devtools-core/pull/333#event-1042886925

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Honza, I noticed there is a tooltip issue when clicking on context menu. And so sometime we might have to click on a menu item twice to trigger the copy behavior.

This issue happens in:

1. As you hover on an image request and then image preview tooltip appears.
2. Right click to trigger context menu to click a menu like "Copy Images As Data URI"
3. Image preview tooltip will be cancel by mouse click, but copy action doesn't trigger as expected.
4. Copy behavior will be trigger till you click on "Copy Images As Data URI" again.

Comment 20

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

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

https://reviewboard.mozilla.org/r/129482/#review132860

Looks reasonable to me!

R+, but pleas look at my comments.

Thanks,
Honza

::: devtools/client/netmonitor/src/request-list-context-menu.js:45
(Diff revision 8)
>      return getSortedRequests(window.gStore.getState());
>    },
>  
>    /**
>     * Handle the context menu opening. Hide items if no request is selected.
> -   * Since visible attribute only accept boolean value but the method call may
> +   * Since disabled attribute only accept boolean value but the method call may

Shuuldn't we also revert/update the comment?

::: devtools/client/netmonitor/src/request-list-context-menu.js:87
(Diff revision 8)
>        click: () => this.copyAsCurl(),
>      });
>  
>      copySubmenu.push({
>        type: "separator",
> -      visible: !!selectedRequest,
> +      disabled: !selectedRequest,

Shouldn't this also be changed to visible?

Comment 21

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

(In reply to Ricky Chien [:rickychien] from comment #19)
> Honza, I noticed there is a tooltip issue when clicking on context menu. And
> so sometime we might have to click on a menu item twice to trigger the copy
> behavior.
> 
> This issue happens in:
> 
> 1. As you hover on an image request and then image preview tooltip appears.
> 2. Right click to trigger context menu to click a menu like "Copy Images As
> Data URI"
> 3. Image preview tooltip will be cancel by mouse click, but copy action
> doesn't trigger as expected.
> 4. Copy behavior will be trigger till you click on "Copy Images As Data URI"
> again.
Yes I can also repro this on my machine. I think that we can ignore is for now since the image preview will be removed anyway.

Honza

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

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

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129482/diff/9-10/

Comment 24

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857516 [details]
Bug 1355833 - Fix broken context menu actions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129482/diff/10-11/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3244e9568d54
Fix broken context menu actions r=Honza

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3244e9568d54

Comment 27

[Ciprian Georgiu, Desktop QA]

I verified this issue using latest Nightly 55.0a1 (2017-04-20) on Windows 10 x64. I can confirm that all context menu and sub menu actions work as expected in Net monitor Launchpad. (Copy URL, Copy as cURL, Copy Request Headers etc.)

Marking this as verified fixed.