Closed Bug 587043 Opened 10 years ago Closed 9 years ago

Closing the tabcandy window closes the browser

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Firefox 4.0
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: martijn.martijn, Assigned: raymondlee)

References

Details

(Keywords: dataloss, Whiteboard: [Aug-13-testday][good first bug][interaction][b8])

Attachments

(1 file, 2 obsolete files)

(I'm on Windows 7 using the classic theme).
When I click on the close button of the tab candy window, the whole browser closes.
I would expect the tab candy window to disappear only.
Blocks: 585689
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Component: Tabbed Browser → TabCandy
QA Contact: tabbed.browser → tabcandy
I guess it should be expected that closing the window closes the app (tab candy isn't a separate window, it takes over the browser window). But it should be more obvious that tab candy isn't a separate window and how you can get out of it. Based on that, while this isn't exactly a dupe, I think it's kind of the same issue as bug 586990.
agree, this is a big pita
https://bugzilla.mozilla.org/show_bug.cgi?id=588355

:(
This causes dataloss when sessionstore hasn't saved info recently and on the forms where the information isn't saved.  I run into this issue constantly.  Requesting blocking because I truly believe a lot of users are going to have the same issue.  When you go into panorama mode it seems as if this is a new window due to the chrome disappearing and clicking the big red X in the top right corner seems and really should be the only logical way to exit this mode.
blocking2.0: --- → ?
Keywords: dataloss
I don't think this blocks now that bug 588999 has created a tab-candy exit hatch.
blocking2.0: ? → -
Rerequesting blocking status.  Please consider that the big red X catches peoples eyes and Escape isn't that discoverable.  Users are going to be clicking this new button that shows up on their toolbars and will look for a way to get out of it and that big red X looks like the best way to do so.  Clicking so it going to close the browser and lose anything that hasn't been saved (due to either users changing the time between saves or because they just happened to hit the browser close button a few seconds before the next save).  Remember there are also users that have sessionstore disabled.

This wouldn't be an issue if the chrome and content area was replaced and the panorama stuff was just shown in the content area or if it was discoverable that the panorama button at the top right had some sort of label or an icon that actually made sense, like a close button.
blocking2.0: - → ?
assigning to aza for design
Assignee: nobody → aza
Priority: -- → P2
Given the number of people I've observed going for the close window to get out, as well as the anecdotal evidence closing the window what Tab Candy is open should return you to the previously active tab.

Requesting blocking.
Assignee: aza → ian
Whiteboard: [Aug-13-testday] → [Aug-13-testday][good first bug][interaction][b8]
Target Milestone: --- → Firefox 4.0
Blocks: 597043
Assignee: ian → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Pushed to try
Attachment #476713 - Flags: feedback?(ian)
Attachment #476713 - Attachment is patch: true
Attachment #476713 - Attachment mime type: application/octet-stream → text/plain
Attached patch v1 (obsolete) — Splinter Review
A file is missing in the last patch.
Attachment #476713 - Attachment is obsolete: true
Attachment #476713 - Flags: feedback?(ian)
(In reply to comment #9)
> Created attachment 476758 [details] [diff] [review]
> v1
> 
Passed Try.
Attachment #476758 - Flags: feedback?(ian)
Comment on attachment 476758 [details] [diff] [review]
v1

Looks good.
Attachment #476758 - Flags: review?(dietrich)
Attachment #476758 - Flags: feedback?(ian)
Attachment #476758 - Flags: feedback+
blocking2.0: ? → betaN+
Comment on attachment 476758 [details] [diff] [review]
v1

this is an improvement, but i feel like it's not the best solution since it results in things like the window-close keyboard shortcut not actually closing the window the first time. this not only affects people who want to close the window, but also extensions who might depend on closing the window to actually close the window, regardless of what's visible in it.

r=me, but i think it's a band-aid.
Attachment #476758 - Flags: review?(dietrich) → review+
Dietrich: do you know where I can method which is first being called when the x button is pressed?  I know the BrowserTryToCloseWindow() gets called  but I couldn't find the code which triggers the action.   Is the x button defined in XUL?
Aza asked me to comment, so here we go.

Raymond: I think the x button is defined in XUL (I didn't use to think that, but with the firefox menu I think it must be.)
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#807

That's just hooked up to cmd_closeWindow which is hooked up to BrowserTryToCloseWindow
(In reply to comment #14)
> Aza asked me to comment, so here we go.
> 
> Raymond: I think the x button is defined in XUL (I didn't use to think that,
> but with the firefox menu I think it must be.)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#807
>

That line is only included when MENUBAR_CAN_AUTOHIDE is true, therefore, it's the x button when the menubar is hidden on Windows, not the one we see in all platforms. 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#439

> That's just hooked up to cmd_closeWindow which is hooked up to
> BrowserTryToCloseWindow
(In reply to comment #15)
> That line is only included when MENUBAR_CAN_AUTOHIDE is true, therefore, it's
> the x button when the menubar is hidden on Windows, not the one we see in all
> platforms. 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#439

Be that as it may, if we're replicating the window close button in once place, it's relatively safe to assume that that goes down a common path. I would start looking there.
Ideally, we would want to go back to the normal browser mode from Panorama UI only when user clicks on the window x button.  The patch adds code to BrowserTryToCloseWindow() so even user uses File > Close Window or window-close shortcut as mentioned in #comment 12, it would go to the normal browser mode rather than closing the window.  The window x button is not part of the XUL land so if we want to the Panorama UI back to browser mode behaviour, we would need to look into deeper.  Aza/Ian: Is the behaviour implemented in the current patch OK?
Actually, I think we want the same behavior (window exits Panorama UI) whether you click the close button, hit the close key combo, or select close from the menu. The only thing on Dietrich's list I'd be concerned about is add-ons, but only if it's relatively straightforward to make an exception for them.
(In reply to comment #18)
> Actually, I think we want the same behavior (window exits Panorama UI) whether
> you click the close button, hit the close key combo, or select close from the
> menu. The only thing on Dietrich's list I'd be concerned about is add-ons, but
> only if it's relatively straightforward to make an exception for them.

Raymond, given this, is there more to be done on this patch?
Attachment #476758 - Flags: approval2.0?
Attachment #476758 - Flags: approval2.0?
Attachment #476758 - Attachment is obsolete: true
(In reply to comment #19)
> (In reply to comment #18)
> > Actually, I think we want the same behavior (window exits Panorama UI) whether
> > you click the close button, hit the close key combo, or select close from the
> > menu. The only thing on Dietrich's list I'd be concerned about is add-ons, but
> > only if it's relatively straightforward to make an exception for them.
> 
> Raymond, given this, is there more to be done on this patch?

There is no more to be done on this.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/6374793c8fb6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified fixed using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101013 Firefox/4.0b8pre

Thank you!!
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.