1306246 - Missing copy/select context-menu from the font panel

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

The font panel exposes interesting information that people may need to copy:
- the font name
- the URL for the font
- the @font-face CSS rule

None of these are selectable and can be copied from the panel.

Comment 1

[Sébastien Blin [:sblin] [:amarok]]

Hi!

I think I can try to work on this bug.

Note: I can select the URL for the font.

Comment 2

[Patrick Brosset <:pbro>]

Thanks! I'll assign the bug to you.
You're right, the URL can be selected, and therefore ctrl+C works, but there's no context menu.
And we also need a way to be able to select the text in the textarea that has the @font-face code.

We have a context menu in the toolbox that works well for any input field already, that you can reuse.
The way this works, is that in the font panel, you'll need to start listening for contextmenu events, and if they happen inside an input, then you can simply call this.inspector.onTextBoxContextMenu(e) and that's it!

Please experiment with this and let know if you need help.

Comment 3

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 4

[Sébastien Blin [:sblin] [:amarok]]

Hi.

I played a little bit with the code. And the infos are already selectable.

I add some listeners for contextmenu events to call this.inspector.onTextBoxContextMenu(e). But I've got a question. The menu created by _openMenu is very complete and a lot of buttons are useless. I think it's better if I create a new Menu with just one button ("Copy text") which copy the font name or the URL or the @font-face. Do you agree?

Comment 5

[Sébastien Blin [:sblin] [:amarok]]

Hmm my bad, the openMenu open a menu with just Cut/Copy/Paste.
The only problem I see is the Paste command is enabled when I click on the font-name and the @CSS-rule.
I'll correct this and write a new test later :)

Comment 6

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1306246_1.patch

Hi,
So I add the textBoxContextMenuPopup available in the font-panel and add a test in fonts/test/

I don't know if a better solution exists to simulate a right-click on a node?

Comment 7

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1306246_1.patch

it's better with the test...

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8799088 [details] [diff] [review]
1306246_1.patch

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

Thanks for the patch!
Here are some comments. Let me know if you need more explanations.

::: devtools/client/inspector/fonts/fonts.js
@@ +229,5 @@
>      s = s.cloneNode(true);
>  
> +    let fontNameSelector = s.querySelector(".font-name");
> +    fontNameSelector.textContent = font.name;
> +    fontNameSelector.addEventListener("contextmenu",

I see one problem with this approach: you're adding 3 event listeners in this function, to nodes that are actually being removed later in the `clear` function. Whenever the font panel is updated (see `update` function), the whole UI is cleared (using the `clear` function), and that just empties the HTML of the container. And then the new elements are created.

I see that you've added removeEventListeners to the `destroy` function, but this only gets called when devtools is closed, not everytime the panel is updated.

So I'd to propose another approach:
- Add just one contextmenu event listener on the whole UI container
- Give it a callback, say, `_onContextMenu`
- When the callback is executed, check `e.target` to make sure the event happened on either `.font-name` or `.font-url` or `.font-css-code`
- If `e.target` is correct, then just call `this.inspector.onTextBoxContextMenu()`, otherwise don't do anything
- In `destroy`, remove the listener.

This way, you need only 1 listener, and it stays at all times, it doesn't need to be added again after an update. Also, the code should be simpler this way.

::: devtools/client/inspector/fonts/test/browser.ini
@@ +18,4 @@
>  [browser_fontinspector_edit-previews.js]
>  [browser_fontinspector_edit-previews-show-all.js]
>  [browser_fontinspector_theme-change.js]
> +[browser_fontinspector_copy-infos.js]

We try to keep these lists ordered alphabetically, so please move this entry up and put it just below [browser_fontinspector.js]

::: devtools/client/inspector/fonts/test/browser_fontinspector_copy-infos.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +// Test that correct previews are shown if the text is edited after 'Show all'

This comment is obsolete now, it probably comes from a copy/paste from another test. Please update it.

@@ +12,5 @@
> +  let { inspector, view } = yield openFontInspectorForURL(TEST_URI);
> +  let viewDoc = view.chromeDoc;
> +
> +  let s = viewDoc.querySelectorAll("#all-fonts > section");
> +  let section = s[0];

You don't need the `s` variable anywhere else, so you could simply do this instead:

let section = viewDoc.querySelector("#all-fonts > section");

which returns the first element that matches the selector.

@@ +15,5 @@
> +  let s = viewDoc.querySelectorAll("#all-fonts > section");
> +  let section = s[0];
> +
> +  info("Selecting font-name info");
> +  let boolIsCopyMenuItem = false;

We don't usually prefix variable names with their types in the devtools codebase, so in order to be consistent with the rest of the code, I suggest renaming this simply to isCopyMenuItem.

@@ +21,5 @@
> +    if (inspector.toolbox.textBoxContextMenuPopup.children[c].id === "cMenu_copy") {
> +      boolIsCopyMenuItem = true;
> +    }
> +  }
> +  ok(boolIsCopyMenuItem, "The copy item is present in textBoxContextMenuPopup");

