Closed Bug 848502 Opened 7 years ago Closed 5 years ago

Add a context popup to the SideMenuWidget for copying URLs

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: vporof, Assigned: miketaylr)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

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.
Priority: -- → P3
Duplicate of this bug: 946237
Duplicate of this bug: 951224
Summary: Add a context popup for the SideMenuWidget → Add a context popup for the SideMenuWidget for copying URLs
Duplicate of this bug: 1014339
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.
Duplicate of this bug: 1053987
Summary: Add a context popup for the SideMenuWidget for copying URLs → Add a context popup to the SideMenuWidget for copying URLs
Bumping priority due to number of dupes. It is clearly in high demand.
Priority: P3 → P2
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.
Assignee: nobody → miket
Attached patch 848502-1.patch (obsolete) — Splinter Review
Attached patch 848502-2.patch (obsolete) — Splinter Review
Attached patch 848502-3.patch (obsolete) — Splinter Review
OK, it helps if you read the XUL tutorial on adding keyboard shortcuts. >_>
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.
Attachment #8582561 - Flags: review?(vporof) → review+
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.
Attachment #8582562 - Flags: review?(vporof) → feedback+
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.
Attachment #8582658 - Flags: review?(vporof) → review+
(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.
(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.
Attached patch 848502-1-2.patch (obsolete) — Splinter Review
Carrying forward r+ on this patch, but updated to always take a DOM element.
Attachment #8582561 - Attachment is obsolete: true
Attached patch 848502-2-2.patch (obsolete) — Splinter Review
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).
Attachment #8582562 - Attachment is obsolete: true
Attached patch 848502-3-2.patch (obsolete) — Splinter Review
Re-requesting review on the keybinding patch because I added a simple test to it.

Thanks!
Attachment #8582658 - Attachment is obsolete: true
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.
Attachment #8583912 - Flags: review?(vporof) → review-
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.
Attachment #8583910 - Flags: review?(vporof) → review-
Attached patch 848502-2-3.patch (obsolete) — Splinter Review
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. ¯\_(ツ)_/¯)
Attachment #8583910 - Attachment is obsolete: true
Attached patch 848502-3-3.patch (obsolete) — Splinter Review
Attachment #8583912 - Attachment is obsolete: true
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.
Attachment #8584110 - Flags: review?(vporof) → review-
Attachment #8584111 - Flags: review?(vporof) → review+
Womp womp. Thanks for the suggested approach Victor--I knew my attempt with opening tabs felt weird.
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
Attached patch 848502-4.patch (obsolete) — Splinter Review
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.
Attachment #8584110 - Attachment is obsolete: true
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.
Attachment #8587805 - Flags: review?(vporof) → review+
Sweet, thanks Victor!
Attached patch 848502-1.patchSplinter Review
Rebased patch, carrying forward r+.
Attachment #8583907 - Attachment is obsolete: true
Attached patch 848502-2.patchSplinter Review
Rebased patch with feedback addressed, carrying forward r+.
Attachment #8587805 - Attachment is obsolete: true
Attached patch 848502-3.patchSplinter Review
Rebased, carrying forward r+.
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
Try is green.
Keywords: checkin-needed
Attachment #8584111 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7faa8f366cab
https://hg.mozilla.org/mozilla-central/rev/25b321b2f1ef
https://hg.mozilla.org/mozilla-central/rev/8ef4fd08d8c6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Depends on: 1159276
Product: Firefox → DevTools
Blocks: 1565711
Blocks: 1565713
No longer blocks: 1565711
No longer blocks: 1565713
You need to log in before you can comment on or make changes to this bug.