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 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 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)

(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.

> 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.

(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

(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.

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 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.

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

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 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.

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 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. :-)

(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?

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

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.

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

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.

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

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

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

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