344257 - [FIX]Hidden pref to open ALL windows open in tabs no longer works in this case

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Scott Field]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b1) Gecko/20060707 Firefox/2.0b1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b1) Gecko/20060707 Firefox/2.0b1

Previously selecting the preference "open links that would open new windows in new tabs in the most recent window" resulted in all windows opening in new tabs. At some point the developers decided that js pop-ups which are resized get opened in a new window.  See https://bugzilla.mozilla.org/show_bug.cgi?id=313300
It has been stated that setting browser.link.open_newwindow.restriction to 0 would bring back the old behavior.

I have a js bookmarklet for Sitebar to save a bookmark to an external server.
javascript:w=window;if(w.content){w=w.content};d=w.document;cp=d.characterSet?d.characterSet:d.charset;w.open('http://www.yhbt.com/sitebar/command.php?command=Add%20Link&name='+escape(d.title)+'&url='+escape(w.location.href)+(cp?'&cp='+cp:''),'sbBmkWin','resizable=yes,dependent=yes,width=210,height=360,top=200,left=300,titlebar=yes,scrollbars=yes');void(0)

With default settings this bookmarklet opens up a new window. Previously when selecting the preference to open all new windows in tabs it would open in a tab. After the change, a new window is now opened. Setting browser.link.open_newwindow.restriction to 0 has no effect.

As the new window often gets hidden and forgotten, I would prefer to have the option to make it open in a new tab (as the preference language would indicate.)

Reproducible: Always

Steps to Reproduce:
1. Select preference "open links that would open new windows in new tabs in the most recent window"
2. Set browser.link.open_newwindow.restriction to 0
3. Click on bookmarklet "javascript:w=window;if(w.content){w=w.content};d=w.document;cp=d.characterSet?d.characterSet:d.charset;w.open('http://www.yhbt.com/sitebar/command.php?command=Add%20Link&name='+escape(d.title)+'&url='+escape(w.location.href)+(cp?'&cp='+cp:''),'sbBmkWin','resizable=yes,dependent=yes,width=210,height=360,top=200,left=300,titlebar=yes,scrollbars=yes');void(0)"


Actual Results:  
New window pops-up

Expected Results:  
Window should open in tab.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=314721 for further discussion.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060711 Minefield/3.0a1
I tested some builds and between 1.9a1_2006020712 and 1.9a1_2006020723 I see this sudden change in behaviour and this could point to Bug 323810.
Bonsai link:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-02-07+11%3A00&maxdate=2006-02-08+00%3A00

Comment 2

[Boris Zbarsky [:bzbarsky]]

Minimal testcase is:

  javascript:void(window.open("", "", "dependent=yes"))

