722681 - Show the tab close button immediately when the second tab of a window is opened

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

When there’s only one tab, and you open a new tab, the original tab’s close button only appears after the opening animation.

For improved perceived performance, we should show the tab close button immediately when the second tab is opened instead of waiting for the animation to finish.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

fryn: feedback ping?

Comment 3

[Frank Yan (:fryn)]

Comment on attachment 594220 [details] [diff] [review]
Patch for bug

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

Thanks for filing the bug and writing a patch.

::: browser/base/content/tabbrowser.xml
@@ +1401,5 @@
>                this._lastRelatedTab = t;
>              }
>  
> +            this.adjustTabstrip();
> +

this should be:
this.tabContainer.adjustTabstrip();

Comment 4

[Frank Yan (:fryn)]

Comment on attachment 594220 [details] [diff] [review]
Patch for bug

feedback+ with the correct method call. :)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug

Updated based on Frank's feedback.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 594852 [details] [diff] [review]
Patch for bug

I'll review this patch as I probably know this code best, having implemented the tab opening animation.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 594852 [details] [diff] [review]
Patch for bug

>               if (this._lastRelatedTab)
>                 this._lastRelatedTab.owner = null;
>               else
>                 t.owner = this.selectedTab;
>               this.moveTabTo(t, newTabPos);
>               this._lastRelatedTab = t;
>             }
> 
>+            this.tabContainer.adjustTabstrip();
>+
>             return t;

>         if (tab.getAttribute("fadein") == "true") {
>-          if (tab._fullyOpen)
>-            this.adjustTabstrip();
>-          else
>+          if (!tab._fullyOpen)
>             this._handleNewTab(tab);

Why did you make this stop handling _fullyOpen=true? For the case of opening a new tab, _fullyOpen is false.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v2

(In reply to Dão Gottwald [:dao] from comment #7)
> >         if (tab.getAttribute("fadein") == "true") {
> >-          if (tab._fullyOpen)
> >-            this.adjustTabstrip();
> >-          else
> >+          if (!tab._fullyOpen)
> >             this._handleNewTab(tab);
> 
> Why did you make this stop handling _fullyOpen=true? For the case of opening
> a new tab, _fullyOpen is false.

I removed the handling of _fullyOpen=true since all it did was call this.adjustTabstrip, which I'm now doing earlier. I've now reverted this section in this version of the patch.

Are there other ways that transitionend can be hit where we wouldn't have called adjustTabstrip already?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #8)
> Created attachment 597169 [details] [diff] [review]
> Patch for bug v2
> 
> (In reply to Dão Gottwald [:dao] from comment #7)
> > >         if (tab.getAttribute("fadein") == "true") {
> > >-          if (tab._fullyOpen)
> > >-            this.adjustTabstrip();
> > >-          else
> > >+          if (!tab._fullyOpen)
> > >             this._handleNewTab(tab);
> > 
> > Why did you make this stop handling _fullyOpen=true? For the case of opening
> > a new tab, _fullyOpen is false.
> 
> I removed the handling of _fullyOpen=true since all it did was call
> this.adjustTabstrip, which I'm now doing earlier.

As I said, _fullOpen is false in the case you're dealing with.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 597169 [details] [diff] [review]
Patch for bug v2

This additional adjustTabstrip call shouldn't happen for each new tab but only if the animation is used and there are two tabs in the tabstrip (including the new one).

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v3

I added a conditional so that this would only happen when there are two tabs and the browser.tabs.animate preference is set to true.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 597438 [details] [diff] [review]
Patch for bug v3

There's also the aSkipAnimation parameter. However, instead of adding that check here, just move this up to the code that actually kicks off the animation.

Comment 13

[Dão Gottwald [:dao]]

And please add a comment on why adjustTabstrip is called early even though it will be called again when the animation stops.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v4

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 597438 [details] [diff] [review]
> Patch for bug v3
> 
> There's also the aSkipAnimation parameter. However, instead of adding that
> check here, just move this up to the code that actually kicks off the
> animation.

This can't be moved to where the animation is kicked off because the tab hasn't been appended to the tabContainer at that point. adjustTabstrip uses tabContainer.childNodes for its logic, and calling it before the tab is appended means that the function will think there is only one tab and won't add the close button.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 597554 [details] [diff] [review]
Patch for bug v4

The animation is kicked off after a timeout -- you can move it right there. This will also catch the case of the tab having been pinned in the meantime.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v5

I wasn't sure if this should be placed after _animStartTime is measured or before, but I put it after since this will probably affect the animation speed and it would be good to measure what affect it has.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 597561 [details] [diff] [review]
Patch for bug v5

No need to check browser.tabs.animate here. Also, you can access tabContainer within that function without "self".

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v6

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 597584 [details] [diff] [review]
Patch for bug v6

>+              let self = this;

remove

>+                  // When opening the second tab, show the close button
>+                  // before the transition has ended.

I'd like this comment to point out that:
1) calling adjustTabstrip here is in a way redundant (it's going to be called again as the transition ends) and
2) that's OK, because we're doing it early for a reason.

Right now you're only really communicating the second point.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v7

(In reply to Dão Gottwald [:dao] from comment #19)
> Comment on attachment 597584 [details] [diff] [review]
> Patch for bug v6
> 
> >+              let self = this;
> 
> remove

Whoops, sorry about that.

> >+                  // When opening the second tab, show the close button
> >+                  // before the transition has ended.
> 
> I'd like this comment to point out that:
> 1) calling adjustTabstrip here is in a way redundant (it's going to be
> called again as the transition ends) and
> 2) that's OK, because we're doing it early for a reason.
> 
> Right now you're only really communicating the second point.

OK, I've updated the comment to point out the redundancy.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 597596 [details] [diff] [review]
Patch for bug v7

>+                  // The call to adjustTabstrip here is redundant but is
>+                  // needed so that when opening the second tab, the close
>+                  // button is shown before the transition has ended.

"This call to adjustTabstrip is redundant but needed so that when opening a second tab, the first tab's close buttons appears immediately rather than when the transition ends."

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a84061b8c7c9

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a84061b8c7c9

Comment 24

[ithinc]

Pinned/hidden tabs are neglected.