484315 - With browser.tabs.closeWindowWithLastTab set to false popups cannot be closed

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090316 Shiretoko/3.1b4pre ID:20090316030927

With the implementation of the hidden pref to prevent the closing of the window when the last tab is closed with Cmd/Ctrl+W, there is no way to close a popup while using the keyboard. This seems to be OS X only. As given in the menu the shortcut should be Shift+Cmd+W but it only ends into a beep. But the window is not closed. The user has to use the mouse to get rid of the popup.

Comment 1

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

Steps:

1. Load http://www.popuptest.com/popuptest1.html
2. Open one of these popups
3. Hit Cmd+W to close the tab => window stays open
4. Hit Shift+Cmd+W => window stays open

It seems like that the focus get lost and the event is not handled. Using the tab key to switch to the content area and press Shift+Cmd+W then, it will close the window.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

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

I think !win.document.documentElement.getAttribute("chromehidden") is a better "is a popup" test.

This change to ignore the pref for popups might make sense, but isn't the real bug the fact that you can't use Cmd+Shift+W after Cmd+W?

The test could also use win.closed rather than checking for !win.document (which should eliminate the need for a setTimeout, I think).

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> I think !win.document.documentElement.getAttribute("chromehidden") is a better
> "is a popup" test.

Given that I know the content window for the popup that I've opened, why would that be better? Seems more explicit the way it is.

> This change to ignore the pref for popups might make sense, but isn't the real
> bug the fact that you can't use Cmd+Shift+W after Cmd+W?

I don't know, I can't reproduce it (Henrik says it's OSX-only) and don't see where exactly it would fail. But if the Cmd+Shift+W malfunction depends on Cmd+W, then it's kind of irrelevant, because Cmd+W should already close the window.

> The test could also use win.closed rather than checking for !win.document
> (which should eliminate the need for a setTimeout, I think).

I didn't know that, will try to eliminate the setTimeout.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

using win.closed

Comment 6

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

(In reply to comment #4)
> (In reply to comment #3)
> > I think !win.document.documentElement.getAttribute("chromehidden") is a better
> > "is a popup" test.
> 
> Given that I know the content window for the popup that I've opened, why would
> that be better? Seems more explicit the way it is.

I meant for the patch, not the test.

Comment 7

[Dão Gottwald [:dao]]

But the tabstrip visiblity depends on window.toolbar.visible. Maybe it shouldn't?

Comment 8

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

Why should it be based on tabstrip visibility?

I thought the argument was that chrome-reduced popups are unlikely to be useful for general navigation, so people are unlikely to want the window-preserving behavior, and navigation bar visibility is more relevant than tabstrip visibility as far as navigation goes.

Comment 9

[Dão Gottwald [:dao]]

The tabstrip visiblity is relevant for navigation since it implies a "new tab" button. The location bar's read-only-ness depends on window.toolbar.visible, too.
I'm not sure that window.toolbar.visible is the right dependency in general, but I don't think this bug should be an exception.

Comment 10

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

Well, we use getAttribute("chromehidden") in getMostRecentBrowserWindow, though I forget the exact reason. It seems to have been changed in bug 337344.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch v2.1

using synthesizeKey instead of removeTab, nothing else changed

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2.1

updated to latest trunk

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch v2.1

updated to latest trunk

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v2.1

updated to latest trunk

Comment 15

[Alfred Kayser]

From end-user point of view, pressing CtrlW on a window without a tabbar should always close it, even with closeWindowWithLastTab = false. 
If there is no tabbar visible, there is no concept of last tab, and thus is the window tabless, and should operate like that.

closeWindowWithLastTab = false should only prevent closing the window when the last tab of the tabbar is being closed, with a visible tabbar.
Or better, when closeWindowWithLastTab = false the cmd_close operation should not fire on the last empty tab (with tabbar visible).

Comment 16

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

Not blocking, as it's based on a hidden pref, but would take a safe patch with tests.

Comment 17

[Dão Gottwald [:dao]]

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

Comment 18

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

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091108 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091108045014

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d2d2ed0ded6d

Comment 20

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

Verified on 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b5pre) Gecko/20091126 Namoroka/3.6b5pre ID:20091126033851