Last Comment Bug 722681 - Show the tab close button immediately when the second tab of a window is opened
: Show the tab close button immediately when the second tab of a window is opened
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 13
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on: 876374
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 06:22 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2013-11-13 03:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug (1.55 KB, patch)
2012-02-03 09:32 PST, Jared Wein [:jaws] (please needinfo? me)
fryn: feedback+
Details | Diff | Splinter Review
Patch for bug (1.56 KB, patch)
2012-02-06 15:59 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug v2 (875 bytes, patch)
2012-02-14 13:39 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug v3 (1007 bytes, patch)
2012-02-15 09:23 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug v4 (1.25 KB, patch)
2012-02-15 14:05 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug v5 (1.61 KB, patch)
2012-02-15 14:33 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug v6 (1.52 KB, patch)
2012-02-15 15:40 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v7 (1.26 KB, patch)
2012-02-15 16:05 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-01-31 06:22:09 PST
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) 2012-02-03 09:32:37 PST
Created attachment 594220 [details] [diff] [review]
Patch for bug
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-02-05 03:19:48 PST
fryn: feedback ping?
Comment 3 Frank Yan (:fryn) 2012-02-06 10:35:18 PST
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) 2012-02-06 10:37:26 PST
Comment on attachment 594220 [details] [diff] [review]
Patch for bug

feedback+ with the correct method call. :)
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-02-06 15:59:02 PST
Created attachment 594852 [details] [diff] [review]
Patch for bug

Updated based on Frank's feedback.
Comment 6 Dão Gottwald [:dao] 2012-02-06 16:13:29 PST
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] 2012-02-06 16:47:26 PST
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) 2012-02-14 13:39:45 PST
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. 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] 2012-02-14 13:43:04 PST
(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] 2012-02-15 03:49:16 PST
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) 2012-02-15 09:23:57 PST
Created attachment 597438 [details] [diff] [review]
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] 2012-02-15 13:33:50 PST
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] 2012-02-15 13:41:26 PST
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) 2012-02-15 14:05:58 PST
Created attachment 597554 [details] [diff] [review]
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] 2012-02-15 14:13:06 PST
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) 2012-02-15 14:33:51 PST
Created attachment 597561 [details] [diff] [review]
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] 2012-02-15 14:52:51 PST
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) 2012-02-15 15:40:51 PST
Created attachment 597584 [details] [diff] [review]
Patch for bug v6
Comment 19 Dão Gottwald [:dao] 2012-02-15 15:53:20 PST
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) 2012-02-15 16:05:05 PST
Created attachment 597596 [details] [diff] [review]
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] 2012-02-16 01:58:13 PST
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 23 Ed Morley [:emorley] 2012-02-17 05:44:55 PST
https://hg.mozilla.org/mozilla-central/rev/a84061b8c7c9
Comment 24 ithinc 2012-08-25 18:11:56 PDT
Pinned/hidden tabs are neglected.

Note You need to log in before you can comment on or make changes to this bug.