Why does your test need to check if the copy menu item exists? I don't think it should. Your test is only concerned with checking that the menu does show when some elements receive a contextmenu event. You don't really need to check the content of the menu, I think.

@@ +30,5 @@
> +  let fontNameRightClick = eltFontName.ownerDocument.createEvent("MouseEvents");
> +  fontNameRightClick.initMouseEvent("contextmenu", true, true,
> +     eltFontName.ownerDocument.defaultView, 1, 0, 0, 0, 0, false,
> +     false, false, false, 2, null);
> +  eltFontName.dispatchEvent(fontNameRightClick);

No need to create/init/dispatch events yourself, the test harness comes bundled with an event library.
In this case, you can use something like:

EventUtils.synthesizeMouseAtCenter(eltFontName, {type: "contextmenu", button: 2}, viewDoc.defaultView);

@@ +31,5 @@
> +  fontNameRightClick.initMouseEvent("contextmenu", true, true,
> +     eltFontName.ownerDocument.defaultView, 1, 0, 0, 0, 0, false,
> +     false, false, false, 2, null);
> +  eltFontName.dispatchEvent(fontNameRightClick);
> +  ok(inspector.toolbox.textBoxContextMenuPopup.state === "showing",

let onContextMenuPopup = once(textboxContextMenu, "popupshown");
EventUtils.synthesizeMouseAtCenter(eltFontName, {type: "contextmenu", button: 2}, viewDoc.defaultView);
yield onContextMenuPopup;

Same comment about using EventUtils and waiting for the menu to be shown below in the other parts of this test.

::: devtools/client/inspector/inspector.js
@@ +1125,4 @@
>        }));
>      }
>  
> +    menu.append(new MenuItem({

What is this new menu item? It looks like something you forgot to remove perhaps?

Comment 9

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1306246_1.patch

Hi!

I updated the patch. I totally agree with the approach you proposed.

(In reply to Patrick Brosset <:pbro> from comment #8)
> Comment on attachment 8799088 [details] [diff] [review]
> 1306246_1.patch
> 
> Review of attachment 8799088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +21,5 @@
> > +    if (inspector.toolbox.textBoxContextMenuPopup.children[c].id === "cMenu_copy") {
> > +      boolIsCopyMenuItem = true;
> > +    }
> > +  }
> > +  ok(boolIsCopyMenuItem, "The copy item is present in textBoxContextMenuPopup");
> 
> Why does your test need to check if the copy menu item exists? I don't think
> it should. Your test is only concerned with checking that the menu does show
> when some elements receive a contextmenu event. You don't really need to
> check the content of the menu, I think.

I wrote this test because the context menu is here to copy a value. 

> 
> @@ +30,5 @@
> > +  let fontNameRightClick = eltFontName.ownerDocument.createEvent("MouseEvents");
> > +  fontNameRightClick.initMouseEvent("contextmenu", true, true,
> > +     eltFontName.ownerDocument.defaultView, 1, 0, 0, 0, 0, false,
> > +     false, false, false, 2, null);
> > +  eltFontName.dispatchEvent(fontNameRightClick);
> 
> No need to create/init/dispatch events yourself, the test harness comes
> bundled with an event library.
> In this case, you can use something like:
> 
> EventUtils.synthesizeMouseAtCenter(eltFontName, {type: "contextmenu",
> button: 2}, viewDoc.defaultView);

Cool, I didn't see this function. 
> ::: devtools/client/inspector/inspector.js
> @@ +1125,4 @@
> >        }));
> >      }
> >  
> > +    menu.append(new MenuItem({
> 
> What is this new menu item? It looks like something you forgot to remove
> perhaps?

Yeap, my bad.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8801349 [details] [diff] [review]
1306246_1.patch

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

Thanks for the updated patch. This looks really good. Just a couple of last changes and we should be good to go.
Are you able to push to TRY to test this change, or do you need me to? (https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try)

::: devtools/client/inspector/fonts/fonts.js
@@ +36,4 @@
>      this.previewInput.addEventListener("input", this.previewTextChanged);
>      this.previewInput.addEventListener("contextmenu",
>        this.inspector.onTextBoxContextMenu);
> +    this.chromeDoc.addEventListener("contextmenu", this._onContextMenu.bind(this));

One small problem with this: `this._onContextMenu.bind(this)` will return a new function every time, so here you are calling `addEventListener` with one function, and later, you are calling `removeEventListener`, with another (different) function, because you call bind again there.
Therefore, the event listener is not being removed, since it can't find the same function.

Therefore, you need to first bind the function and then use the same bound function in both `addEventListener` and `removeEventListener`:

First, in `init`, next to where we do other function bindings, at the top:
this._onContextMenu = this._onContextMenu.bind(this);

Then, later in `init`:
this.chromeDoc.addEventListener("contextmenu", this._onContextMenu);

Finally, in `destroy`:
this.chromeDoc.removeEventListener("contextmenu", this._onContextMenu);

One last request: we also have this inside `init`:
this.previewInput.addEventListener("contextmenu", this.inspector.onTextBoxContextMenu);

Well, with your new listener, you could remove it and instead, inside the new `_onContextMenu`, also accept events for #font-preview-text-input.

@@ +254,5 @@
> +
> +  _onContextMenu: function (e) {
> +    if (e.target.className === "font-name" ||
> +        e.target.className === "font-url" ||
> +        e.target.className === "font-css-code") {

As said before, you could change this condition to also accept right clicks inside the preview text input:

e.target.id === "font-preview-text-input"

Comment 11

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1306246_1.patch

Hi!

I made some changes as requested.

By the way, I don't think I'm level 1 to push to try, so if you can do the job :).

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8801939 [details] [diff] [review]
1306246_1.patch

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

Sorry, a couple last comments to be addressed and then we should be good to go.
Once you do attach a new patch, I'll push it to TRY. But in the meantime, you should probably at least run the tests locally to make sure we didn't break anything too badly.
I suggest running the font-inspector tests only, with:

mach mochitest devtools/client/inspector/fonts/test/

If you see no errors at the end in the summary, then we're good to go.

::: devtools/client/inspector/fonts/fonts.js
@@ +24,4 @@
>  FontInspector.prototype = {
>    init: function () {
>      this.update = this.update.bind(this);
> +    this._onContextMenu = this._onContextMenu.bind(this);

We don't have a good coding guideline for this but there are no other methods in this file starting with a _ character. So even if I'd love to have a consistent format for this across the whole codebase, I think the most important thing is to be consistent with the file, at least.
Sorry for not saying this in a previous review, I missed it.
So, just: onContextMenu instead of _onContextMenu.

@@ +35,4 @@
>      this.previewTextChanged = this.previewTextChanged.bind(this);
>      this.previewInput = this.chromeDoc.getElementById("font-preview-text-input");
>      this.previewInput.addEventListener("input", this.previewTextChanged);
> +    this.previewInput.addEventListener("contextmenu", this._onContextMenu);

One simplification can be done I think. The previewInput is part of the chromeDoc document, so you don't need to listen for contextmenu events on it.
Having just this:
this.chromeDoc.addEventListener("contextmenu", this._onContextMenu);
is enough to listen to contextmenu events everywhere in this document.

@@ +61,5 @@
>      this.inspector.sidebar.off("fontinspector-selected", this.onNewNode);
>      this.inspector.selection.off("new-node-front", this.onNewNode);
>      this.showAllLink.removeEventListener("click", this.showAll);
>      this.previewInput.removeEventListener("input", this.previewTextChanged);
> +    this.previewInput.removeEventListener("contextmenu", this._onContextMenu);

Therefore you should be able to remove this too.

Comment 13

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1306246_1.patch

> I suggest running the font-inspector tests only, with:
> 
> mach mochitest devtools/client/inspector/fonts/test/

I always run a mach test before proposing a patch ;)
(72 passed, 0 failures here)

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8802612 [details] [diff] [review]
1306246_1.patch

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

Looks great, thanks.
Here's a TRY push to make sure nothing is broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60fa1bcf215288c3451905008df397cc9c765f26

Comment 15

[Patrick Brosset <:pbro>]

Unfortunately 2 years have passed and we haven't landed this. I am not sure how this fell through the cracks, but the font panel has changed a lot since then.
The text is selectable, so at least people can copy by doing so.
Font URLs also have a copy icon next to them.
Having a context menu to copy would be a nice to have.

Resetting the assignee field for now. Feel free to pick up again.