jscher2000 Reporter
	Description • 6 years ago

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

	jscher2000 Reporter
	Comment 1 • 6 years ago

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

	Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland
	Comment 2 • 6 years ago

(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

	jscher2000 Reporter
	Comment 3 • 6 years ago

(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.

	:Gijs (he/him) Assignee
	Comment 4 • 6 years ago

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.

	jscher2000 Reporter
	Comment 5 • 6 years ago

(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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 6 • 6 years ago

Getting a bit late for 64, but we can still take a patch for 65.

	:Gijs (he/him) Assignee
	Comment 7 • 6 years ago

(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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 8 • 6 years ago

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.

	:Gijs (he/him) Assignee
	Comment 9 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d57e6dedcc21f27f208d1d0b36a6d1ae6c39df

	Pulsebot
	Comment 11 • 5 years ago

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