848502 - Add a context popup to the SideMenuWidget for copying URLs

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

For example, in the debugger it would be really useful to copy a source's url. A context menu for the sources is a good idea.

Comment 4

[Eddy Bruel [:ejpbruel]]

Bug 1053987 also suggested letting the keyboard shortcut for copying work when that source is selected. It's probably easy for anyone working on this to fix that while he's at it, so I'm merging that bug into this one.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Bumping priority due to number of dupes. It is clearly in high demand.

Comment 7

[Mike Taylor [:miketaylr]]

Here's some patches that add the following two context menu items in the debuggers source panel: "Copy URL" and "Open in New Tab" (seems useful) - mostly cargo culted from the netmonitor.

I've never messed with XUL keyboard shortcuts, so some guidance there would be appreciated--my naive attempt at adding a <key> element to debuggerKeys <keyset> failed.

Comment 8

[Mike Taylor [:miketaylr]]

Attachment: 848502-1.patch

Comment 9

[Mike Taylor [:miketaylr]]

Attachment: 848502-2.patch

Comment 10

[Mike Taylor [:miketaylr]]

Attachment: 848502-3.patch

OK, it helps if you read the XUL tutorial on adding keyboard shortcuts. >_>

Comment 11

[Victor Porof [:vporof][:vp]]

Comment on attachment 8582561 [details] [diff] [review]
848502-1.patch

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

