377551 - Pages opened in new tabs should skip chrome-less windows (like popups)

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P4)

Description

[Markus Peter]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/419 (KHTML, like Gecko) Safari/419.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

The "New pages should be opened in new tabs" should be more intelligent: If more than one window is open, it should not open the new tab in the current/last opened window if it's a window without browser chrome (e.g. popups without menu bars etc.) but instead open the page in one of the windows which have full browser chrome.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Jesse Ruderman]

Bug 369527 or bug 243893?

Comment 2

[Markus Peter]

I don't think it's actually a dupe even if it's related. Bug 243893 is clearly a bug reporting concerning the tab bar which was resolved and fixed, but the issue I'm talking here about is the general usability issue caused by the behavior of re-using popup windows.

The behavior should be fixed in my opinion. I think Mike Beltzer talked in Bug 243893 about it in comment 26 - this bug report is the same as use case 2 in his comment, but I cannot re-open the bug and this gets reported once a week by users of our site which uses popup windows for legitimate reasons: People can open chat windows in new popups on demand (and they do not want to have the browser chrome there) but they expect the target="_blank" links in the chat windows to be opened in a true browser window and I do not want to have to abuse JavaScript just to force a browser to open a simple link in a true browser window with chrome.

If you won't fix the usability issue, simply tell me that and I will communicate this to our users, but handling this report as a dupe is not enough - this issue simply was not really discussed in 243893.

Comment 3

[Samuel Sidler (old account; do not CC)]

Confirming as a valid RFE and updating summary a bit.

Comment 6

[Jesse Ruderman]

See bug 391778 for an example of where Firefox's (default!) behavior completely breaks a site.  A small popup on Sony's site tries to open a large (unsized) popup, but users end up with an additional tab in a tiny, non-resizable window.

Combined with the change between Firefox 1.0 and Firefox 2.0 to make "New pages should be opened in: a new tab" the default, this is a serious usability regression for some sites.

Comment 7

[Stephen Foss]

