1147479 - Improve the transition when adding an item from the Reading List

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Michael Maslaney [:mmaslaney] ([e-mail])]

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.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Attachment: Needs to be applied overtop of bug 1147444.

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 2

[Blake Winton (:bwinton) (:☕️)]

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

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: Use the updated height here, too.

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

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch.

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

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Transitions example

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

Comment 7

[Blake Winton (:bwinton) (:☕️)]

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

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: This looks much better.

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.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch.

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

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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!

Comment 11

[Blake Winton (:bwinton) (:☕️)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/35fa88282aa6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35fa88282aa6

Comment 14

[Blake Winton (:bwinton) (:☕️)]

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

Comment 15

[Andrei Vaida [:avaida]]

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 16

[Blake Winton (:bwinton) (:☕️)]

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

Approval Request Comment

See Comment 14.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd483f7d9dc8

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/088cbfb10079

Comment 20

[Andrei Vaida [:avaida]]

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.

Comment 21

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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