1159725 - Links in markup-view attributes should open with middle-click or a similar shortcut

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

With bug 921102, we now have links for URLs in the markup-view. Opening those links require to use the contextual-menu (single click on the URL isn't possible since that is supposed to focus the attribute field for keyboard navigation, and double-click turns it to edit mode).

This is a proposal to follow the link on middle-click. Or alt-click? ctrl? meta? whatever?

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #0)

> This is a proposal to follow the link on middle-click. Or alt-click? ctrl?
> meta? whatever?

In Jetpack we say 'accel', meaning 'Control' on Windows and Linux, and 'Command' | 'Apple' on Macs. It feels natural to me to use this modifier because it's the same one I use to open links in a new tab in web content.

Comment 2

[Sebastian Zartner [:sebo]]

(In reply to Jeff Griffiths (:canuckistani) from comment #1)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #0)
> 
> > This is a proposal to follow the link on middle-click. Or alt-click? ctrl?
> > meta? whatever?
> 
> In Jetpack we say 'accel', meaning 'Control' on Windows and Linux, and
> 'Command' | 'Apple' on Macs. It feels natural to me to use this modifier
> because it's the same one I use to open links in a new tab in web content.

It's also the same one used in Firebug. To be precise, Firebug supports both, middle-click and Accel+click.

Sebastian

Comment 3

[John Pavlicek]

bgrins suggested this would be a good next bug for me to work on (Thanks, Brian).  I'll need some help to point me in the correct direction, but I'd like to take this one on.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to John Pavlicek from comment #3)
> bgrins suggested this would be a good next bug for me to work on (Thanks,
> Brian).  I'll need some help to point me in the correct direction, but I'd
> like to take this one on.
Thanks for your interest, here are some pointers:

- links in markup-view attributes are created here: https://github.com/mozilla/gecko-dev/blob/4f556f742675358b0aaeeb1b6f711ce89458e33a/browser/devtools/markupview/markup-view.js#L2718
so it's just a <span> with 2 data attributes: data-type and data-link. If you find a span with these attributes you know that a) it's a link b) the type of resource it points to and c) the actual url or file name to go to.

- right now, these links are only used from the inspector-panel's context menu, here: https://github.com/mozilla/gecko-dev/blob/4f556f742675358b0aaeeb1b6f711ce89458e33a/browser/devtools/inspector/inspector-panel.js#L768
Every time the context menu is shown, the code checks if the right-click happened on a link element and based on this hides/shows/modifies the items in the menu to do the right thing.

- the actual method that then follows the link is here: https://github.com/mozilla/gecko-dev/blob/4f556f742675358b0aaeeb1b6f711ce89458e33a/browser/devtools/inspector/inspector-panel.js#L1106

- there's a mousedown handler in the markup-view that gets executed when you click somewhere on a node:
https://github.com/mozilla/gecko-dev/blob/4f556f742675358b0aaeeb1b6f711ce89458e33a/browser/devtools/markupview/markup-view.js#L1885
this could be used to detect whether the event occurred on a linkified attribute and then followAttributeLink could be modified a little bit so it can be called from here.

Comment 5

[John Pavlicek]

I've been reading through the code trying to understand it, and I think I have a general understanding.  The general idea of what we want to do is modify the mousedown handler so that it executes the method for following the link in inspector-panel.js when the user middle-clicks (or similar).  I think I've correctly figured out that the mousedown handler needs a check on event.button === 1 for the middle click.  This is just an additional check in the handler and doesn't negate any other behavior, correct?

Assuming that's correct, I think I want to pass the inspector.followAttributeLink method the event.target, but I don't really know what changes I need to make to do this.  Can you point me in the right direction?

Comment 6

[Patrick Brosset <:pbro>]

