Closed Bug 1147479 Opened 5 years ago Closed 5 years ago

Improve the transition when adding an item from the Reading List

Categories

(Firefox :: General, defect)

38 Branch
x86
All
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox38 --- verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: mmaslaney, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #8583299 - Flags: ui-review?(mmaslaney)
Attachment #8583299 - Flags: ui-review?(mmaslaney) → ui-review+
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…  :)
Attachment #8583299 - Flags: review?(jaws)
Points: --- → 2
We need to switch the height to 52px here, as well.
(Redirecting to Unfocused, because Jaws isn't accepting reviews currently.)
Attachment #8583299 - Attachment is obsolete: true
Attachment #8583299 - Flags: review?(jaws)
Attachment #8584666 - Flags: ui-review+
Attachment #8584666 - Flags: review?(bmcbride)
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.
Attachment #8584666 - Flags: review?(bmcbride) → review-
Attached patch The next version of the patch. (obsolete) — Splinter Review
(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.)
Attachment #8584666 - Attachment is obsolete: true
Attachment #8585055 - Flags: ui-review+
Attachment #8585055 - Flags: review?(bmcbride)
Flags: qe-verify+
QA Contact: andrei.vaida
Attached file 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.)
Attachment #8585055 - Flags: review?(bmcbride) → review-
Ah, okay.  I'll update the patch(es) to do that instead.  Thanks for the explanation!  :)
Attached patch This looks much better. (obsolete) — Splinter Review
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.
Attachment #8585055 - Attachment is obsolete: true
Attachment #8585619 - Flags: ui-review+
Attachment #8585619 - Flags: review?(bmcbride)
I've left in the two requestAnimationFrames from your example.
Attachment #8585619 - Attachment is obsolete: true
Attachment #8585619 - Flags: review?(bmcbride)
Attachment #8585622 - Flags: ui-review+
Attachment #8585622 - Flags: review?(bmcbride)
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!
Attachment #8585622 - Flags: review?(bmcbride) → review+
Hah!  Right?  :)  It would have been smaller still if I hadn't left the "visible" in from the previous patch…  ;)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35fa88282aa6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Iteration: --- → 40.1 - 13 Apr
Flags: firefox-backlog+
Blocks: 1132074
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.
Status: RESOLVED → VERIFIED
Comment on attachment 8585622 [details] [diff] [review]
The next version of the patch.

Approval Request Comment

See Comment 14.
Attachment #8585622 - Flags: approval-mozilla-beta?
Attachment #8585622 - Flags: approval-mozilla-aurora?
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.
Attachment #8585622 - Flags: approval-mozilla-beta?
Attachment #8585622 - Flags: approval-mozilla-beta+
Attachment #8585622 - Flags: approval-mozilla-aurora?
Attachment #8585622 - Flags: approval-mozilla-aurora+
Target Milestone: Firefox 38 → Firefox 40
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.