997681 - Allow closing the last tab with a middle click

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

This goes back to bug 456382.

Bug 995626 made us show the close button on the last tab. Should the same change be made for middle click?

Comment 1

[Mardeg]

Woah. Middle-clicking that is for clearing that tab to restart with about:newtab
At least when browser.tabs.closeWindowWithLastTab is false or browser.tabs.closeButtons is set to 2

Comment 2

[Dão Gottwald [:dao]]

This is about browser.tabs.closeWindowWithLastTab being true (the default setting).

Comment 3

[Mardeg]

By "restart" I didn't mean restart the browser, just as a healthy mental state :)

Comment 4

[Mardeg]

(In reply to Dão Gottwald [:dao] from comment #2)
> This is about browser.tabs.closeWindowWithLastTab being true (the default
> setting).
Ok, as long as the non-default setting behaviour isn't changed by this.

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Dão Gottwald [:dao] from comment #0)
> This goes back to bug 456382.
> 
> Bug 995626 made us show the close button on the last tab. Should the same
> change be made for middle click?

Yes, that makes sense.

Comment 6

[Dão Gottwald [:dao]]

The code that needs to change is here:
http://hg.mozilla.org/mozilla-central/annotate/c962bde5ac0b/browser/base/content/tabbrowser.xml#l4264

We need to remove the "this.childNodes.length > 1 || !this._closeWindowWithLastTab" check.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #0)
> Bug 995626 made us show the close button on the last tab.

Actually, bug 951618 did that.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

Comment 9

[:Gijs (he/him)]

Comment on attachment 8412590 [details] [diff] [review]
patch

Review of attachment 8412590 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane to me.

::: browser/base/content/tabbrowser.xml
@@ -3473,5 @@
> -        tabContainer: this,
> -
> -        observe: function (subject, topic, data) {
> -          switch (data) {
> -            case "browser.tabs.closeWindowWithLastTab":

Uh, interesting. On m-c this still has another case for which we're observing (close buttons), but it seems that got removed in bug 865826...

@@ +4226,5 @@
>          if (event.button != 1)
>            return;
>  
>          if (event.target.localName == "tab") {
> +          this.tabbrowser.removeTab(event.target, {animate: true, byMouse: true});

Animating if this is the last tab seems useless. Maybe pass animate: this.childNodes.length == 1 ?

(This is pretty orthogonal to the patch, so I don't much care if you do or don't (want to) do this, just an observation)

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #9)
> > +          this.tabbrowser.removeTab(event.target, {animate: true, byMouse: true});
> 
> Animating if this is the last tab seems useless. Maybe pass animate:
> this.childNodes.length == 1 ?

removeTab won't allow the animation to happen for the last tab.

https://hg.mozilla.org/integration/fx-team/rev/09863b96e772

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09863b96e772

Comment 12

[Paul Silaghi, QA [:pauly]]

Verified fixed 31.0a1 (2014-04-28) Win 7