Closed Bug 722681 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch Patch for bug (obsolete) — Splinter Review
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #594220 - Flags: feedback?(fryn)
fryn: feedback ping?
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();
Attachment #594220 - Flags: feedback?(fryn) → feedback-
Comment on attachment 594220 [details] [diff] [review]
Patch for bug

feedback+ with the correct method call. :)
Attachment #594220 - Flags: feedback- → feedback+
Attached patch Patch for bug (obsolete) — Splinter Review
Updated based on Frank's feedback.
Attachment #594220 - Attachment is obsolete: true
Attachment #594852 - Flags: review?(ttaubert)
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.
Attachment #594852 - Flags: review?(ttaubert) → review?(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.
Attachment #594852 - Flags: review?(dao) → review-
Attached patch Patch for bug v2 (obsolete) — Splinter Review
(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?
Attachment #594852 - Attachment is obsolete: true
Attachment #597169 - Flags: review?(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 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).
Attachment #597169 - Flags: review?(dao) → review-
Attached patch Patch for bug v3 (obsolete) — Splinter Review
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.
Attachment #597169 - Attachment is obsolete: true
Attachment #597438 - Flags: review?(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.
Attachment #597438 - Flags: review?(dao) → review-
And please add a comment on why adjustTabstrip is called early even though it will be called again when the animation stops.
Attached patch Patch for bug v4 (obsolete) — Splinter Review
(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.
Attachment #597438 - Attachment is obsolete: true
Attachment #597554 - Flags: review?(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.
Attachment #597554 - Flags: review?(dao) → review-
Attached patch Patch for bug v5 (obsolete) — Splinter Review
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.
Attachment #597554 - Attachment is obsolete: true
Attachment #597561 - Flags: review?(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".
Attachment #597561 - Flags: review?(dao) → review-
Attached patch Patch for bug v6 (obsolete) — Splinter Review
Attachment #597561 - Attachment is obsolete: true
Attachment #597584 - Flags: review?(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.
Attached patch Patch for bug v7Splinter Review
(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.
Attachment #597584 - Attachment is obsolete: true
Attachment #597584 - Flags: review?(dao)
Attachment #597596 - Flags: review?(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."
Attachment #597596 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/a84061b8c7c9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Pinned/hidden tabs are neglected.
You need to log in before you can comment on or make changes to this bug.