Current transition: http://cl.ly/0Q3o0r1Z1V0g We can improve this transition by adding in some easing, in addition to having the newly added item slide down from the top.

What do you think about this, Michael? A build with all the transitions is up at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/Transitions.dmg

Comment on attachment 8583299 [details] [diff] [review] Needs to be applied overtop of bug 1147444. Don't forget to read the comment in bug 1147444… :)

We need to switch the height to 52px here, as well. (Redirecting to Unfocused, because Jaws isn't accepting reviews currently.)

Comment on attachment 8584666 [details] [diff] [review] Use the updated height here, too. Review of attachment 8584666 [details] [diff] [review]: ----------------------------------------------------------------- Plus comments in bug 1147444 about simplifying the transition event handling. ::: browser/themes/shared/readinglist/sidebar.inc.css @@ +32,5 @@ > cursor: pointer; > padding: 6px; > opacity: 1; > height: 52px; > + transition: opacity 150ms ease-in-out, height 150ms ease-in-out; It would be worth testing this on a really low-end machine (like the Asus T100's floating about, that the perf team recommends), to ensure it doesn't suffer from jank. I was less concerned when the height transition was delayed. It should be fine, but just in case... @@ +122,5 @@ > opacity: 0; > + height: 52px > +} > + > +.item.removed { I don't see why you need the .added and .removed classes. Could probably simplify even further by combining the .adding and .removing classes into one .concealed class - and transition in/out of that for both adding and removing.

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #4) > ::: browser/themes/shared/readinglist/sidebar.inc.css > @@ +32,5 @@ > > + transition: opacity 150ms ease-in-out, height 150ms ease-in-out; > It would be worth testing this on a really low-end machine (like the Asus > T100's floating about, that the perf team recommends), to ensure it doesn't > suffer from jank. I was less concerned when the height transition was > delayed. It should be fine, but just in case... > > @@ +122,5 @@ > > +.item.removed { > I don't see why you need the .added and .removed classes. Could probably > simplify even further by combining the .adding and .removing classes into > one .concealed class - and transition in/out of that for both adding and > removing. I'm using the .removing/.removed (and the .pre-adding/.adding) classes to transition first the opacity, and then the max-height (when removing, that is. When adding, I transition first the max-height and then the opacity). And I'm doing them in order to make the transition both less confusing, and (hopefully) less janky. ;) I'll test it on a low-end netbook on Monday, though, just to make sure. (I also switched this to max-height, for the same reasons as bug 1147444.)

I still don't think that's needed - it's way too complex for what it does. I'm attaching some example code to show what I mean. (The requestAnnimationFrame calls are needed due to reflow optimization and to avoid needing to force a sync reflow. I don't understand why your patch was working in the sidebar without that.)

Ah, okay. I'll update the patch(es) to do that instead. Thanks for the explanation! :)

Note: I've added "visible" to the item div so that the items show up at the start. That gets removed in the next patch, but I wanted to leave it in there.

I've left in the two requestAnimationFrames from your example.

Comment on attachment 8585622 [details] [diff] [review] The next version of the patch. Review of attachment 8585622 [details] [diff] [review]: ----------------------------------------------------------------- Well that ended up being a much smaller patch!

Hah! Right? :) It would have been smaller still if I hadn't left the "visible" in from the previous patch… ;)

Uplift request: [User impact if declined]: There will be a more jarring transition when adding items from the reading list. [Describe test coverage new/current, TBPL]: Manual testing, and baking on mozilla-central for two days. [Risks and why]: Low risk because its a very small change impacting only reader mode. [String/UUID change made/needed]: none

Verified fixed on Nightly 40.0a1 (2015-04-02), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment on attachment 8585622 [details] [diff] [review] The next version of the patch. Approval Request Comment See Comment 14.

Comment on attachment 8585622 [details] [diff] [review] The next version of the patch. Low risk, we want Reading list to be polished, taking it. Should be in 38 beta 2 or 3.

Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Removing qe-verify flag as this was verified on Nightly and Beta, and won't be verified on 39 Aurora.