(In reply to John Pavlicek from comment #5)
> I've been reading through the code trying to understand it, and I think I
> have a general understanding.  The general idea of what we want to do is
> modify the mousedown handler so that it executes the method for following
> the link in inspector-panel.js when the user middle-clicks (or similar).  I
> think I've correctly figured out that the mousedown handler needs a check on
> event.button === 1 for the middle click.
Yes you're understanding is correct so far. One detail though, I think we want to support both middle-click and accel-click.
> This is just an additional check
> in the handler and doesn't negate any other behavior, correct?
Correct.
> Assuming that's correct, I think I want to pass the
> inspector.followAttributeLink method the event.target, but I don't really
> know what changes I need to make to do this.  Can you point me in the right
> direction?
So, yes, one way would be to pass event.target to followAttributeLink.
This method, as is, doesn't expect it though, its first 2 lines get the link and its type from `this.panelDoc.popupNode` which points to the node that received the contextmenu click. That's because this method is only used today when using the context menu.
So, you could add an optional argument to this method which the mousedown handler in markup-view.js would pass (event.target). followAttributeLink would first check if it has a DOMNode argument, and if not, defaults to using this.panelDoc.popupNode.

Comment 7

[John Pavlicek]

I've begun by trying to add the dummy argument to followAttributeLink like this:

===========================================================
followAttributeLink: function(e, exttarget) {

    if (exttarget === undefined) {
      let type = this.panelDoc.popupNode.dataset.type;
      let link = this.panelDoc.popupNode.dataset.link;
    }
...
===========================================================

I must have done something wrong because, clicking on the menu item no longer opens up a browser window for a URL.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to John Pavlicek from comment #7)
> followAttributeLink: function(e, exttarget) {
> 
>     if (exttarget === undefined) {
>       let type = this.panelDoc.popupNode.dataset.type;
>       let link = this.panelDoc.popupNode.dataset.link;
>     }
> ...
You're defining type and link as 2 block-scope variables inside an if block here, so they're not going to be visible outside that block. This is probably the reason why the menu stopped working.
When you're having problems like this, always remember to look in your terminal window for js errors, you can also look at the "browser console" (ctrl/cmd+shift+J) as it shows errors too.
This should, in most cases, give you the error, the stack trace, and a reference to the file+line where it occurred.
You can also use the "browser toolbox" to debug a problem if you need to (Tools menu/Web developer/Browser toolbox). The browser toolbox is another instance of the devtools, running in a separate process that attaches to the browser itself, so you can debug things happening in the devtools.

I think what you want to do here is something like this:

In inspector.xul, there are 2 <menuitem> elements that call followAttributeLink and copyAttributeLink directly. Let's change that so they call 2 new method names like: onFollowLink and onCopyLink.

In inspector-panel.js, create these 2 methods, and make them use this.panelDoc.popupNode.dataset to get the link and type and use those to call the actual followAttributeLink and copyAttributeLink methods with.

Now, change both of these methods' arguments to (type, link) so that they become easier to use from other modules.

In markup-view.js, on mousedown, if the meta key is pressed or the middle button is used, then call followAtributeLink(type, link), where type and link are retrieved from event.target.dataset.type and event.target.dataset.link respectively.

Comment 9

[John Pavlicek]

Attachment: Proposed patch

Here's my first attempt at implementing all the changes.  Once I had the Browser Toolbox, that made life much easier in learning what was going on.

If this looks good, I think my next step is the test cases.  I think we'll want to test both new methods of opening the link.  Should this go in the existing test for right-click and menu selection or create a new test?

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8624882 [details] [diff] [review]
Proposed patch

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

That's a great start. Definitely on the right track, and the code changes only need a few minor tweaks.
In terms of tests, you should take a look at the existing tests first:
browser/devtools/markupview/test/browser_markupview_links_*
There's a bunch of them there, first make sure they still pass with your patch applied, and then we need a new one for simulating the middle click and meta-click.

::: browser/devtools/inspector/inspector-panel.js
@@ +1134,5 @@
> +
> +  /**
> +   * This method is here for the benefit of opening links from either the 
> +   * node-menu-link-follow menu item or a middle/ctrl+click directly in the 
> +   * inspector. It's behavior depends on which node was clicked.

I think this comment needs a bit of rephrasing. Now that this method has been made more generic, I don't think it's worth listing its callers anymore. And also, its behavior does not depend on which node was clicked anymore, it only depends on the input parameters.
What about this:

Given a type and link found in a node's attribute in the markup-view, attempt to follow that link (which may result in opening a new tab or opening the style editor or debugger).

@@ +1136,5 @@
> +   * This method is here for the benefit of opening links from either the 
> +   * node-menu-link-follow menu item or a middle/ctrl+click directly in the 
> +   * inspector. It's behavior depends on which node was clicked.
> +   */
> +  followAttributeLink: function(type, link, e) {

The 3rd argument (e) isn't used, it should be removed from the declaration.

Also we now need this method to do some input validations to ensure that type and link are passed, this should be enough:

if (!type || !link) {
  return;
}

@@ +1177,5 @@
> +  },
> +
> +  /**
> +   * This method is here for the benefit ofcopying Links. It's behavior depends
> +   * on which node was right-clicked when the menu was opened.

Same remark as for the followAttributeLink comment. Let's try and make this more generic now that the method can be called from anywhere as long as we have a link.

@@ +1179,5 @@
> +  /**
> +   * This method is here for the benefit ofcopying Links. It's behavior depends
> +   * on which node was right-clicked when the menu was opened.
> +   */
> +  copyAttributeLink: function(link, e) {

You should get rid of the 2nd (e) argument.

::: browser/devtools/markupview/markup-view.js
@@ +1889,5 @@
>      if (target.nodeName === "button") {
>        return;
>      }
>  
> +    if ((event.button === 1) || ((event.button === 0) && (event.ctrlKey)))

I think this condition should be:
event.button === 1 || event.metaKey

We want the meta key to be used (ctrl on windows/linux and cmd on mac), and if it's used, we don't really care which mouse button was used.

Also, I think this new block should be further down in the _onMouseDown function, probably just before starting the drag.
Otherwise there may be problems with followAttributeLink being called several times in a row.

Comment 11

[John Pavlicek]

Regarding the bit about metaKey v. ctrlKey.  It's my understanding that, in Windows, this the the Windows key, not Ctrl.  I will say that the Windows key is very annoying, since, in Windows 8, at least, it causes the home screen to be shown when the key is released.  It does open a new tab, though.  How should this be handled?

Comment 12

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #10)
> ::: browser/devtools/markupview/markup-view.js
> @@ +1889,5 @@
> >      if (target.nodeName === "button") {
> >        return;
> >      }
> >  
> > +    if ((event.button === 1) || ((event.button === 0) && (event.ctrlKey)))
> 
> I think this condition should be:
> event.button === 1 || event.metaKey
> 
> We want the meta key to be used (ctrl on windows/linux and cmd on mac), and
> if it's used, we don't really care which mouse button was used.
> 
> Also, I think this new block should be further down in the _onMouseDown
> function, probably just before starting the drag.
> Otherwise there may be problems with followAttributeLink being called
> several times in a row.

Patrick, note that despite its documentation[1] metaKey is always false on Windows, so you need to check ctrlKey. Also I believe it's ok to restrict the Ctrl + click to the left mouse button as that's how it works URLs in Firebug and Firefox on Windows already. I can't test right now how the behavior is on Mac, though for consistency it should be the same as for opening links in Firefox.
So I believe the condition may actually look like this:

if ((event.button === 1) || ((event.button === 0) && (event.metaKey || event.ctrlKey)))

Sebastian

[1] https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

Comment 13

[Patrick Brosset <:pbro>]

Thanks Sebastian, I was not aware of this. Also, ok for checking that button===0 when meta/ctrl are pressed.
So let's go for:
if ((event.button === 1) || ((event.button === 0) && (event.metaKey || event.ctrlKey)))

Comment 14

[John Pavlicek]

Attachment: Patch

I think that I have the code working, but I'm struggling with setting up the tests.  I've included the beginnings of my test.  It's a lot like browser_markupview_links_05.js, except I've added MouseEvents to it to perform the actual click.  I'm not happy with where I have this going, at the moment, because, if you look at the first test, you'll see that I have large bit of copy/paste for the middle-click v. ctrl/meta+left click.  I've tried pulling this into a function, like the mouse click is, but, every time I do that, the tests simply do not run.  The part that I'm trying to put into the function is this:

-----------------------------------

  let onTabOpened = once(gBrowser.tabContainer, "TabOpen");
  performMouseDown(popupNode, false);
  let {target: tab} = yield onTabOpened;
  yield waitForTabLoad(tab);

  ok(true, "A new tab opened");
  is(tab.linkedBrowser.currentURI.spec, TEST_URL_ROOT + "doc_markup_tooltip.png",
    "The URL for the new tab is correct");
  gBrowser.removeTab(tab);

-----------------------------------------

where I pass the function the popupNode and true/false for which type of click I'm performing, just like "performMouseDown" does.  Nothing happens, and I don't even see the "info" function being called when looking at the output.

Any ideas?

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8628486 [details] [diff] [review]
Patch

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

This looks mostly good, but I'll need to take one more look at the test once you submit a new patch.

::: browser/devtools/markupview/markup-view.js
@@ +1905,5 @@
>        event.preventDefault();
>      }
>  
> +    if ((event.button === 1) || ((event.button === 0) && (event.metaKey || event.ctrlKey)))
> +    {

minor formatting remark: please put { on the same line as the if, and try and make sure lines are no longer than 80 chars. You'd probably be better off re-formatting the if like:

if (event.button === 1 ||
    (event.button === 0 && (event.metaKey || event.ctrlKey)) {
  ...
}

::: browser/devtools/markupview/test/browser_markupview_links_07.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that the clicking on links in attributes actually do the right things.

nit: please rephrase to something more self-explanatory, these descriptions are useful.
// Tests that a middle-click or meta/ctrl-click on an attribute link in the markup-view actually follows that link.

@@ +13,5 @@
> +
> +  info("Select a node with a URI attribute");
> +  yield selectNode("video", inspector);
> +
> +  info("Set the popupNode to the node that contains the uri");

The word popupNode means the anchor element that was right-clicked on when a contextual menu appears. In your new test, there's no contextual menu at all, what you are dealing with here is just an element that you want to click on. So this is just a naming problem, but I think you should rephrase that comment and change the variable name:

info("Find the link element from the markup-view");
let {editor} = yield getContainerForSelector("video", inspector);
let linkEl = editor.attrElements.get("poster").querySelector(".link");

@@ +17,5 @@
> +  info("Set the popupNode to the node that contains the uri");
> +  let {editor} = yield getContainerForSelector("video", inspector);
> +  let popupNode = editor.attrElements.get("poster").querySelector(".link");
> +
> +  info("Follow the link and wait for the new tab to open");

Rephrase to something like:
info("Attempt to follow the link by middle-clicking on it, and wait for the new tab to open");

It's important that all info(...) in a test contain unique strings, because it makes it far easier to follow what the test does by reading logs (and we do this often when tests start to fail).

@@ +28,5 @@
> +  is(tab.linkedBrowser.currentURI.spec, TEST_URL_ROOT + "doc_markup_tooltip.png",
> +    "The URL for the new tab is correct");
> +  gBrowser.removeTab(tab);
> +
> +  info("Follow the link and wait for the new tab to open");

This block indeed seems to be duplicated in the file. Extracting it in a function should work fine. Maybe the problem you were having is that you didn't make the new function a generator (so it can yield)?

function* followLinkAndWaitForTab(linkEl, isMetaClick, expectedTabURI) {
  let onTabOpened = once(gBrowser.tabContainer, "TabOpen");
  performMouseDown(linkEl, isMetaClick);
  let {target} = yield onTabOpened;
  yield waitForTabLoad(target);
  ok(true, "A new tab opened");
  is(target.linkedBrowser.currentURI.spec, expectedTabURI,
     "The URL for the new tab is correct");
  gBrowser.removeTab(target);
}

@@ +85,5 @@
> +  return def.promise;
> +}
> +
> +function performMouseDown(popupNode, metactrl) {
> +  let evt = popupNode.ownerDocument.createEvent('MouseEvents');

nit: Please use only double-quotes.
For info, we started using ESLint to ensure style consistency in the code, you can read about this more at: https://wiki.mozilla.org/DevTools/CodingStandards
I strongly encourage you to install ESLint locally and check if your code changes comply to the rules.

Comment 16

[Patrick Brosset <:pbro>]

Clearing the NI? flag since I reviewed the patch.

Comment 17

[John Pavlicek]

Attachment: b1159725.patch

I've updated the code, hopefully I've got the formatting right.  Thanks for pointing me to ESlint.  I know some of those (brace on next line) are habits from other code I've worked in the past so I don't think of them.

The markupvew_links_07.js test should now test all three options with both a middle- and ctrl/meta-click.  It was that I hadn't set them up as generators, since I had no idea what that meant.  It makes sense, now.

Also, I had to update markupvew_links_05.js and markupvew_links_06.js to use the new onFollowLink function instead of the followAttributeLink function.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8629328 [details] [diff] [review]
b1159725.patch

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

This looks good to me, I only have a few minor remarks below.
Do you have push access to TRY? If not, please needinfo me on the bug so I can push for you.

::: browser/devtools/inspector/inspector-panel.js
@@ +1137,5 @@
> +   * to follow that link (which may result in opening a new tab, the style editor
> +   * or debugger).
> +   */
> +  followAttributeLink: function(type, link) {
> +

nit: get rid of this extra empty line.

::: browser/devtools/markupview/markup-view.js
@@ +1905,5 @@
>        event.preventDefault();
>      }
>  
> +    if ((event.button === 1) ||
> +        ((event.button === 0) && (event.metaKey || event.ctrlKey))) {

There are some extra parens that aren't needed here. Also, for the benefit of making the code more readable, I would do something like:

let isMiddleClick = event.button === 1;
let isMetaClick = event.button === 0 && (event.metaKey || event.ctrlKey);

if (isMiddleClick || isMetaClick) {
 ....
}

@@ +1931,3 @@
>        this.markup.indicateDragTarget(this.elt.nextElementSibling ||
> +                                     this.markup.getContainer(
> +                                         this.node.parentNode()).closeTagLine);

This looks like an unrelated formatting change. I'd advise changing it back, even if it's an improvement, it's better to do these kinds of things in separate patches or separate bugs. Otherwise this may be a source of confusion when looking at the file change history.

::: browser/devtools/markupview/test/browser_markupview_links_07.js
@@ +29,5 @@
> +
> +  info("Select a node with a IDREF attribute");
> +  yield selectNode("label", inspector);
> +
> +  info("Find the link element from the markup-view contains the ref");

Find the link element from the markup-view *that* contains the ref

@@ +42,5 @@
> +
> +  info("Select a node with an invalid IDREF attribute");
> +  yield selectNode("output", inspector);
> +
> +  info("Find the link element from the markup-view contains the ref");

same comment here

Comment 19

[John Pavlicek]

Attachment: Proposed patch

I don't have push access to TRY, so I'd need you to do that for me.

This update should take care of the previous comments.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: b1159725.patch

Thanks for this last update. I rebased the patch (which wouldn't apply cleanly on fx-team anymore), and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=743f2c4efff5

Comment 21

[John Pavlicek]

It looks like it failed one of the tests, though reading the related bug, I'm not sure if this was expected or not.  Is there anything further that needs to be done?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to John Pavlicek from comment #21)
> It looks like it failed one of the tests, though reading the related bug,
> I'm not sure if this was expected or not.  Is there anything further that
> needs to be done?

Looks like the failure is in layout/forms/test/test_bug665540.html, which is unrelated to your changes.  No need to do anything with that.  You can mark the checkin-needed keyword if you are ready to have it checked in.

Comment 23

[John Pavlicek]

Check-in should have been added, but I have a question about how the rebase works

I tried reading online about it, but I couldn't figure it out at all.  I wanted to be able to recreate this so I knew how to do it in the future.  What I tried to do was pop my patch off (hg qpop), pull the updated repository down (hg pull -u), rebase the old changeset parent to the new (hg rebase -s 249399 -d 253897).  This just gave me an error about the ancestry, which I'm not surprised about, but I couldn't figure out what I was supposed to have done.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to John Pavlicek from comment #23)
> I tried reading online about it, but I couldn't figure it out at all.  I
> wanted to be able to recreate this so I knew how to do it in the future. 
> What I tried to do was pop my patch off (hg qpop), pull the updated
> repository down (hg pull -u), rebase the old changeset parent to the new (hg
> rebase -s 249399 -d 253897).  This just gave me an error about the ancestry,
> which I'm not surprised about, but I couldn't figure out what I was supposed
> to have done.

You don't need to do an hg rebase, you can just `hg qpush` the patch back on on after you do the `pull -u`.  If there is a rejection, then it will store it in a .rej file and it can be manually resolved and then refreshed with `hg qref`

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ce485b16a186

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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