Thank you gentlemen and scholars!  Happy I helped the Mozilla-Firefox cause. (I guess I'll have to make a PayPal contribution, now)

Comment 8

[Mike Connor [:mconnor]]

We need to fix the remaining cases of this, I thought I had this fixed, but for Mac at least there's issues.

Comment 9

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 10

[Florian Quèze [:florian]]

Attachment: work in progress

* The openDialog part doesn't work yet.

* I'm not sure the window.toolbar.visible test is acceptable to ensure we are not on a chrome-less window.

Comment 11

[Florian Quèze [:florian]]

Attachment: patch v2

Comment 12

[Asaf Romano (gone)]

Comment on attachment 286733 [details] [diff] [review]
patch v2

This patch no loner applies.

Index: browser/base/content/browser.js
===================================================================
RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
retrieving revision 1.877
diff -u -U 8 -r1.877 browser.js
--- browser/base/content/browser.js	25 Oct 2007 23:02:20 -0000	1.877
+++ browser/base/content/browser.js	30 Oct 2007 20:27:27 -0000
@@ -3960,20 +3960,44 @@
     }
     var url = aURI ? aURI.spec : "about:blank";
     switch(aWhere) {
       case Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW :
         newWindow = openDialog(getBrowserURL(), "_blank", "all,dialog=no", url);
         break;
       case Ci.nsIBrowserDOMWindow.OPEN_NEWTAB :
         var loadInBackground = gPrefService.getBoolPref("browser.tabs.loadDivertedInBackground");
-        var newTab = gBrowser.loadOneTab("about:blank", null, null, null, loadInBackground, false);
-        newWindow = gBrowser.getBrowserForTab(newTab).docShell
-                            .QueryInterface(Ci.nsIInterfaceRequestor)
-                            .getInterface(Ci.nsIDOMWindow);
+        if (window.toolbar.visible) {

See https://bugzilla.mozilla.org/show_bug.cgi?id=337344#c72

+          // open a new tab in the same window.
+          var newTab = gBrowser.loadOneTab("about:blank", null, null, null, loadInBackground, false);
+          newWindow = gBrowser.getBrowserForTab(newTab).docShell
+                              .QueryInterface(Ci.nsIInterfaceRequestor)
+                              .getInterface(Ci.nsIDOMWindow);
+        }
+        else {
+          // we are in a chrome-less window, look for another window we can use.
+          var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
+                             .getService(Components.interfaces.nsIWindowMediator);
+          var windows = wm.getZOrderDOMWindowEnumerator("navigator:browser", true);

I think getZOrderDOMWindowEnumerator is still broken on linux.

Comment 13

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

(In reply to comment #12)
> I think getZOrderDOMWindowEnumerator is still broken on linux.

Indeed. See bug 302281 comment 5 and bug 156333.

Comment 14

[Florian Quèze [:florian]]

Attachment: patch v3

This duplicates the ugly code of getMostRecentBrowserWindow from nsBrowserContentHandler.js

Comment 15

[Asaf Romano (gone)]

I think it's worth checking the most-common case above the #ifdef block in which |window| just fits the criteria, and only go through the most-recent-loop in all other edge cases....

Comment 16

[Asaf Romano (gone)]

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

+    if (!window.document.documentElement.getAttribute("chromehidden"))
+      return window;

Somehow i missed that, r=mano

Comment 17

[Florian Quèze [:florian]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.926; previous revision: 1.925
done

Comment 18

[Jack Eidsness]

I'm pretty sure this has busted target="_blank" links and links from external applications, but not middle-clicks, for me.  They now open up new tabs that remain empty, and then open a new window, too, and the content loads in the new window.

Comment 19

[Jack Eidsness]

I spoke too soon - it was just a conflict with an add-on. (server switcher 0.4, if you care to know).

Comment 20

[José Jeria]

Did seems to have caused bug 439054 which ruins the experience on several intranet applications we developed. 

Comment 21

[Patrick Strasser]

I see more than one issue (with bug 439054): 

* Some like to have _blank targets opened in a new popup
* Some like to have _blank targets opened in a new full-size window, not necessarily a popup
* Some like to have _blank targets opened in the same frame (popup/browser window), but preferably as a tab.

Tab-browsing is something that adds to pop-ups and non-pop-ups, and has to be handled independent of opening a new document, may it be a link or a js-inited window. 
Would it be possible to add a new switch to the window.open method to specify some of this wishes? Tabbed browsing is wide-spread nowadays, why not add control for it?

Comment 22

[Jesse Ruderman]

Patrick, see bug 105409.

Comment 23

[Patrick Strasser]

I understand that adding new features is a hairy thing and needs a lot of consideration.

The resolution to this bug changes the user experience very heavily. It changed the behaviour on the client/user side. The old behaviour was useful like is the new one. But it is hardcoded, and neither the coder nor the user can change it. I understand that adding an option for the coder would be problematic. I don't see much danger in allowing a coder to hint for opening in the same context, though. But at least the user should be able to choose the behaviour.

I really would suggest to have an option to toggle between the new and the old behaviour. I'd suggest to add an option likewise to 'When I open a link in a new tab, switch to it immediately' in the Tabs Options page.

The preference Browser.link.open_newwindow could be extended to a 4th value, enabling the new behaviour.

See http://kb.mozillazine.org/Browser.link.open_newwindow (description is not valid any more with this bug resolution!)

Comment 24

[Patrick Strasser]

After reviewing some bugs and thinking more about tab and popup-behavior...

This resolution disables tabs in popups completely. This is a bad regression.
Moreover there's no need to configure this:
* Opening external links in a full-chrome window is a good thing. Full-screen popups that look like full-chrome windows are bad. Opening a link in such a window in disguise should not be fostered.
* A user wanting a new window instead of a tab (comment 6) needs no special precautions: The context menu has "Open link in new window". No need for "Open link in a tab in a full-chrome window".
* Having tabs in popups and showing a tab bar is a good thing to keep context for web apps. See bug 439054 comment 11.

I think the resolution to this bug should be changed to only affect external links. These get focus after loading, so they do not get lost if they end up in a different window. Other links should not be diverted.

Comment 25

[Rodrigo Mello]

This behavior messed with some software we developped. Please, come back to Firefox 2 behavior. ;)
Thanks!