Closed
Bug 1234936
Opened 9 years ago
Closed 9 years ago
It's not obvious why browser doesn't close (aka It's unclear which tab with onbeforeunlad prevented browser from closing)
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 46
People
(Reporter: arni2033, Assigned: Gijs)
References
()
Details
(Keywords: regression)
Attachments
(4 files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
ntim
:
review+
aryx
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
billm
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
(deleted),
patch
|
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
>>> 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
Correction! In Steps 1 and 2 you must click on content area after you open a tab because of bug 636905.
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/
Attachment #8702654 -
Flags: review?(wmccloskey)
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/
Attachment #8702655 -
Flags: review?(wmccloskey)
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
status-firefox44:
--- → affected
status-firefox45:
--- → affected
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 :-).
Attachment #8702655 -
Flags: review?(wmccloskey) → review+
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.
Attachment #8702654 -
Flags: review?(wmccloskey)
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.
(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.
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.
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/
Attachment #8702654 -
Flags: review?(ntim.bugs)
![]() |
||
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
Attachment #8702654 -
Flags: review+
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/
Attachment #8702654 -
Attachment description: MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r?billm → MozReview Request: Bug 1234936 - part 1: show attention highlight on pinned tabs with no images, r=Aryx,ntim
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/
Attachment #8702655 -
Attachment description: 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 → MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm
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 !
Attachment #8702654 -
Flags: review?(ntim.bugs) → review+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/97e179d4873b https://hg.mozilla.org/integration/fx-team/rev/df0d060fbe69
Assignee | ||
Updated•9 years ago
|
Comment 17•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/97e179d4873b https://hg.mozilla.org/mozilla-central/rev/df0d060fbe69
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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
Attachment #8702654 -
Flags: approval-mozilla-beta?
Attachment #8702654 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8703106 [details] [diff] [review] Part 2 for beta For beta, we need patch1 + this patch (which is part2)
Attachment #8703106 -
Flags: approval-mozilla-beta?
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.
Attachment #8702655 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8703106 -
Attachment description: Patch for beta → Part 2 for beta
Comment hidden (obsolete) |
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+
Attachment #8702654 -
Flags: approval-mozilla-beta?
Attachment #8702654 -
Flags: approval-mozilla-beta+
Attachment #8702654 -
Flags: approval-mozilla-aurora?
Attachment #8702654 -
Flags: approval-mozilla-aurora+
Comment on attachment 8703106 [details] [diff] [review] Part 2 for beta Beta44+
Attachment #8703106 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8702655 [details] MozReview Request: Bug 1234936 - part 2: always focus tabs when they show an onbeforeunload dialog, r=billm Aurora45+
Attachment #8702655 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Flags: needinfo?(arni2033)
Comment hidden (obsolete) |
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
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
Reporter | ||
Comment 30•9 years ago
|
||
This bug was verified on Nightly 46 in Comment 26, so chaning the status now.
Status: RESOLVED → VERIFIED
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
status-b2g-v2.5:
--- → fixed
Comment hidden (obsolete) |
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
You need to log in
before you can comment on or make changes to this bug.
Description
•