642355 - "You're about to close 2 tabs..." dialog when closing popup window; triggered by Print Preview

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. load http://www.mapquest.com/
2. enter in the "Search For" text field: tour eiffel, paris 
3. click the "Get Map" button
4. click the Print button (above the "Search For")
5. in the new window: Firefox->Print->Print Preview
6. in Print Preview toolbar, click the Close button
Note that the URL bar that was present before Print Preview is now gone
7. click the X button in the upper right corner to close the popup window

=> "You're about to close 2 tabs..." dialog appears

EXPECTED RESULTS
URL bar should be restored when exiting Print Preview.
No warning dialog about 2 tabs.

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox 4 RC1 on Windows 7.
Bug does not occur in Firefox nightly build 2011-03-09 on Linux.

Comment 1

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

The issue here is the menubar. If it is shown you will not see this problem. If you disable the menubar on Linux we don't even show the firefox button for popups so I can't find a way to check this regression on Linux.

The steps pass with Firefox 3.6 and the menu bar hidden.

Comment 2

[Alice0775 White]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110318 Firefox/4.0b13pre ID:20110318071849

Error in error console are as follows:

*After Step 5(Enter Print Preview):

Error: newBrowser is undefined
Source file: chrome://browser/content/tabbrowser.xml
Line: 867

Error: content is null
Source file: chrome://browser/content/browser.js
Line: 5556



*After Step6(Exit Print Preview):

Error: window.content is null
Source file: chrome://global/content/printUtils.js
Line: 265


*After Step 7 and Chose Close Tabs(Close the popup window):

Error: aWindow.content is null
Source file: resource://gre/components/nsSessionStore.js
Line: 845

Comment 3

[Alice0775 White]

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 4

[Dão Gottwald [:dao]]

Attachment: patch

This bug is caused by browser.css hiding the tabs toolbar:
http://hg.mozilla.org/mozilla-central/annotate/8618a149ea2e/browser/base/content/browser.css#l319
This patch prevents this and lets updateVisiblity hide the toolbar in popups regardless of browser.tabs.autoHide -- this was already supposed to work this way, but apparently window.toolbar.visible ended up on the wrong line, where it makes no sense.

Comment 5

[Dão Gottwald [:dao]]

Also, this happens regardless of whether the menu bar is hidden.

Comment 6

[Dão Gottwald [:dao]]

And on Linux, presumably OS X too.

Comment 7

[Dão Gottwald [:dao]]

Last but not least, this prevents nsSessionStore.js from handling the closed popup; it sticks around in the session and reappears when restoring that session.

Comment 8

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

(In reply to comment #4)
> This bug is caused by browser.css hiding the tabs toolbar:
> http://hg.mozilla.org/mozilla-central/annotate/8618a149ea2e/browser/base/content/browser.css#l319

How is print preview involved? Why does it cause exceptions like those in comment 2, or the alert about 2 tabs from comment 0?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> (In reply to comment #4)
> > This bug is caused by browser.css hiding the tabs toolbar:
> > http://hg.mozilla.org/mozilla-central/annotate/8618a149ea2e/browser/base/content/browser.css#l319
> 
> How is print preview involved? Why does it cause exceptions like those in
> comment 2, or the alert about 2 tabs from comment 0?

Print preview adds a tab, but that tab doesn't get an xbl binding applied, so things like setting tab.selected fail.

Comment 10

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

(In reply to comment #4)
> This bug is caused by browser.css hiding the tabs toolbar:

Oh - "... thus preventing XBL binding attachment and breaking pretty much all uses of tabContainer" was the crucial part I missed.

> lets updateVisiblity hide the toolbar in popups
> regardless of browser.tabs.autoHide

How is this related?

Comment 11

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

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

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

>     <toolbar id="TabsToolbar"
>+             class="toolbar-primary chromeclass-toolbar"

Won't chromeclass-toolbar result in window[chromehidden~="location"][chromehidden~="toolbar"] windows being similarly busted (per http://hg.mozilla.org/mozilla-central/annotate/8618a149ea2e/toolkit/content/xul.css#l57)?

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #10)
> > lets updateVisiblity hide the toolbar in popups
> > regardless of browser.tabs.autoHide
> 
> How is this related?

Without it, the toolbar wouldn't hide in popups.

(In reply to comment #11)
> >     <toolbar id="TabsToolbar"
> >+             class="toolbar-primary chromeclass-toolbar"
> 
> Won't chromeclass-toolbar result in
> window[chromehidden~="location"][chromehidden~="toolbar"] windows being
> similarly busted (per
> http://hg.mozilla.org/mozilla-central/annotate/8618a149ea2e/toolkit/content/xul.css#l57)?

Apparently (except that [chromehidden~="location"] won't match unless a user toggled dom.disable_window_open_feature.location)

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v2

removed chromeclass-toolbar

Comment 14

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

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

Is toolkit/themes/pinstripe/global/toolbar.css specifying a min-height of 24px for .toolbar-primary going to cause problems? I guess our browser.css gives it a height of 26px, so it should be OK (though maybe we should get rid of that toolkit rule for other reasons).

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> I guess our browser.css gives it a height of 26px, so it should be OK

Yes

> (though maybe we should get rid of that toolkit rule for other reasons).

Yeah, I don't think it belongs there.

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/bb7a453efe3d

Comment 17

[(no longer active)]

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

Comment 18

[Vlad [QA]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre

Comment 19

[Thomas]

I still have this on Firefox 6 beta.
Closing a popup gives the message mentionned, neither ok nor cancel button do anything