858976 - cmd+click a link in a panel should open the link in a new tab

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

At the moment if I cmd+click a link in a panel nothing happens.  I expected the same behavior that would happen in a tab, which is that a new tab is opened for the link.

At the moment I don't think it is possible to open a link from a panel in a new tab; this should be possible.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #0)
> At the moment I don't think it is possible to open a link from a panel in a
> new tab; this should be possible.

Well the only option I see is to add a click listener to every link on the page, capture it, pass a message to main.js, and have main.js open the link in a tab.  Waay too much.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

What actually happens right now when you cmd-click a link? Does it throw an error?

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Wes Kocher (:KWierso) from comment #2)

> What actually happens right now when you cmd-click a link? Does it throw an
> error?

It doesn't do anything, nothing happens. Probably because this behavior it's supposed to works only in tab's document.

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #1)

> Well the only option I see is to add a click listener to every link on the
> page, capture it, pass a message to main.js, and have main.js open the link
> in a tab.  Waay too much.

There is a simpler approach; we can just add a listener to the panel for any click events that are bubbles up – if a code in the page prevents that, it's correct doesn't do anything.
However we're just "mimic" the original behavior for page loaded in tabs, but I guessed this bug was about it.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: pull/1448

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8398926 [details] [review]
pull/1448

I'd like to get Irakli's ok on this approach before approving it.  I don't see a better approach but maybe there is one?

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8398926 [details] [review]
pull/1448

We need to honor the `browser.tabs.loadInBackground` pref

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

I'm thinking we should file a platform bug for this, and perhaps land this patch in the meantime.

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

I just made this patch to discuss the approach; and show that we need to add a listener to every link in the page and then pass messages to main.js.
But even if the code it simpler I feel uneasy to land this patch. I don't feel honestly that this is a critical features – and therefore a P1 – that deserve this kind of "monkey patch"; it's probably better fixing the behavior directly on platform side as you suggested, and leave as is without introducing new complexity in the meantime.

But if instead it's considered important, and therefore worthy to land a patch like that, we can follow your suggestion in comment 7.

> We need to honor the `browser.tabs.loadInBackground` pref

Totally forgot that, thanks!

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> I just made this patch to discuss the approach; and show that we need to add

Of course it was "we don't need".

Comment 10

[Tomislav Jovanovic :zombie]

Erik, i believe the bug, as reported, only addresses the issue on OSX. and while technically ctrl-click does exist on windows and linux, the most common way to open in a new tab on those platforms (an analogous action) is middle-click on a link.

so if the final decision is to actually do this, i think it should include middle-click too.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #10)
> Erik, i believe the bug, as reported, only addresses the issue on OSX. and
> while technically ctrl-click does exist on windows and linux, the most
> common way to open in a new tab on those platforms (an analogous action) is
> middle-click on a link.
> 
> so if the final decision is to actually do this, i think it should include
> middle-click too.

I think that should be separate bug, unless we can hit two birds with one stone.

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tomislav Jovanovic [:zombie] from comment #10)

> Erik, i believe the bug, as reported, only addresses the issue on OSX.

I disagree; most of the laptop with touchpad doesn't have a way to do a middle click, doesn't depends by what OS is running on. So where I agree that we should probably have the middle-click option too if we land this patch – in this bug or another one – I wouldn't say that this bug only address the issue on OS X.

> so if the final decision is to actually do this, i think it should include
> middle-click too.

It seems that the "final decision" is file a platform bug, for the panel's link click issue. We're still debate if it would make sense land this fix as is in the meantime, or not.

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #11)

> I think that should be separate bug, unless we can hit two birds with one
> stone.