Clean

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +400,5 @@
> +    if (!this._contextMenu) {
> +      return;
> +    }
> +    if (typeof this._contextMenu == "string") {
> +      this._contextMenu = this.document.getElementById(this._contextMenu);

Might be easier to just always require passing in the node itself, instead of checking the type here.

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8582562 [details] [diff] [review]
848502-2.patch

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

Very nice!

This is going to need a test though. Search for `waitForClipboard` for some examples.

::: browser/devtools/debugger/debugger-panes.js
@@ +51,5 @@
>    initialize: function() {
>      dumpn("Initializing the SourcesView");
>  
>      this.widget = new SideMenuWidget(document.getElementById("sources"), {
> +      contextMenu: document.getElementById("debuggerSourcesContextMenu"),

$("#debuggerSourcesContextMenu") is cleaner.

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 8582658 [details] [diff] [review]
848502-3.patch

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

::: browser/devtools/debugger/debugger-panes.js
@@ +836,5 @@
>    _onCopyUrlCommand: function() {
>      let selected = this.selectedItem.attachment;
> +    if (!selected && !selected.source) {
> +      return;
> +    }

Should make this part of the other patch. Or are you folding everything before landing?

I'm not sure if `source` can ever be falsy though. Might be better to remove that check, or throw if it's the case.

Comment 14

[Mike Taylor [:miketaylr]]

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> Might be easier to just always require passing in the node itself, instead
> of checking the type here.

Yeah, agreed. Will change.

(In reply to Victor Porof [:vporof][:vp] from comment #12)
> This is going to need a test though. Search for `waitForClipboard` for some
> examples.

Awesome, was just about to look into some tests. Thx.(In reply to Victor Porof [:vporof][:vp] from comment #13)
> Comment on attachment 8582658 [details] [diff] [review]
> 848502-3.patch
> 
> Review of attachment 8582658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +836,5 @@
> >    _onCopyUrlCommand: function() {
> >      let selected = this.selectedItem.attachment;
> > +    if (!selected && !selected.source) {
> > +      return;
> > +    }
> 
> Should make this part of the other patch. Or are you folding everything
> before landing?

Ah yeah, will put it in the other patch.

> I'm not sure if `source` can ever be falsy though. Might be better to remove
> that check, or throw if it's the case.

My thinking here was to prevent the command from running if the user has the call stack side menu panel open. But maybe there's a smarter way to do that.

Comment 15

[Mike Taylor [:miketaylr]]

(In reply to Mike Taylor [:miketaylr] from comment #14)
> My thinking here was to prevent the command from running if the user has the
> call stack side menu panel open. But maybe there's a smarter way to do that.

So yeah, I don't need that at all. Removing the extra source check.

Comment 16

[Mike Taylor [:miketaylr]]

Attachment: 848502-1-2.patch

Carrying forward r+ on this patch, but updated to always take a DOM element.

Comment 17

[Mike Taylor [:miketaylr]]

Attachment: 848502-2-2.patch

Addressed feedback and added two tests here, one for each context menu item.

(In reply to Victor Porof [:vporof][:vp] from comment #12)
> $("#debuggerSourcesContextMenu") is cleaner.

I decided to keep doc.gEBID because the rest of the file uses that style (26 times...). Hope that's OK. (Maybe a good first/follow up patch for someone to add a $ alias to head.js and update the debugger tests).

Comment 18

[Mike Taylor [:miketaylr]]

Attachment: 848502-3-2.patch

Re-requesting review on the keybinding patch because I added a simple test to it.

Thanks!

Comment 19

[Victor Porof [:vporof][:vp]]

Comment on attachment 8583912 [details] [diff] [review]
848502-3-2.patch

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

r- only because of the race.

::: browser/devtools/debugger/test/browser_dbg_sources-keybindings.js
@@ +27,5 @@
> +      });
> +  });
> +
> +  function testCopyURLShortcut() {
> +    waitForClipboard(sendCopyShortcut, SCRIPT_URI);

There's a race here. `closeDebuggerAndFinish(gPanel)` can start before testCopyURLShortcut finishes. Basically you want to return a promise here resolved when waitForClipboard succeeds, and rejected otherwise.

Comment 20

[Victor Porof [:vporof][:vp]]

Comment on attachment 8583910 [details] [diff] [review]
848502-2-2.patch

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

r- for the races. Everything else looks good!

::: browser/devtools/debugger/test/browser_dbg_sources-contextmenu-01.js
@@ +24,5 @@
> +        ok(false, "Got an error: " + aError.message + "\n" + aError.stack);
> +      });
> +  });
> +
> +  function testCopyMenuItem() {

This function should return a promise in order for the `.then(null, aError => {` catch above to work properly.

@@ +27,5 @@
> +
> +  function testCopyMenuItem() {
> +    let contextMenu = gDebugger.document.getElementById("debuggerSourcesContextMenu");
> +    once(contextMenu, "popupshown")
> +      .then(() => waitForClipboard(gSources._onCopyUrlCommand, SCRIPT_URI))

Nit: could synthesize the keyboard combo here instead of calling the function directly, but I guess this works too.

::: browser/devtools/debugger/test/browser_dbg_sources-contextmenu-02.js
@@ +25,5 @@
> +      });
> +  });
> +
> +  gBrowser.tabContainer.addEventListener("TabOpen", function onOpen(e) {
> +    ok(true, "A new tab has been opened.");

This is another race. The test could finish before the tab opens, in which case the check fails. You want this event listener to resolve a promise on which `closeDebuggerAndFinish(gPanel)` waits for.

@@ +30,5 @@
> +    e.target.addEventListener("load", () => gBrowser.removeCurrentTab(), false);
> +    gBrowser.tabContainer.removeEventListener("TabOpen", onOpen, false);
> +  }, false);
> +
> +  function testNewTabMenuItem() {

Ditto, you want this function to return a promise.

Comment 21

[Mike Taylor [:miketaylr]]

Attachment: 848502-2-3.patch

OK, these tests should be properly Promise-y now. Note that I added waitForClipboard to head.js that returns a resolved or rejected promise.

I'm also sending mouse events on the context menu items now, rather than calling the handlers directly.

(listenForTabOpen in -02.js feels a bit awkward to me, but seems to get the job done. ¯\_(ツ)_/¯)

Comment 22

[Mike Taylor [:miketaylr]]

Attachment: 848502-3-3.patch

Comment 23

[Victor Porof [:vporof][:vp]]

Comment on attachment 8584110 [details] [diff] [review]
848502-2-3.patch

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

::: browser/devtools/debugger/test/browser_dbg_sources-contextmenu-02.js
@@ +36,5 @@
> +      }, false);
> +      gBrowser.tabContainer.removeEventListener("TabOpen", onOpen, false);
> +
> +      return new Promise(resolve => {
> +        resolve();

This equivalent to just returning `closeDebuggerAndFinish(gPanel)`.

However, (see below)

@@ +40,5 @@
> +        resolve();
> +      }).then(() => closeDebuggerAndFinish(gPanel));
> +    }, false);
> +
> +    return new Promise(resolve => resolve());

This here is returning a promise resolved instantly, without actually waiting for anything. Creating a promise like this is completely useless.

You want something like this:


function clickOpenInNewTab() {
  return new Promise((resolve, reject) => {
    let newTabMenuItem = gDebugger.document.getElementById("debugger-sources-context-newtab");
    if (!newTabMenuItem) {
      reject(new Error("The Open in New Tab context menu item is not available."));
    }

    ok(newTabMenuItem, "The Open in New Tab context menu item is available.");

    let tabOpened = listenForTabOpen();
    EventUtils.synthesizeMouseAtCenter(newTabMenuItem, {}, gDebugger);
    tabOpened.then(resolve);
  });
}

function testNewTabMenuItem() {
  let contextMenu = gDebugger.document.getElementById("debuggerSourcesContextMenu");
  let newTabPromise = once(contextMenu, "popupshown")
    .then(clickOpenInNewTab)
    .then(testNewTabContents);

  contextMenu.openPopup(gSources.container, "overlap", 0, 0, true, false);
  return newTabPromise;
}

function testNewTabContents(tab) {
  // you probably also want to test this
}

function listenForTabOpen() {
  return new Promise(resolve => {
    gBrowser.tabContainer.addEventListener("TabOpen", function onOpen(e) {
      gBrowser.tabContainer.removeEventListener("TabOpen", onOpen, false);

      tab.addEventListener("load", function tabLoaded() {
        tab.removeEventListener("load", tabLoaded, false);

        resolve(tab);
        gBrowser.removeCurrentTab();

      }, false);
    }, false);
  });
}

