arni2033 Reporter
	Description • 9 years ago
see-comment-1

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151223030323
STR:
1.   Open attached "testcase 1" or the "data:" url from the form above in a new tab
2.   Repeat {Step 1} 4 more times   [you'll get 5 offending tabs total]
3.A) Pin tabs opened in Steps 1 and 2
3.B) Open many new tabs to make tabs opened in Step 1 and 2 invisible in tabstoolbar
4.   Click close button in firefox window OR click   (≡) -> Exit Nightly
5.   Repeat {Step 4} 4 more times

Result:       
 Firefox window remains open. There's no indication of what is happening

Expectations: 
 1) It must be clear which tab contains my unfinished work
 2) It must be clear why the browser doesn't close

It was regressed by bug 332195
> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f1f6a7ae585eb6ed1eb6caebdf4e491fbfa87fd4&tochange=66d59599465c708d4dc985c774c99ccc26aca69d

	arni2033 Reporter
	Comment 1 • 9 years ago

Correction!
In Steps 1 and 2 you must click on content area after you open a tab because of bug 636905.

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

Review commit: https://reviewboard.mozilla.org/r/29161/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29161/

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

Review commit: https://reviewboard.mozilla.org/r/29163/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29163/

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

This is a little annoying because I'd hoped to avoid having to select tabs that have beforeunload dialogs, because of the large amount of abuse that web "feature" gets from trap pages and so on.

Unfortunately, I tested just scrolling the tab into view when it's highlighted, but I think the experience is too weird that way. You click the close button, and some tabs light up, but otherwise nothing happens, and your selected tab might get scrolled out of view? Ordinary users would be non-plussed, I think. Better to just select the tab in question outright.

The first part addresses scenario "A" here - there was a simple oversight in the selector we use for displaying the "attention" and "titlechanged" highlight styles on pinned tabs that broke it for tabs with no favicon. That's a trivial one-line CSS fix.

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

The other annoying thing is that this would need uplift. The reason that's annoying is that bug 967873 missed 44, and so now our close-window architecture looks pretty different. I *think* we can just use gInCanCloseWindow by setting it in WindowIsClosing ( http://hg.mozilla.org/releases/mozilla-beta/file/tip/browser/base/content/browser.js#l6563 ) but I'm not 100% sure.

We can pref off bug 332195 on beta, but that would make me quite sad. I guess the compromise would be missing out the gInCanCloseWindow check, which means that closing non-focused tabs would also re-focus the tabs automatically again. I could live with that for 44.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 6 • 9 years ago

Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

https://reviewboard.mozilla.org/r/29163/#review25983

::: browser/base/content/browser.js:6273
(Diff revision 1)
> -    let {permitUnload, timedOut} = browser.permitUnload();
> +    let {permitUnload, timedOut} = browser.permitUnload(true);

What's |true| for?

::: browser/base/content/browser.js:6282
(Diff revision 1)
> -  return true;
> +  gInCanCloseWindow = false;

I think it would be a little cleaner to use try...finally here.

::: browser/base/content/tabbrowser.xml:4376
(Diff revision 1)
> +              !(gInCanCloseWindow && event.detail.inPermitUnload) &&

As far as I know we only call permitUnload from tab closing or from CanCloseWindow. I guess we can close tabs that aren't currently selected. If the closing tab has an onbeforeunload handler, do we actually not want to select the tab? It seems a little strange not to. (Also, we could get rid of this ugly global :-).

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 7 • 9 years ago

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

https://reviewboard.mozilla.org/r/29161/#review25985

Someone else should review this. I don't know anything about CSS.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 8 • 9 years ago

Oh, I see, we highlight the tab title in that case. And the tab is going to be visible if you're closing it, presumably. Unless the tab calls window.close() on itself I guess. I wonder what happens in that case.

	arni2033 Reporter
	Comment 9 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #4)
> to avoid having to select tabs that have beforeunload dialogs, because of the large amount of abuse
Imo the only way to notify user of his unfinished work in other tab w/o selecting it is some widget (<panel>) similar to Win7's screen that appears on shutdown and lists all applications which don't let OS shut down. But that looks like a lot of extra work for unimportant feature.

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

