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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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.
Created attachment 594220 [details] [diff] [review]
Patch for bug
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #594220 - Flags: feedback?(fryn)
fryn: feedback ping?

Comment 3

5 years ago
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 4

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

feedback+ with the correct method call. :)
Attachment #594220 - Flags: feedback- → feedback+
Created attachment 594852 [details] [diff] [review]
Patch for bug

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-
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?
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-
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.
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.
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.
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-
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.
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-
Created attachment 597584 [details] [diff] [review]
Patch for bug v6
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.
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.
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+
http://hg.mozilla.org/integration/mozilla-inbound/rev/a84061b8c7c9
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/a84061b8c7c9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 24

5 years ago
Pinned/hidden tabs are neglected.
Depends on: 876374
You need to log in before you can comment on or make changes to this bug.