::: browser/devtools/debugger/test/head.js
@@ +438,5 @@
>      }
>    });
>    return deferred.promise;
>  }
> +function waitForClipboard(setup, expected) {

Nit: rename this to waitForClipboardPromise to avoid confusion with the SimpleTest implementation.

Comment 24

[Karl Dubost💡 :karlcow]

FYI. Paper trail and feedback loop
https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/5896571-show-sources-path-and-optionally-copy-it-to-clipbo

Comment 25

[Mike Taylor [:miketaylr]]

Womp womp. Thanks for the suggested approach Victor--I knew my attempt with opening tabs felt weird.

Comment 26

[Mike Taylor [:miketaylr]]

Note to self: use the message manager to check the contents of the tab so this works out of the box for e10s: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/The_message_manager#Chrome_<->_content_communication

Comment 27

[Mike Taylor [:miketaylr]]

Attachment: 848502-4.patch

OK, I'm feeling much better about this patch (and promises in general). 

Rather than check the new tab's content (and have to mess with the message manager stuff), I'm just checking the tab URI is what we expect once it's loaded.

Comment 28

[Victor Porof [:vporof][:vp]]

Comment on attachment 8587805 [details] [diff] [review]
848502-4.patch

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

A+. A few small races still, but trivial to fix. Land away after addressing the comments below.

::: browser/devtools/debugger/test/browser_dbg_sources-contextmenu-01.js
@@ +45,5 @@
> +
> +  function openContextMenu() {
> +    let contextMenu = gDebugger.document.getElementById("debuggerSourcesContextMenu");
> +    EventUtils.synthesizeMouseAtCenter(gSources.selectedItem.prebuiltNode, {type: 'contextmenu'}, gDebugger);
> +    return once(contextMenu, "popupshown");

Still a race here (apologies if it was in the previous patches and I didn't catch it. You want to start waiting for an event before triggering it.
So:

let contextMenu = ...
let popupShown = once(contextMenu, "popupshown");
EventUtils.synthesizeMouseAtCenter(...);
return popupShown;

::: browser/devtools/debugger/test/browser_dbg_sources-contextmenu-02.js
@@ +28,5 @@
> +      });
> +  });
> +
> +  function testNewTabURI(tabUri) {
> +    return new Promise(resolve => {

Nit: creating a promise here is not really necessary.

@@ +59,5 @@
> +      }
> +
> +      ok(newTabMenuItem, "The Open in New Tab context menu item is available.");
> +      EventUtils.synthesizeMouseAtCenter(newTabMenuItem, {}, gDebugger);
> +      waitForTabOpen().then(resolve);

Here you want to call waitForTabOpen before EventUtils.synthesizeMouseAtCenter.

@@ +66,5 @@
> +
> +  function openContextMenu() {
> +    let contextMenu = gDebugger.document.getElementById("debuggerSourcesContextMenu");
> +    EventUtils.synthesizeMouseAtCenter(gSources.selectedItem.prebuiltNode, {type: 'contextmenu'}, gDebugger);
> +    return once(contextMenu, "popupshown");

Ditto here as in the other test, start waiting for the event before triggering it.

Comment 29

[Mike Taylor [:miketaylr]]

Sweet, thanks Victor!

Comment 30

[Mike Taylor [:miketaylr]]

Attachment: 848502-1.patch

Rebased patch, carrying forward r+.

Comment 31

[Mike Taylor [:miketaylr]]

Attachment: 848502-2.patch

Rebased patch with feedback addressed, carrying forward r+.

Comment 32

[Mike Taylor [:miketaylr]]

Attachment: 848502-3.patch

Rebased, carrying forward r+.

Comment 33

[Mike Taylor [:miketaylr]]

Last night I ran https://treeherder.mozilla.org/#/jobs?repo=try&revision=40485e0b4714 and it looks like browser/devtools/debugger/test/browser_dbg_variables-view-accessibility.js exposed a (Mac only, apparently) bug in _onCopyUrlCommand (in debugger-panes.js).

The fix was to change

> let selected = this.selectedItem.attachment;

to 

> let selected = this.selectedItem && this.selectedItem.attachment;

Which means we early return when someone does cmd+c and there isn't a focused script in the side menu (which is what we want, I think).

New (mac-only) try run going now @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef1ed5620a5

Comment 34

[Mike Taylor [:miketaylr]]

Try is green.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7faa8f366cab
https://hg.mozilla.org/integration/fx-team/rev/25b321b2f1ef
https://hg.mozilla.org/integration/fx-team/rev/8ef4fd08d8c6

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7faa8f366cab
https://hg.mozilla.org/mozilla-central/rev/25b321b2f1ef
https://hg.mozilla.org/mozilla-central/rev/8ef4fd08d8c6