(In reply to Bill McCloskey (:billm) from comment #8)
> Oh, I see, we highlight the tab title in that case. And the tab is going to
> be visible if you're closing it, presumably. Unless the tab calls
> window.close() on itself I guess. I wonder what happens in that case.

IIRC the web can only do that if it's been opened with window.open. Seems like too much of an edge-case (the web page wants to close itself but also has an onbeforeunload handler?) to worry too much about.

(In reply to Bill McCloskey (:billm) from comment #6)
> Comment on attachment 8702655 [details]
> MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an
> onbeforeunload dialog and we're trying to close the window, r?billm
> 
> https://reviewboard.mozilla.org/r/29163/#review25983
> 
> ::: browser/base/content/browser.js:6273
> (Diff revision 1)
> > -    let {permitUnload, timedOut} = browser.permitUnload();
> > +    let {permitUnload, timedOut} = browser.permitUnload(true);
> 
> What's |true| for?

Oops, that's a leftover. I'll revert.


> ::: browser/base/content/browser.js:6282
> (Diff revision 1)
> > -  return true;
> > +  gInCanCloseWindow = false;
> 
> I think it would be a little cleaner to use try...finally here.
> 
> ::: browser/base/content/tabbrowser.xml:4376
> (Diff revision 1)
> > +              !(gInCanCloseWindow && event.detail.inPermitUnload) &&
> 
> As far as I know we only call permitUnload from tab closing or from
> CanCloseWindow. I guess we can close tabs that aren't currently selected. If
> the closing tab has an onbeforeunload handler, do we actually not want to
> select the tab? It seems a little strange not to. (Also, we could get rid of
> this ugly global :-).

As you noted in comment #8, we highlight the title and/or icon of the tab in that case.

I'm biased in that I've fixed a number of bugs where webpages find ways to "trap" users onto a page by a combination of onbeforeunload dialogs and frames/forms/event handlers. One of the things the work in bug 332195 accomplished was give those pages less leverage over the user: now you won't automatically get switched to the right tab, and you can close a tab like that without it getting selected (which sometimes triggered another round of horridness from the page in question).

OTOH, in one way (but not others...) that is a bit of an edge-case. Fixing this bug for all beforeunload dialogs (rather than just the CanCloseWindow case) would also be more upliftable and essentially fix the "new" part of bug 1234937. Though the core issue (multiple dialog-based-event-loop-weirdness) would still need to get fixed, that predates bug 332195 and could be done at a more leisurely pace.

So let's do that for now, even if it makes me sad.

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

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29161/diff/1-2/

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 12 • 9 years ago

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

https://reviewboard.mozilla.org/r/29161/#review25997

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

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29161/diff/1-2/

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

Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29163/diff/1-2/

	Tim Nguyen :ntim
	Comment 15 • 9 years ago

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

https://reviewboard.mozilla.org/r/29161/#review26001

LGTM !

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/97e179d4873b
https://hg.mozilla.org/integration/fx-team/rev/df0d060fbe69

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/97e179d4873b
https://hg.mozilla.org/mozilla-central/rev/df0d060fbe69

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

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

(approval request for both changes)

Approval Request Comment
[Feature/regressing bug #]: bug 332195
[User impact if declined]: serious issues with modal dialogs not being noticeable on pinned tabs and/or when used for beforeunload, if the tab is not focused. This is especially surprising when closing a window and when unfocused tabs respond to beforeunload events, thus preventing the window from being closed.
[Describe test coverage new/current, TreeHerder]: there is test coverage for this feature and for dialogs in general, so it's unlikely something got fundamentally broken by this landing
[Risks and why]: the CSS change is trivial and is basically 0-risk. That fixes the pinned tab issue. The other change is slightly riskier, but not dissimilar from a check we already have on release and beta, but that is specific to panorama (tab groups) there. I'll put up a patch for beta that just makes that check slightly more generic.
[String/UUID change made/needed]: no

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 9 years ago

Comment on attachment 8703106 [details] [diff] [review]
Part 2 for beta

For beta, we need patch1 + this patch (which is part2)

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 21 • 9 years ago

Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

Part2 also needs to land on Aurora.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 9 years ago

Comment on attachment 8702654 [details]
MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim

This seems related to bug 1233747 that I also uplifted. Taking this one too. Beta44+, Aurora45+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 9 years ago

Comment on attachment 8703106 [details] [diff] [review]
Part 2 for beta

Beta44+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 25 • 9 years ago

Comment on attachment 8702655 [details]
MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm

Aurora45+

	arni2033 Reporter
	Comment 26 • 9 years ago

This is fixed on Nightly 46 (ID 20151231030214)
(tab with onbeforeunload dialog becomes active in both cases described in comment 0)

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cdd7804674b
https://hg.mozilla.org/releases/mozilla-aurora/rev/93cfe61a7bae

	Carsten Book [:Tomcat]
	Comment 29 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/00cff6fcb589
https://hg.mozilla.org/releases/mozilla-beta/rev/692ee3b8ba0a

	arni2033 Reporter
	Comment 30 • 9 years ago

This bug was verified on Nightly 46 in Comment 26, so chaning the status now.

	Carsten Book [:Tomcat]
	Comment 31 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/00cff6fcb589
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/692ee3b8ba0a

	Mayur Patil
	Comment 33 • 9 years ago

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Tabs behavior is acceptable

Actual Results: 
As expected