586234 - When middle clicking links in popups, open the new tab in a full browser window

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alice0775 White]

Attachment: testcase

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre ID:20100810040309
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre ID:20100810040309

In Popup window,
The behavior of the tabstrip is different from Firefox 3.6.x.

When I open in new tab(middle-click and/or Ctrl+click link nor Alt+Enter in LocationBar) on popup window, tab strip does not appears.

I think the tabstrip should be appears and switching of tab must be possible.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with NEW Profile
2. Open testcase
3. Click link
4. Middle-click and/or Ctrl+click link nor Alt+Enter in LocationBar

Actual Results:
 Tabstrip does not appears though the new tab is opened.

Expected Results:
 Tabstrip ahould be appear as same as Firefox3.6.x.


Steps to Reproduce in real world:
1. Start Minefield with NEW Profile +  Flash Player
2. Open URL ( http://www.einslive.de/ )
3. Click "WEBRADIO" at the top o page to open popup
4. Middle-click a link(HTML-Version, etc.) at bottom of popup window
   OR KeyPress Alt+Enter in the LocationBar

Actual Results:
 Tabstrip does not appears though the new tab is opened.
 You can confirm when click X button of window control.

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/528a9b4f475d
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100317 Minefield/3.7a4pre ID:20100317002903
Fails:
http://hg.mozilla.org/mozilla-central/rev/11798edae90d
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100317 Minefield/3.7a4pre ID:20100317003949
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=528a9b4f475d&tochange=11798edae90d
Candidate Bug:
Bug 347930 - Tab strip should be a toolbar instead

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is a big loss of functionality. Asking for blocking2.0.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

We should actually disable the function to force a link from a popup into a new tab, instead. I don't think showing a tabstrip is a great UX, either. Team UX: agree?

Not sure that this blocks, to be honest. Not great, but this should not be expected functionality.

Comment 6

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #4)
> We should actually disable the function to force a link from a popup into a new
> tab, instead. I don't think showing a tabstrip is a great UX, either. Team UX:
> agree?

If I understand this correctly: Are you asking what should happen if you cmd-click or middle-click a link that results in a pop-up? It should just work as a normal pop-up in my opinion, but my understanding of the description of the problem might be wrong.

(in several other locations we let middle-click/cmd-click fall back to normal click if we can't do what's being asked for, or if the middle-click has undefined behavior (e.g. mid-clicking the back button)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> If I understand this correctly: Are you asking what should happen if you
> cmd-click or middle-click a link that results in a pop-up?

No - the question is what should happen when someone is *in* a popup that has no tab bar (e.g. the one opened by clicking the link in the testcase), and then requests a new tab using e.g. middle click.

Comment 8

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #7)
> No - the question is what should happen when someone is *in* a popup that has
> no tab bar (e.g. the one opened by clicking the link in the testcase), and then
> requests a new tab using e.g. middle click.

Ah, thanks. Yeah, showing a tabstrip in the tiny pop-up window isn't going to be helpful. I'd redirect it to the underlying window if possible — if not, maybe disable it like beltzner suggests.

Comment 9

[msth67]

(In reply to comment #7)
> (In reply to comment #6)
> > If I understand this correctly: Are you asking what should happen if you
> > cmd-click or middle-click a link that results in a pop-up?
> 
> No - the question is what should happen when someone is *in* a popup that has
> no tab bar (e.g. the one opened by clicking the link in the testcase), and then
> requests a new tab using e.g. middle click.

This may sound dumb,but wouldn't the behavior of Firefox 3.6 be adequate,i.e. the requested new tab opens in the popup,alongside the existing tab and each of these tabs (in the popup) can be closed independently from the others,or the popup as a whole can be closed like a normal window.

Comment 10

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #9)
> This may sound dumb,but wouldn't the behavior of Firefox 3.6 be adequate,i.e.
> the requested new tab opens in the popup,alongside the existing tab and each of
> these tabs (in the popup) can be closed independently from the others,or the
> popup as a whole can be closed like a normal window.

No, it's a fair question. The problem is that these windows are often weird (since they can disable the toolbar, turn off resizing, etc).

Ideally, we would reset these properties if another tab was trying to open in that window, so you got a normal window from there on. If we can't do that, it's better to redirect the request to the underlying window.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

Writing this was an interesting journey, although I repeatedly regretted taking this bug...

This gets rid of some our few openNewTabWith and openNewWindowWith callers, which I believe is a win. openNewTabWith in particular doesn't look like it belongs in utilityOverlay.js at all, as it breaks when called from a non-browser window. I'll look into cleaning this up in a followup.

Another side effect of this patch is that it kills the exotic "force feed preview page" feature, which seems to be neither advertised nor discoverable nor useful.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

discovered a bug upon reviewing my own code

Comment 13

[Alice0775 White]

(In reply to comment #12)
> Created attachment 469869 [details] [diff] [review]
> patch v2
> 
> discovered a bug upon reviewing my own code

something wrong in the patch.
it will load a link in current tab when right click on a link.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v3

(In reply to comment #13)
> something wrong in the patch.
> it will load a link in current tab when right click on a link.

Thanks, this should fix it.

I also restored handleLinkClick's return value since mak thinks it's useful.

Comment 16

[Oskar Larsson Högfeldt]

As you can read in the description of the above duplicate bug, this only happens when "toolbar" is "no" (window.open, third argument is a string containing "toolbar=no".

Comment 18

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #10)
> (In reply to comment #9)
> > This may sound dumb,but wouldn't the behavior of Firefox 3.6 be adequate,i.e.
> > the requested new tab opens in the popup,alongside the existing tab and each of
> > these tabs (in the popup) can be closed independently from the others,or the
> > popup as a whole can be closed like a normal window.
> 
> No, it's a fair question. The problem is that these windows are often weird
> (since they can disable the toolbar, turn off resizing, etc).
> 
> Ideally, we would reset these properties if another tab was trying to open in
> that window, so you got a normal window from there on. If we can't do that,
> it's better to redirect the request to the underlying window.

Actually, I would expect the window to stay as it was (i.e. non-resizable, no toolbars). But maybe it's just me...

I think it would be pretty neat if middle-clicks opened new tabs in parent window by default, but it should be possible to drag a tab to a pop-up window and have it there explicitly.

By the way, what will happen if the parent window is closed?

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> By the way, what will happen if the parent window is closed?

A new window will be opened.

Comment 20

[Dão Gottwald [:dao]]

Also, I think this should block. It would be okay if middle click did nothing in popups, but the way this currently fails doesn't seem acceptable...

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Sneaky. Block a blocker with it, eh Dave? This now blocks due to the dataloss case thanks to the new EM, ew.

Comment 22

[Dão Gottwald [:dao]]

Bug 593687 may need a separate fix, though one that follows the one here.

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch v3

updated to tip

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 483721 [details] [diff] [review]
patch v3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>         let tab = win.gBrowser.loadOneTab(aURI ? aURI.spec : "about:blank", {
>                                           referrerURI: referrer,
>+                                          relatedToCurrent: !needToFocusWin,
>                                           fromExternal: isExternal,
>                                           inBackground: loadInBackground});

Why this change? I don't see why tabs opened from this path would be "related to current" (or why we'd want them to have that behavior), regardless of which window (current or other) they're opening in... r=me with it removed.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

We need some additional tests for openUILink, and contentAreaClick/handleLinkClick in general. Looks like bug 549340 includes some.

Comment 26

[Dão Gottwald [:dao]]

I tried the test in bug 549340, it times out early in waitForFocus...

Comment 27

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/ec6b12294371

Comment 28

[ithinc]

As a followup, maybe we can move "open in sidebar" from PlacesUIUtils.openNodeIn to whereToOpenLink and openLinkIn? We may add a new value "sidebar" for where.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alice, are all cases fixed for you in the latest nightly build?

Comment 30

[Alice0775 White]

(In reply to comment #29)
> Alice, are all cases fixed for you in the latest nightly build?
No, not fixed
Alt+Enter in LocationBar makes hidden tab

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #30)
> No, not fixed
> Alt+Enter in LocationBar makes hidden tab

I filed bug 610203 for that.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #30)
> (In reply to comment #29)
> > Alice, are all cases fixed for you in the latest nightly build?
> No, not fixed

I tried the testcase attached to this bug with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101107 Firefox/4.0b8pre ID:20101107030911 and the same version on Windows.

Doing a right click on the link in the popup and open it as new tab still doesn't open the tab in the browser window but opens it as a hidden tab inside the popup. so I wonder what has been really fixed by the attached patch.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> Doing a right click on the link in the popup and open it as new tab still
> doesn't open the tab in the browser window but opens it as a hidden tab inside
> the popup.

bug 606678

Comment 34

[ithinc]

I see (where == "current") returns from handleLinkClick. Shouldn't it be (event.button == 0 && !event.ctrlKey && !event.shiftKey &&
!event.altKey && !event.metaKey) at the very beginning?

Comment 35

[ramons]

As expressed in various bug reports (such as 626365) the old behavior (as in FF3.6.x) is actually preferred by many users. As a UX professional I quite disagree that the tab strip in the pop-up amounts to bad UX, especially when considering the current behavior of opening the new tab in a window that does not have focus or may no longer exist. There are several cases where the FF3.6.x behavior makes perfect sense and is indeed and without doubt the expected behavior.
I find the reasons given here rather weak and it seems as it was more a decision by developers to drop functionality so that other issues did not need to get fixed. IMHO it entirely goes against end-user preferences.

Unfortunately, any new bugs in this regard get closed as duplicates or invalid. Please advise on how to proceed to get this removed or remodeled to become configurable. I would be fine with keeping it the way it is as default, but have an option in about:config to change back to old behavior.