And this is happening because of the following code:

    // Now check whether it's ok to ask a window provider for a window.  Don't
    // do it if we're opening a dialog or if our parent is a chrome window or
    // if we're opening something that has dependent, modal, dialog, or chrome
    // flags set.
    nsCOMPtr<nsIDOMChromeWindow> chromeWin = do_QueryInterface(aParent);
    if (!aDialog && !chromeWin &&
        !(chromeFlags & (nsIWebBrowserChrome::CHROME_DEPENDENT     |
                         nsIWebBrowserChrome::CHROME_MODAL         |
                         nsIWebBrowserChrome::CHROME_OPENAS_DIALOG | 
                         nsIWebBrowserChrome::CHROME_OPENAS_CHROME))) {

in this case, nsIWebBrowserChrome::CHROME_DEPENDENT is set.

First of all, does that flag do something useful for content windows?  If not, why are we even setting it?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This is the first thing that comes to mind -- disallow the CHROME_DEPENDENT bit for non-chrome windows opened by unprivileged script, where "unprivileged" means "doesn't have UniversalBrowserWrite".

Looking at our window creators, CHROME_DEPENDENT in fact does nothing if CHROME_OPENAS_CHROME is not set.  So I _could_ probably just remove the test for CHROME_DEPENDENT and it would work OK for Seamonkey and Firefox.  But this flag dependency is not documented on either nsIWindowCreator or nsIWebBrowserChrome, which makes me a little unwilling to depend on it in general embedding situations...  Perhaps some such dependency _should_ be documented, though?  These interfaces are pretty underdocumented in general.  :(

Comment 4

[Benjamin Smedberg]

> general embedding situations...  Perhaps some such dependency _should_ be
> documented, though?  These interfaces are pretty underdocumented in general. 

I think we should document that "dependent" only means something in the context of chrome windows and is not allowed for content window opening.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 229527 [details] [diff] [review]
Patch to that effect

sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 229527 [details] [diff] [review]
Patch to that effect

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.

Comment 9

[Worcester12345]

[mustfix] keyword?

Comment 10

[Boris Zbarsky [:bzbarsky]]

This isn't a mustfix.  It's a nice-to-fix, maybe.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.

Comment 12

[Scott Field]

(In reply to comment #11)
> Fixed on branch.
> 
My original example 
javascript:w=window;if(w.content){w=w.content};d=w.document;cp=d.characterSet?d.characterSet:d.charset;w.open('http://www.yhbt.com/sitebar/command.php?command=Add%20Link&name='+escape(d.title)+'&url='+escape(w.location.href)+(cp?'&cp='+cp:''),'sbBmkWin','resizable=yes,dependent=yes,width=210,height=360,top=200,left=300,titlebar=yes,scrollbars=yes');void(0)
not only continues to open in a new window, but now a blank tab opens along with it.

I get the same behavior when clicking on the link
http://tinyurl.com/ch7nb

Comment 13

[Boris Zbarsky [:bzbarsky]]

Scott, I can't reproduce that behavior with <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/firefox-2.0b1.en-US.linux-i686.tar.gz>.  And a vanilla profile.  What build are you using?  Do you still see the problem in safe mode?

Comment 14

[Scott Field]

(In reply to comment #13)
> Scott, I can't reproduce that behavior with
> <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/firefox-2.0b1.en-US.linux-i686.tar.gz>.
>  And a vanilla profile.  What build are you using?  Do you still see the
> problem in safe mode?
> 
With a new profile and browser.link.open_newwindow set to 3 and browser.link.open_newwindow.restriction set to 0 I'm now getting the desired behavior of the pop-ups opening in a tab.  (Time to start disabling extensions) However,the sitebar bookmarklet that I cited results in a blank tab as well as the desired tab.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Huh...  The bookmarklet just opens one tab over here...  Is that a Mac-specific issue of some sort or something like that?

Comment 16

[Scott Field]

(In reply to comment #15)
> Huh...  The bookmarklet just opens one tab over here...  Is that a Mac-specific
> issue of some sort or something like that?
>

Just determined that the tab mix plus extension is the culprit. 

Comment 17

[Boris Zbarsky [:bzbarsky]]

Huh.  I wonder how it interferes with this...

Comment 18

[Ria Klaassen (not reading all bugmail)]

Tab mix has also settings for pop-ups in its options. When I choose "open all pop-ups in tabs" also the bookmarklet is opened in a tab.

Reopened, because the pop-up opens in the background now with browser.link.open_newwindow.restriction to 2. I see only a short flash of the pop-up before it disappears.
This is a regression from this fix.
Or shall I file a new bug for that?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> because the pop-up opens in the background now with
> browser.link.open_newwindow.restriction to 2.

With or without tabmix?  If it's with tabmix, please reclose this bug, file a new one, post a link to the tabmix extension.

Comment 20

[Ria Klaassen (not reading all bugmail)]

Sorry for not being clear. Without Tab mix. I see the same problem on branch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er... I see.  It's probably worth filing a new bug on this in general; I don't see your problem on Linux, and I'm not going to be able to fix Win32 widgetry issues...

In that bug, please say which of the several testcases in this bug are you using, what the exact steps you are performing with this testcase are, and whether the results change if you remove "dependent" from the window feature string in whatever testcase you're using.

Thanks!