995626 - Fade in tab labels and close buttons on tab opening

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Bug 980360 added "fadeinlabel" and "fullyopen" attributes and a bunch of supporting code. None of this should be needed.

https://tbpl.mozilla.org/?tree=Try&rev=0897936f6567
https://tbpl.mozilla.org/?tree=Try&rev=dce9fd8dcd50
http://compare-talos.mattn.ca/?oldRevs=0897936f6567&newRev=dce9fd8dcd50&server=graphs.mozilla.org&submit=true

Comment 1

[Dão Gottwald [:dao]]

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

This should be mostly self-explanatory, except for this part:

>             if (animate) {
>               mozRequestAnimationFrame(function () {
>                 this.tabContainer._handleTabTelemetryStart(t, aURI);
> 
>                 // kick the animation off
>                 t.setAttribute("fadein", "true");
>-
>-                // 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.
>-                if (this.tabs.length - this._removingTabs.length == 2)
>-                  this.tabContainer.adjustTabstrip();
>               }.bind(this));
>             }

When opening a second tab, this code would prevent that tab's close button from animating, i.e. it would show immediately. Luckily bug 951618 makes this code obsolete.

Comment 2

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

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

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

Untested, rs=me

::: browser/base/content/browser.css
@@ +146,5 @@
>  }
>  
> +.tab-close-button:not([fadein]):not([pinned]),
> +.tab-label:not([fadein]):not([pinned]) {
> +  visibility: collapse;

This means that the label and close button are laid out during the opening and closing of tabs. I'm surprised this didn't negatively affect TART more.

::: browser/base/content/tabbrowser.xml
@@ -1868,5 @@
> -              // The second tab just got closed and we will end up with a single
> -              // one. Remove the first tab's close button immediately (if needed)
> -              // rather than after the tabclose animation ends.
> -              this.tabContainer.adjustTabstrip();
> -            }

It's not fair to say that this is cruft from the fade-in label patch, this is something that should have been caught in the bug that added the close button to the first tab.

Comment 4

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

Rebased, tested, and landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/1adbd1657ba1

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1adbd1657ba1

Comment 6

[Dão Gottwald [:dao]]

We can't uplift this without bug 951618. I think we should just let this ride trains as usual.

Comment 7

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

(In reply to Dão Gottwald [:dao] from comment #6)
> We can't uplift this without bug 951618. I think we should just let this
> ride trains as usual.

I agree, and we may also want some time to tweak the transition parameters as well.

Comment 8

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

Hm, this approach looks a little glitchy to me (especially when the tab strip gets filled up completely and the tabs start shrinking).
I guess there is no way we can do the opacity transition at the same time as the tab expanding?
If that's the case we should probably look for an alternative (design) solution and consider not uplifting the current behavior.
(I know that realization comes a little late – really sorry about that)

Comment 9

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

(In reply to Philipp Sackl [:phlsa] (on PTO Apr 16-23) from comment #8)
> Hm, this approach looks a little glitchy to me (especially when the tab
> strip gets filled up completely and the tabs start shrinking).
> I guess there is no way we can do the opacity transition at the same time as
> the tab expanding?

Not without making the tab opening animation jankier. Should we back this out in the meantime?

> If that's the case we should probably look for an alternative (design)
> solution and consider not uplifting the current behavior.
> (I know that realization comes a little late – really sorry about that)

That's OK. We held off on uplifting because we wanted to make sure that this was the right path anyways.

Comment 10

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> (In reply to Philipp Sackl [:phlsa] (on PTO Apr 16-23) from comment #8)
> > Hm, this approach looks a little glitchy to me (especially when the tab
> > strip gets filled up completely and the tabs start shrinking).
> > I guess there is no way we can do the opacity transition at the same time as
> > the tab expanding?
> 
> Not without making the tab opening animation jankier. Should we back this
> out in the meantime?

Yes, that would be great.
Going on a little tangent: Could changes like these (where we have something working but it needs time to be tweaked and evaluated) be a good reason to revive the UX branch again? We have a similar situation with the arrowpanel animation.

Comment 11

[Dão Gottwald [:dao]]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/de2328f5d6c9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/de2328f5d6c9

Comment 13

[Dão Gottwald [:dao]]

Looks like this is a dead end. If there's a new design, a new bug should be filed for that.