324443 - Middle clicking a tab loads the clipboard URL into the active tab (not the one clicked on)

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Andrew Schultz]

As noted in bug 171787, middle-clicking on a tab loads the clipboard URL into the currently-focused tab, not the one that's clicked on.  To see this you need to set the pref "middlemouse.contentLoadURL" to true (default on linux).

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

->me just so I don't forget it.  I don't want to fix this myself though, so please steal the bug from me.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Two options I see are:
1. make contentAreaFoo aware of tabs (requires adding a parameter to its loadURI)
2. make the tabbrowser grab the middle click (likely requires duplicating some code from contentAreaFoo's middleMousePaste())

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

see also firefox bug 284379

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

This seems to work.

Comment 5

[[e-mail]]

Comment on attachment 212760 [details] [diff] [review]
patch

This doesn't verify that the tab is a tabbed browser tab, and it updates the URLbar for inactive tabs. It might be easier to make middle-clicking switch tab?

Comment 6

[jag (Peter Annema)]

I'd expect middle-click paste to behave the same as dragging a URL onto a tab. Preferably, neither switches to the tab if it's not the active one.

If I recall correctly, changing the URL bar value was done in case the page doesn't load so you still have the URL available to try again. Do we want to emulate this for inactive tabs?

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #5)
> (From update of attachment 212760 [details] [diff] [review] [edit])
> This doesn't verify that the tab is a tabbed browser tab

var browser;
try {
  browser = getBrowser();
} catch (ex) { }
var ot = event.originalTarget;
if (browser &&
    ot.localName == "tab" &&
    ot.parentNode.parentNode.parentNode.parentNode == browser) {

So, what's the right way to do that?

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: revised

This fixes the URL bar issue (properly, I hope), and should handle content tabs properly too.

Comment 9

[[e-mail]]

Comment on attachment 213799 [details] [diff] [review]
revised

>+    var browser;
>+    try {
>+      browser = getBrowser();
>+    } catch (ex) { }
Hmm... without this patch, what happens if you middle-click a mail message?

>+    var ot = event.originalTarget;
>+    if (browser &&
>+        ot.localName == "tab" &&
>+        ot.parentNode == browser.mTabContainer) {
I think that if the parentNode is the tab container then it's pretty likely that the local name is "tab". And based on the first question we might always expect to have a browser at this point, otherwise loadURI will fail. Or maybe we should move this logic into loadURI.

>+      ot.linkedBrowser.loadURI(url);
>+      ot.linkedBrowser.userTypedValue = url;
Possibly need to test for about:blank?

Comment 10

[jag (Peter Annema)]

If you middle-click on the active tab you'll also wanna update the urlbar value.

If you middle click in empty space in the tab bar, what happens? Looks like it'll load it into the active tab. Should that open a new tab instead, like D&D would do?

Comment 11

[[e-mail]]

(In reply to comment #10)
>If you middle-click on the active tab you'll also wanna update the urlbar value.
That code's still there, isn't it?

>If you middle click in empty space in the tab bar, what happens? Looks like it'll
>load it into the active tab. Should that open a new tab instead, like D&D would do?
Hmm... I guess you need to do event.originalTarget.ownerDocument == document, then you can check for event.originalTarget.localName == "tab", otherwise you're looking at the tab strip? Should the new tab load in the background?

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #11)
> >If you middle click in empty space in the tab bar, what happens? Looks like it'll
> >load it into the active tab. Should that open a new tab instead, like D&D would do?
> Hmm... I guess you need to do event.originalTarget.ownerDocument == document,
> then you can check for event.originalTarget.localName == "tab", otherwise
> you're looking at the tab strip? Should the new tab load in the background?

For me, drag-drop does background.  It ignores shift... I wonder if we should fix that.

Comment 13

[jag (Peter Annema)]

(In reply to comment #9)
> (From update of attachment 213799 [details] [diff] [review] [edit])
> >+    var ot = event.originalTarget;
> >+    if (browser &&
> >+        ot.localName == "tab" &&
> >+        ot.parentNode == browser.mTabContainer) {
> I think that if the parentNode is the tab container then it's pretty likely
> that the local name is "tab".

What about the anonymous content spacers and the new tab and close tab buttons?

======================================================================

(In reply to comment #11)
> (In reply to comment #10)
> >If you middle-click on the active tab you'll also wanna update the 
> >urlbar value.
> That code's still there, isn't it?

Wouldn't that now go through the bit that says "if it's a tab, set the userTypedValue"?

> >If you middle click in empty space in the tab bar, what happens? Looks 
> >like it'll load it into the active tab. Should that open a new tab 
> >instead, like D&D would do?
> Hmm... I guess you need to do event.originalTarget.ownerDocument == document,
> then you can check for event.originalTarget.localName == "tab", otherwise
> you're looking at the tab strip? Should the new tab load in the background?

Yes, it should, unless your preference of course is to switch to new tabs. And, as CTho points out, perhaps invert the default when shift is held.

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

This behaves well for middle clicking:
1. the page content
2. the current tab
3. a background tab
4. an empty part of the tab bar, and the new tab button (and maybe the close button, didn't try)

Comment 15

[[e-mail]]

Comment on attachment 214212 [details] [diff] [review]
patch

>+    var browser;
>+    try {
>+      browser = getBrowser();
>+    } catch (ex) { }
I don't think we need to try/catch this; all our windows with loadURI also have getBrowser(). Same goes for the null-checks.

>+      ot.linkedBrowser.loadURI(url);
>+      ot.linkedBrowser.userTypedValue = url;
>+      if (ot == browser.mCurrentTab && url != "about:blank") {
>+          gURLBar.value = url;
>+      }
Need to have loadURI last, in case the URI loads before you set the value.

>+      browser.addTab(url, null, null, event.shiftKey);
This isn't a child tab, just a new tab.

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 17

[[e-mail]]

Comment on attachment 214744 [details] [diff] [review]
patch

>+    var ot = event.originalTarget;
I think I'd like this to be named "tab".

>+      if (event.shiftKey ^ (pref && pref.getBoolPref("browser.tabs.loadInBackground")))
>+      if (event.shiftKey ^ (pref && pref.getBoolPref("browser.tabs.loadInBackground")))
Should be != as discussed on IRC. If you can think of a way to avoid the duplication then better still.

Comment 18

[jag (Peter Annema)]

I don't really like this trick that compares two unrelated booleans. I think the ^ was actually clearer in conveying the intent of the code.

I'd have preferred |var target = event.originalTarget;| over |tab| to make |else if (target.ownerDocument == document)| a little easier to comprehend (the word "tab" sends you down the wrong path here, since the object at this point isn't a tab). In fact, in addition to that I'd like a comment there along the lines of "check if it's in chrome or in content".

I guess that's what you get for not reviewing faster :-)