980360 - Fade in tab titles and close buttons when opening a tab

Status: RESOLVED DUPLICATE of bug 995626

(Firefox :: Tabbed Browser, defect)

Description

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

Attachment: Patch v1

Part of the spec for tab opening[1] is to fade in the tab titles and close buttons. This makes the entire opening process look smoother and also removes the jumpiness caused by the ellipsis overflow.

This patch does exactly that. Not sure about the performance implications though. Gijs offered to push this to try for me.

[1] http://phlsa.github.io/ff-tab-opening

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 8386837 [details] [diff] [review]
Patch v1

>+.tab-label,
>+.tab-close-button {
>+  opacity: 1;
>+  transition: opacity 200ms 200ms;
>+}

Remove opacity:1

> .tab-throbber:not([fadein]):not([pinned]),
>-.tab-label:not([fadein]):not([pinned]),
>-.tab-icon-image:not([fadein]):not([pinned]),
>-.tab-close-button:not([fadein]):not([pinned]) {
>+.tab-icon-image:not([fadein]):not([pinned]) {
>   display: none;
> }

Does this regress bug 713287?

Comment 2

[:Gijs (he/him)]

Comment on attachment 8386837 [details] [diff] [review]
Patch v1

Clearing review, please get review from Dão when this has finished:

Baseline: remote:   https://tbpl.mozilla.org/?tree=Try&rev=f641160142bb
Post-patch: remote:   https://tbpl.mozilla.org/?tree=Try&rev=1dc0d13360e4

When those have finished, please retrigger the "s" and "o" things on TBPL at least 4 times each (so that there's 5 runs of each, on both things), and check out:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=f641160142bb&newRev=1dc0d13360e4&submit=true

(if you need help with that, ping me IRL/here when the first ones have appeared)

Comment 3

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

(In reply to Dão Gottwald [:dao] from comment #1)
> Comment on attachment 8386837 [details] [diff] [review]
> Patch v1
> > .tab-throbber:not([fadein]):not([pinned]),
> >-.tab-label:not([fadein]):not([pinned]),
> >-.tab-icon-image:not([fadein]):not([pinned]),
> >-.tab-close-button:not([fadein]):not([pinned]) {
> >+.tab-icon-image:not([fadein]):not([pinned]) {
> >   display: none;
> > }
> 
> Does this regress bug 713287?

I do see a slight jump towards the end of the close animation, but not any more than in the current Nightly. Not sure where I could find the error message mentioned in that bug.

I'm deferring uploading anything new until the try builds are finished.

Comment 4

[Dão Gottwald [:dao]]

> I do see a slight jump towards the end of the close animation, but not any
> more than in the current Nightly. Not sure where I could find the error
> message mentioned in that bug.

The error message was a red herring, it's unrelated.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8386837 [details] [diff] [review]
> Patch v1
> 
> Clearing review, please get review from Dão when this has finished:
> 
> Baseline: remote:   https://tbpl.mozilla.org/?tree=Try&rev=f641160142bb
> Post-patch: remote:   https://tbpl.mozilla.org/?tree=Try&rev=1dc0d13360e4
> 
> When those have finished, please retrigger the "s" and "o" things on TBPL at
> least 4 times each (so that there's 5 runs of each, on both things), and
> check out:
> 
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=f641160142bb&newRev=1dc0d13360e4&submit=true
> 
> (if you need help with that, ping me IRL/here when the first ones have
> appeared)

The TART section here looks terrible. :-(

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=f641160142bb&newRev=1dc0d13360e4&submit=true

Comment 6

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

(In reply to :Gijs Kruitbosch from comment #5)
> The TART section here looks terrible. :-(
> 
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=f641160142bb&newRev=1dc0d13360e4&submit=true

It does :(
I do have one idea for improving that, that I'd like to try. Will post a patch soon.

Comment 7

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

Attachment: Slightly different patch

baseline: https://tbpl.mozilla.org/?tree=Try&rev=9980f293152e
with patch: https://tbpl.mozilla.org/?tree=Try&rev=11b2bd00d55a
compare-talos: http://compare-talos.mattn.ca/?oldRevs=9980f293152e&newRev=11b2bd00d55a&server=graphs.mozilla.org&submit=true

I used the same time measurements from Philipp's original patch. Dao, what do you think about these TART changes?

Comment 8

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

Comment on attachment 8401961 [details] [diff] [review]
Slightly different patch

The patch above has an 8.69% regression on OSX. I think the only way to do this and keep display:none on the tab title during the animation is to introduce a setTimeout that will set an attribute triggering the fadein of the title after 200ms.

Comment 9

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

Attachment: Patch (new approach)

Gijs, what do you think about this? It works pretty well for me. Pushing to tryserver now.

Comment 10

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

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=41633908c8a5
With patch: https://tbpl.mozilla.org/?tree=Try&rev=dce8ecb58255
Compare talos: http://compare-talos.mattn.ca/?oldRevs=41633908c8a5&newRev=dce8ecb58255&server=graphs.mozilla.org&submit=true

Comment 11

[:Gijs (he/him)]

Comment on attachment 8404986 [details] [diff] [review]
Patch (new approach)

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

This looks OK, but can't we make the _fullyOpen property on the tabbrowser tab reflect the attribute value (ie, define a property with getter/setter on the tabbrowser-tab binding)? That way, if anyone messes with the property later, they don't have to remember to do the attribute too (or risk them getting out of sync).

::: browser/base/content/browser.css
@@ +150,2 @@
>  .tab-throbber:not([fadein]):not([pinned]),
> +.tab-label:not([fullyopen]),

I'm assuming you didn't leave the :not([pinned]) because pinned tabs have their label hidden anyway? However, we do that with width:0, and I'm not sure that we shouldn't keep that the way it is, as the consequences of width:0 and display:none aren't the same for accessibility.

If you agree, please leave the :not([pinned]) bit here.

Comment 12

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

Attachment: Alternative patch v1.1

re-requesting review here, just so you can verify that i did as you requested.

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=41633908c8a5
With patch: https://tbpl.mozilla.org/?tree=Try&rev=70dcfaeab02c
Compare talos: http://compare-talos.mattn.ca/?oldRevs=41633908c8a5&newRev=70dcfaeab02c&server=graphs.mozilla.org&submit=true

Comment 13

[:Gijs (he/him)]

Comment on attachment 8405028 [details] [diff] [review]
Alternative patch v1.1

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

LGTM codewise!

::: browser/base/content/tabbrowser.xml
@@ +4063,5 @@
>          <body><![CDATA[
>            if (tab.parentNode != this)
>              return;
>            tab._fullyOpen = true;
> +          tab.scrollTop;

Nit: add a comment here saying you're flushing layout to get the transition for the fullyopen attribute to happen.

@@ +4772,5 @@
> +            this.setAttribute("fullyopen", "true");
> +          } else {
> +            this.removeAttribute("fullyopen");
> +          }
> +          return !!val;

Huh, interesting, a lot of these setters seem to not have a return statement. Anyway, this works for me. :-)

Comment 14

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

(In reply to :Gijs Kruitbosch from comment #13)
> Huh, interesting, a lot of these setters seem to not have a return
> statement. Anyway, this works for me. :-)

I just tested and it's not necessary.

var a = tab._fullyOpen = 6; will have a == 6 while tab._fullyOpen == true. Kinda weird, but maybe that is more expected?

Comment 15

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

As an aside, yeah that should be more expected since it can be expanded to var a = 6; tab._fully open = 6;

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8405028 [details] [diff] [review]
Alternative patch v1.1

So... this patch adds a second transition after the tab width transition is completely done, effectively doubling the overall animation time. This feels significantly more heavyweight and is different from what was prototyped at http://phlsa.github.io/ff-tab-opening/. It doesn't seem like an overall improvement. I should note that without this patch, I have a hard time seeing any jumpiness (not even sure what this means in this context) caused by the ellipsis. Is this maybe worse on OS X than on Windows and Linux?

Bottom line is, I don't think we should take this patch and I certainly don't think it should be on the fast track to Firefox 29. With the other tweaks we've made (e.g. bug 979938), we should be at least on par with if not in a better shape than pre-Australis. I'm not sure how this bug ended up being a priority this late in the game.

Comment 17

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

Philipp, can you try out the build at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-aa0ed1a9e560/try-macosx64/ and let us know if this patch is close enough to your intended goal?

Comment 18

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

Attachment: Alternative patch v1.2

This is the most up-to-date patch that fixes the tab-close regression. Carrying forward r+ from Gijs, but still awaiting feedback from Philipp as to if this approach is acceptable visually.

Comment 19

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

I got feedback from Stephen over IRC. He said that he agreed with Dao that the above patch felt too slow, but he liked the following:
> <shorlander> jaws: this feels pretty decent to me: https://pastebin.mozilla.org/4784887

    .tab-close-button[fadein],
    .tab-label[fadein] {
      transition: opacity 70ms;
    }
     
    .tab-close-button[fullyopen]:not([fadeinlabel]),
    .tab-label[fullyopen]:not([fadeinlabel]) {
      visibility: hidden;
      opacity: .6;
    } 

http://logs.glob.uno/?c=mozilla%23fx-team&s=11+Apr+2014&e=11+Apr+2014#c129117

Comment 20

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

Pushed with Stephen's tweaks: https://hg.mozilla.org/integration/fx-team/rev/15877522f55d

Comment 21

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

Attachment: Patch that landed

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing mochitest-dt failures.
https://hg.mozilla.org/integration/fx-team/rev/f9d1bb9daee8

https://tbpl.mozilla.org/php/getParsedLog.php?id=37674641&tree=Fx-Team

Comment 23

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

Duping forward to bug 995626 since it fixes the same bug but with a different approach.