Closed Bug 642355 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 5

People

(Reporter: mats, Assigned: dao)

References

()

Details

(Keywords: regression, reproducible)

Attachments

(1 file, 1 obsolete file)

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.
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.
Summary: "You're about to close 2 tabs..." dialog when closing popup window; triggered by Print Preview → "You're about to close 2 tabs..." dialog when closing popup window; triggered by Print Preview and menubar hidden
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
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
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #521640 - Flags: review?(gavin.sharp)
Blocks: 641157
Also, this happens regardless of whether the menu bar is hidden.
Summary: "You're about to close 2 tabs..." dialog when closing popup window; triggered by Print Preview and menubar hidden → "You're about to close 2 tabs..." dialog when closing popup window; triggered by Print Preview
And on Linux, presumably OS X too.
OS: Windows 7 → All
Hardware: x86 → All
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.
Severity: minor → normal
Blocks: 644749
(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?
(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.
(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 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)?
(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)
Attached patch patch v2Splinter Review
removed chromeclass-toolbar
Attachment #521640 - Attachment is obsolete: true
Attachment #521640 - Flags: review?(gavin.sharp)
Attachment #522951 - Flags: review?(gavin.sharp)
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).
Attachment #522951 - Flags: review?(gavin.sharp) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/bb7a453efe3d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → Firefox4.2
Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Target Milestone: Firefox5 → Firefox 5
I still have this on Firefox 6 beta.
Closing a popup gives the message mentionned, neither ok nor cancel button do anything
Blocks: 561482
No longer blocks: 561482
Flags: in-testsuite?
Blocks: 561482
No longer blocks: 561482
Blocks: 561482
You need to log in before you can comment on or make changes to this bug.