Closed Bug 1502179 Opened 2 years ago Closed 2 years ago

Prompted to close 2 windows when closing parent window of popup

Categories

(Firefox :: Tabbed Browser, defect, P2)

63 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: jscher2000, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

(1) With any number n of tabs in a regular window

(2) Click a link that triggers a window.open() with features (e.g., third link on https://www.jeffersonscher.com/res/popit.html )

(3) Click the X close button on the regular window


Actual results:

Firefox displayed "Confirm close" dialog reading: "You are about to close 2 windows with (n+1) tabs. Are you sure you want to continue?"

If you continue, Firefox closes the main window, but does not close the popup.


Expected results:

Since Firefox is not going to close the popup, it should at most display a "Confirm close" dialog reading: "You are about to close n tabs. Are you sure you want to continue?" (assuming n > 1).

Ref. https://support.mozilla.org/questions/1238132
Running Mozregression for this was a bit of a nightmare because I had to keep resetting browser.tabs.warnOnClose and browser.warnOnQuit to their default values, and our security software needs 5 confirmation prompts clicked for the "in between" builds... 

2018-10-25T14:22:07: DEBUG : Found commit message:
Bug 1483935 - correctly check all windows for tabs when quitting, r=mconley

Bug 1438499 added an optional parameter to warnAboutClosingTabs.
In bug 1475427, the arguments to warnAboutClosingTabs changed, and instead
of passing a closing tab reference as the second argument, we now need to
pass the number of tabs as the first argument. The patch in that bug
updated the callsite in nsBrowserGlue.js to add the new argument, but
didn't remove the `null` argument that we were passing for the 'extra' tab.

Additionally, the change in bug 1475427 bails early from
warnAboutClosingTabs if the number of tabs passed is less than 2. That tab
count, too, needs to take into account multiple windows and not just the
last window iterated over.

This patch fixes both of these issues.

Differential Revision: https://phabricator.services.mozilla.com/D3807
(In reply to jscher2000 from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:63.0) Gecko/20100101
> Firefox/63.0
> 
> Steps to reproduce:
> 
> (1) With any number n of tabs in a regular window
> 
> (2) Click a link that triggers a window.open() with features (e.g., third
> link on https://www.jeffersonscher.com/res/popit.html )
> 
> (3) Click the X close button on the regular window
> 
> 
> Actual results:
> 
> Firefox displayed "Confirm close" dialog reading: "You are about to close 2
> windows with (n+1) tabs. Are you sure you want to continue?"
> 
> If you continue, Firefox closes the main window, but does not close the
> popup.
> 
> 

I don't get these results on my Surface Book, Windows 10, Firefox 63.
i.e. a dialogue box saying you are about to close n tabs is displayed, i click on the diaglue and the main windows is closed and the popup is not closed

> Expected results:
> 
> Since Firefox is not going to close the popup, it should at most display a
> "Confirm close" dialog reading: "You are about to close n tabs. Are you sure
> you want to continue?" (assuming n > 1).
> 
> Ref. https://support.mozilla.org/questions/1238132
Blocks: 1483935
Status: UNCONFIRMED → NEW
Component: Untriaged → Tabbed Browser
Ever confirmed: true
Keywords: regression
(In reply to Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland from comment #2)
> (In reply to jscher2000 from comment #0)
> 
> I don't get these results on my Surface Book, Windows 10, Firefox 63.
> i.e. a dialogue box saying you are about to close n tabs is displayed, i
> click on the diaglue and the main windows is closed and the popup is not
> closed

Sorry, I somehow forgot to mention that this occurs when the "main window" is the only regular window.
This is a result of `warnAboutClosingWindow` having logic that doesn't consider popups real windows. As a result, it treats this case as closing the last "real" window, triggering the `browser-lastwindow-close-requested` observer topic, which nsBrowserGlue treats the same as quitting, even though it doesn't, in fact, close the last window (or close the browser).

I don't really understand why we decided not to treat popups as real windows and therefore treat it as quitting - maybe because it's not trivial to "get back to" a working browser in those cases (on non-macOS)?

I'd be curious what happens on builds prior to bug 1438499, if you close the last "real" window, leaving the popup open, with the `warnOnQuit` dialog on, and pick 'Save and Quit'. Do navigations in the popup window get saved? What kind of session is even saved? Or do we just actually quit the browser in that case? What happens on macOS?

This is pretty messed up, and it's not clear to me what the correct behavior would be.
(In reply to :Gijs (he/him) from comment #4)
> I don't really understand why we decided not to treat popups as real windows
> and therefore treat it as quitting - maybe because it's not trivial to "get
> back to" a working browser in those cases (on non-macOS)?

"Trivial" is hard to define. Photon's menu button appears along with the location bar in popups-with-features, at least on Windows, so New Window is immediately available, and with more digging, Library > History > Recently Closed Windows is available. On the other hand, the location bar is read-only, there's no Home button, there's no Bookmarks Toolbar, and the Bookmarks Sidebar is a chore to open (buried deeply in the Library menu). So while the popup is a usable window for some purposes, many users may find it difficult to figure out what to do next, other than close it and start Firefox again.
Priority: -- → P2
Getting a bit late for 64, but we can still take a patch for 65.
(In reply to :Gijs (he/him) from comment #4)
> I'd be curious what happens on builds prior to bug 1438499, if you close the
> last "real" window, leaving the popup open, with the `warnOnQuit` dialog on,
> and pick 'Save and Quit'. Do navigations in the popup window get saved? What
> kind of session is even saved? Or do we just actually quit the browser in
> that case?

I used mozregression, so unsure what gets saved, but you do get a "Quit" dialog, and if you either 'save and quit' or 'quit' then we leave the popup window open. So in that sense, it's just as broken as today, but it's easier to trigger this today because you don't need to trip a bunch of about:config pref to get it.


Feels like we should just only ask about the current window and avoid hitting the 'last window' case if this isn't, in fact, the last window, even if the remaining windows are popups. That's what happened before without the 'save and quit' dialog manually enabled, and seems not completely unreasonable, even if it could be improved.
Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #9033399 - Attachment description: Bug 1502179 - don't treat closing last non-popup window as quitting when a popup window is still open (on Windows/Linux), r?jaws → Bug 1502179 - don't treat closing last non-popup window as quitting when a popup window is still open (on Windows/Linux), r?jaws!,mikedeboer!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e4397c150d9
don't treat closing last non-popup window as quitting when a popup window is still open (on Windows/Linux), r=jaws,mikedeboer
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Flags: in-testsuite+

Per IRC discussion with Gijs, it'd be better to let this ride the trains.

I can reproduce this issue with Nightly 65.0a1 (2018-10-25) (64-bit) on Linux x86_64

I am verifying that this is fixed in latest Nightly 66.0a1

Build id             20190117215514
User Agent       Mozilla/5.0 (X11; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

QA Whiteboard: [bugday-20190116]
You need to log in before you can comment on or make changes to this bug.