Closed
Bug 951618
Opened 11 years ago
Closed 11 years ago
Always show the close button on the last tab
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
VERIFIED
FIXED
Firefox 31
Tracking | Status | |
---|---|---|
firefox31 | --- | verified |
People
(Reporter: phlsa, Assigned: vtsatskin)
References
Details
(Whiteboard: p=2 s=it-31c-30a-29b.3 [qa!])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
It is already possible to close the last tab using the menu or the keyboard shortcut. Firefox should be consistent here and also offer a close button on the last tab of a window.
Updated•11 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: [triage]
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Whiteboard: [triage]
Reporter | ||
Comment 2•11 years ago
|
||
There should be renewed discussion about this issue.
Bug 724555 is a wontfix that references bug 455990. The main argument there is that removing the close button was consistent with Safari and a bunch of text editors back in 2008.
Today, both IE and Chrome show a close button at all times and close the window when it is clicked. But above all, hiding the button is an inconsistency within Firefox.
Under these circumstances I think there are enough reasons to rethink this decision.
I'm not sure if reopening the bug is the right thing to do in terms of process. Dão, perhaps you can help me out here.
@Marco, does this mean that we should include the bug into the triage again?
Status: RESOLVED → REOPENED
Flags: needinfo?(mmucci)
Flags: needinfo?(dao)
Resolution: DUPLICATE → ---
Updated•11 years ago
|
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Whiteboard: [triage]
Updated•11 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: [feature] p=0
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → vtsatskin
Assignee | ||
Comment 3•11 years ago
|
||
I've gone ahead and created a new preference called browser.tabs.closeButtonOnLastTab. When the preference is enabled, the close button will always be shown on the last tab, otherwise the opposite is true, the close button will never be shown on the last tab.
Background and previous behaviour:
Through implementing the preference, I discovered there were four different modes of displaying close buttons. The mode is controlled by the browser.tabs.closeButtons preference. The modes are:
- close buttons only shown on active tab (closeButtons == 0)
- close buttons shown on all tabs (closeButtons == 1)
- no close buttons at all (closeButtons == 2)
- close buttons at end of tabstrip (closeButtons == 3)
Previously if closeWindowWithLastTab was set to true, the close tab button would be hidden when only one tab was present. If it was set to false, the button would be shown but pressing it would close the tab and open a new "new tab" page.
In closeButtons modes 2 and 3, the close tab button will never be shown, even if the closeButtonOnLastTab is set to true.
Changed Behaviour:
Previously in mode 0 and 1, the close button being shown on the last would be controlled by the closeWindowWithLastTab preference (as described earlier). Now however, closeWindowWithLastTab only controls if the window will close when closing the last tab (or a new tab page appears instead). closeButtonOnLastTab will control the showing the close button on the last tab.
If closeButtons are in mode 2 or 3, closeButtonOnLastTab will have no effect on the tab close button because none should be shown.
Test cases have been written to handle all combinations of these preferences, so if my explanation is not clear, the test should define the behaviour more explicitly.
Assignee | ||
Comment 4•11 years ago
|
||
Other patch notes:
- I've removed a bunch of rouge hidden characters in firefox.js
- The browser.tabs.closeButtonOnLastTab is set to false by default to align with current behaviour. If we choose to go through with the changes, it will be a simple line change.
Assignee | ||
Updated•11 years ago
|
Attachment #8390963 -
Flags: review?(mconley)
Attachment #8390963 -
Flags: review?(dao)
Attachment #8390963 -
Flags: review?(bmcbride)
Reporter | ||
Comment 5•11 years ago
|
||
I think the best way to go forward from here is to land a patch that always shows the button and where the button closes the window on Nightly for some time and see if there are any reactions. Unless the feedback is hugely negative I'd move forward with it because of the external consistency gain.
Comment 6•11 years ago
|
||
Can we avoid adding a pref here? Doesn't seem necessary to make this pref-controllable.
Comment 7•11 years ago
|
||
Comment on attachment 8390963 [details] [diff] [review]
951618.patch
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #6)
> Can we avoid adding a pref here? Doesn't seem necessary to make this
> pref-controllable.
yep
Attachment #8390963 -
Flags: review?(mconley)
Attachment #8390963 -
Flags: review?(dao)
Attachment #8390963 -
Flags: review?(bmcbride)
Attachment #8390963 -
Flags: review-
Flags: needinfo?(dao)
Assignee | ||
Comment 8•11 years ago
|
||
I've removed the proposed pref entirely. Close buttons will always be shown in closeButton modes 0 and 1, never for 2 and 3.
Assignee | ||
Updated•11 years ago
|
Attachment #8393697 -
Flags: review?(gavin.sharp)
Attachment #8393697 -
Flags: review?(dao)
Comment 9•11 years ago
|
||
Comment on attachment 8393697 [details] [diff] [review]
951618.no.pref.patch
Please remove this rule:
http://hg.mozilla.org/mozilla-central/annotate/d6e549d77ab2/browser/themes/osx/browser.css#l2721
It's dead code now.
Otherwise this looks good. I only reviewed the tabbrowser.xml change, don't think there's much value in the test.
Attachment #8393697 -
Flags: review?(dao) → review-
Updated•11 years ago
|
Attachment #8390963 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8393697 -
Attachment is obsolete: true
Attachment #8393697 -
Flags: review?(gavin.sharp)
Attachment #8394384 -
Flags: review?(dao)
Comment 11•11 years ago
|
||
Comment on attachment 8394384 [details] [diff] [review]
951618.patch
Again, I only reviewed the code changes, not the test.
Attachment #8394384 -
Flags: review?(dao) → review+
Updated•11 years ago
|
Whiteboard: [feature] p=0 → [feature] p=2
Updated•11 years ago
|
Comment 13•11 years ago
|
||
Landed without the test:
https://hg.mozilla.org/integration/fx-team/rev/267197d42c90
Flags: in-testsuite-
Comment 14•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Whiteboard: p=2 → p=2 s=it-31c-30a-29b.3 [qa?]
Comment 15•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #13)
> Landed without the test:
> https://hg.mozilla.org/integration/fx-team/rev/267197d42c90
Dao, do you need a test for this? We can get one created if you think it's valuable.
Florin, please make sure this gets verified fixed.
status-firefox31:
--- → fixed
QA Contact: florin.mezei
Whiteboard: p=2 s=it-31c-30a-29b.3 [qa?] → p=2 s=it-31c-30a-29b.3 [qa+]
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #13)
> Landed without the test:
> https://hg.mozilla.org/integration/fx-team/rev/267197d42c90
What was the rationale for removing the test?
The test was created to define the behaviour of permutations of closeButtons and closeWindowWithLastTab. I see that as value enough to include it.
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #15)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > Landed without the test:
> > https://hg.mozilla.org/integration/fx-team/rev/267197d42c90
>
> Dao, do you need a test for this? We can get one created if you think it's
> valuable.
As per the above discussion, there is a test created for this bug.
Comment 17•11 years ago
|
||
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #15)
> Dao, do you need a test for this?
No.
(In reply to Valentin Tsatskin [:vt] from comment #16)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > Landed without the test:
> > https://hg.mozilla.org/integration/fx-team/rev/267197d42c90
>
> What was the rationale for removing the test?
Because I didn't think reviewing it was a good use of my time.
> The test was created to define the behaviour of permutations of closeButtons
Tests aren't free, they take time to run on every OS for every code change and they often need manual maintenance. I think we'd rather remove the hidden closeButtons pref than spending these resources on it.
> and closeWindowWithLastTab. I see that as value enough to include it.
This code doesn't even check closeWindowWithLastTab anymore. I don't see how this could change accidentally.
Updated•11 years ago
|
QA Contact: florin.mezei → paul.silaghi
Comment 18•11 years ago
|
||
(In reply to Philipp Sackl [:phlsa] from comment #0)
> It is already possible to close the last tab using the menu
What menu?
Comment 19•11 years ago
|
||
Tab right click context menu/close tab is still grayed out. Shouldn't be active ?
Flags: needinfo?(vtsatskin)
Comment 20•11 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #19)
> Tab right click context menu/close tab is still grayed out. Shouldn't be
> active ?
Yes, it should. Please file a new bug.
Flags: needinfo?(vtsatskin)
Reporter | ||
Comment 21•11 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #18)
> (In reply to Philipp Sackl [:phlsa] from comment #0)
> > It is already possible to close the last tab using the menu
> What menu?
I was talking about the OS X menu.
Comment 22•11 years ago
|
||
Verified fixed FF 31.0a1 (2014-04-16) Win 7, OS X 10.8, Ubuntu 13.04.
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Whiteboard: p=2 s=it-31c-30a-29b.3 [qa+] → p=2 s=it-31c-30a-29b.3 [qa!]
Comment 24•11 years ago
|
||
Wouldn't be nice to also close the last tab with middle click on the X button or anywhere in the tab area as it happens with the second tab ?
Flags: needinfo?(dao)
Comment 25•11 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #24)
> Wouldn't be nice to also close the last tab with middle click on the X
> button or anywhere in the tab area as it happens with the second tab ?
-> bug 997681
Flags: needinfo?(dao)
Comment 26•11 years ago
|
||
Thanks, Dao.
You need to log in
before you can comment on or make changes to this bug.
Description
•