Fade in tab labels and close buttons on tab opening

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: dao, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30 wontfix)

Details

(Whiteboard: [Australis:P3-])

Attachments

(1 attachment)

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 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.
Attachment #8405770 - Flags: review?(jaws) → review+
Summary: Remove cruft from bug 980360 → Fade in tab labels and close buttons on tab opening
Rebased, tested, and landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/1adbd1657ba1
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1adbd1657ba1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 31
We can't uplift this without bug 951618. I think we should just let this ride trains as usual.
(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.
Depends on: 997382
Depends on: 997681
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)
(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.
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
No longer depends on: 997681
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/de2328f5d6c9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 31 → ---
Assignee: dao → nobody
Looks like this is a dead end. If there's a new design, a new bug should be filed for that.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.