It can be done either in this bug or in another one. The code would basically change from:

     let accel = platform === "darwin" ? metaKey : ctrlKey;
     let isLeftClick = button === 0;
     
     if (accel && isLeftClick) {

To something like:

     let isOSX = platfrom === "darwin";
     let accel = isOSX ? metaKey : ctrlKey;
     let isLeftClick = button === 0;
     let isMiddleClick = button === 1;

     if ((accel && isLeftClick) || (isMiddleClick && !isOSX)) {

That should do the job, but I didn't check yet. The test cases of course needs to be updated too, be sure that the middle click will work only on platform that are not OS X.

I presume that if we fix that on platform side, such hacks won't be necessary – the shortcuts doesn't work 'cause panel's document are tabless, we already experience something like that.

Comment 13

[Tomislav Jovanovic :zombie]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #12)
> I disagree; most of the laptop with touchpad doesn't have a way to do a

yeah, i forgot about notebooks (i still use a mouse with one), but even if only 50% of windows are  desktops, that's still about 5x more users than on OSX.


> be sure that the middle click will work only on platform that are not OS X.

in my experience, middle-click works on osx too, and this code confirms it:

    var middle = !ignoreButton && e.button == 1;
    var middleUsesTabs = getBoolPref("browser.tabs.opentabfor.middleclick", true);

    #ifdef XP_MACOSX
      if (meta || (middle && middleUsesTabs))
    #else
      if (ctrl || (middle && middleUsesTabs))
    #endif

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#133

i think that pref is also true on OSX by default (though we should respect it either way).

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tomislav Jovanovic [:zombie] from comment #13)

> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #12)
> > I disagree; most of the laptop with touchpad doesn't have a way to do a
> 
> yeah, i forgot about notebooks (i still use a mouse with one), but even if
> only 50% of windows are  desktops, that's still about 5x more users than on
> OSX.

I'm not discussing on the number of users per OS; but that saying that even if the bug is related only to the keyboard + mouse shortcut it doesn't means automatically that "only addresses the issue on OS X".

Said that, I agree with you that we should also implement the middle-click.

> > be sure that the middle click will work only on platform that are not OS X.

> in my experience, middle-click works on osx too, and this code confirms it:

I checked on a documentation online that was clearly outdated.
Seeing this one: https://support.mozilla.org/en-US/kb/mouse-shortcuts-perform-common-tasks confirm that middle-click is supported also by OS X, plus, it seems that we should honor the `Shift` as well, especially if we following the `browser.tabs.loadInBackground` pref.

It seems to me that more we dig into all the use cases, more behaviors we have to simulate, and as I previously said I'm not sure is worthy.

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> Comment on attachment 8398926 [details] [review]
> pull/1448
> 
> I'd like to get Irakli's ok on this approach before approving it.  I don't
> see a better approach but maybe there is one?

Only other alternative I could think of is to expose those events on the panel instance
itself via `onLink` or `onClick` and let panel implementer decide what to do there. Although
I guess that's still possible via preventing propagation and sending message to panel manually.

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> Comment on attachment 8398926 [details] [review]
> pull/1448
> 
> We need to honor the `browser.tabs.loadInBackground` pref

I have mixed feelings about this. I can imagine that based on what panel does
expected behavior may be different. Anyhow following user preference is a good
default, although it maybe worth exposing an option to a panel to override that
default (we can defer exposing such an option to a next cut though).

Comment 17

[Dave Townsend [:mossop]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #14)
> I checked on a documentation online that was clearly outdated.
> Seeing this one:
> https://support.mozilla.org/en-US/kb/mouse-shortcuts-perform-common-tasks
> confirm that middle-click is supported also by OS X, plus, it seems that we
> should honor the `Shift` as well, especially if we following the
> `browser.tabs.loadInBackground` pref.
> 
> It seems to me that more we dig into all the use cases, more behaviors we
> have to simulate, and as I previously said I'm not sure is worthy.

The platform has a function you can just call with the URL and event triggering the load that will load the url in the appropriate place, we should just use that.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#83

Comment 18

[Matteo Ferretti [:zer0] [:matteo]]

I rewrote the code to use `openUILink`, only for the cases where we don't want the panel to handle them.
I'm just wondering how to modify the unit test: because now we rely on an external function to defines the behavior, the only thing we actually care is that is called in the scenario we expect; so probably the best thing it's just "spying" the function to check that is called, and that would be enough to pass the test.

Otherwise, I can leave the test as is - maybe adding the middle click too.

Irakli, what do you think?

Comment 19

[Ben McCann]

We have the same or a very similar bug and it's something I'd love to see addressed. However, it's not just ctrl+click for us but also right-clicking.

We make our own sidebar by cloning the 'sidebar-splitter' DOM element and 'sidebar-box' DOM element and appending them to the 'browser' DOM element. One big problem we have right now is that you can't ctrl+click any links or right-click anywhere in the sidebar we've created. Do you think our problem is the same as this bug? I'd be happy to email a reproducible test case to anyone willing to look at it.

Comment 20

[Dave Townsend [:mossop]]

(In reply to Ben McCann from comment #19)
> We have the same or a very similar bug and it's something I'd love to see
> addressed. However, it's not just ctrl+click for us but also right-clicking.
> 
> We make our own sidebar by cloning the 'sidebar-splitter' DOM element and
> 'sidebar-box' DOM element and appending them to the 'browser' DOM element.
> One big problem we have right now is that you can't ctrl+click any links or
> right-click anywhere in the sidebar we've created. Do you think our problem
> is the same as this bug? I'd be happy to email a reproducible test case to
> anyone willing to look at it.

ctrl-clicking is the same as this, you'll have to apply a similar fix to your sidebar. Right clicking is something different.

Comment 21

[Ben McCann]

I'm surprised that we would have to apply a separate fix to our sidebar. Wouldn't it be fixed when this issue is resolved?

Comment 22

[Dave Townsend [:mossop]]

(In reply to Ben McCann from comment #21)
> I'm surprised that we would have to apply a separate fix to our sidebar.
> Wouldn't it be fixed when this issue is resolved?

No, this patch listens for clicks in the panel document and does appropriate things with them. It won't be listening to your sidebar document.

Comment 23

[Ben McCann]

Ah, thanks for the clarification. I'm not familiar with the panel document and thought my sidebar might be an instance of a panel

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Ben McCann from comment #21)
> I'm surprised that we would have to apply a separate fix to our sidebar.
> Wouldn't it be fixed when this issue is resolved?

Which bug were you referring to? (just out of curiosity)

Comment 25

[Ben McCann]

Erik, not sure I'm following, so let me know if I didn't answer your question. When I open my custom sidebar with a webpage loaded inside it, I cannot ctrl-click links in the webpage that sidebar contains. That seemed like it might be this bug. I was hoping that my sidebar was an instance of the panel type referred to here and so when you fix cmd+click in a panel it would solve my issue.

Comment 26

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Ben McCann from comment #25)
> Erik, not sure I'm following, so let me know if I didn't answer your
> question. When I open my custom sidebar with a webpage loaded inside it, I
> cannot ctrl-click links in the webpage that sidebar contains. That seemed
> like it might be this bug. I was hoping that my sidebar was an instance of
> the panel type referred to here and so when you fix cmd+click in a panel it
> would solve my issue.

I see, we should have a bug open for that then too.  I was wondering if you knew about an existing bug for this.

Comment 27

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

To clarify ( because Ben mentioned this in bug 858399 ) I do not think this should be optional, IMO cmd+click should just work as expected.

Comment 28

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Jeff Griffiths (:canuckistani) from comment #27)
> To clarify ( because Ben mentioned this in bug 858399 ) I do not think this
> should be optional, IMO cmd+click should just work as expected.

Agreed, if we implement this, it shouldn't be optional, 'cause if we set a target from HTML in a link, it works as expected.

Comment 29

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Matteo, what is the status with this?

Comment 30

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8398926 [details] [review]
pull/1448

Does this look ok to you Irakli?

Comment 31

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8398926 [details] [review]
pull/1448

Matteo, I have r+-ed it but please address my comments in the pull, I don't think we need another review for that though.

Comment 32

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/3d2e3b3aa8a412f6b29b4247a7167655e61110b7
Bug 858976 - cmd+click a link in a panel should open the link in a new tab

https://github.com/mozilla/addon-sdk/commit/d49c98a3dee5f03d7c15593f833b1d965a757c69
Merge pull request #1448 from ZER0/panel-click/858976

fix Bug 858976 - cmd+click a link in a panel should open the link in a new tab r=@gozala