1147444 - Improve the transition when deleting an item from the Reading List

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

When a user deletes a Reading List Item, I'm picturing a transition where the deleted reading item eases out in transparency, and the the following items sightly ease up. 

Blake, I know you had a couple ideas for this.

Comment 1

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

Attachment: bug1147444.diff

Build available at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/DeleteTransition.dmg

I'm not sure how Jaws will like the double-transitionend code, but let's see if the UX is how we want before we start fixing that part…  ;)

Comment 2

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

(Two points, because of the risk of the double-transitionend code not being up to snuff.)

Comment 3

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

Comment on attachment 8583254 [details] [diff] [review]
bug1147444.diff

Hey Jared, I've slightly modified the way this is done in my patch for bug 1147479, so you might want to check that one out in parallel.  Let me know if you wanted a single diff with both the changes to review instead…

Comment 4

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

Note: It looks like I need to use a height of 52px instead of 64px to get it to achieve the same spacing as before.  I'll double check this on Linux and Windows tomorrow.

Comment 5

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

Attachment: Patch with fixed height.

Yeah, the new height was necessary everywhere, so here it is.
(Redirecting to Unfocused, because Jaws isn't accepting reviews currently.)

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8584665 [details] [diff] [review]
Patch with fixed height.

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

::: browser/components/readinglist/sidebar.js
@@ +113,5 @@
>      log.trace(`onItemDeleted: ${item}`);
>  
>      let itemNode = this.itemNodesById.get(item.id);
> +    let transitionend = () => {
> +      if (this.itemNodesById.get(item.id)) {

I wonder if we could simplify this into one branch, doing everything when the second transition ends. Can check that via event.propertyName=="height"

@@ +127,5 @@
> +
> +        this.emptyListInfo.hidden = (this.numItems > 0);
> +      }
> +    }
> +    itemNode.addEventListener('transitionend', transitionend, false);

I'm hoping like hell we don't need something like bug 608589 here. Pretty sure we'll be fine.

::: browser/themes/shared/readinglist/sidebar.inc.css
@@ +31,5 @@
>    flex-flow: row;
>    cursor: pointer;
>    padding: 6px;
> +  opacity: 1;
> +  height: 52px;

This ends up cutting off the line of text for the domain on Linux: http://d.pr/i/13yr7

As much as I dislike explicitly specifying a height, I've come up against this exact situation before, and not found an alternative :\

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and FWIW, normal weight on Windows is 52px, and on Linux is 59.0667px (*sigh*)

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #6)
> I wonder if we could simplify this into one branch, doing everything when
> the second transition ends. Can check that via event.propertyName=="height"

Oh, heh, and I see you're removing this in bug 1147479....

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, or not? I'm getting confused now. Think this would be much simpler with just one branch/listener, handling things at only one point in time.

Comment 10

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

I agree!  I'll definitely do that with the event.propertyName.

Comment 11

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

Attachment: The next version of the patch.

Okay, so.  It turns out we can transition max-height instead of height, and that works with the different heights on Windows and Linux.  I set it to "80px" because I saw "74.<something>px" on my Linux VM with a crazy text-size.

I still have two branches, but the comments in bug 1147479 should explain why.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

See bug 1147479 comment 6.

Comment 13

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

Attachment: A better version of the patch.

Sigh.  Let's pretend I didn't add this to the other bug…

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 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8585621 [details] [diff] [review]
A better version of the patch.

Review of attachment 8585621 [details] [diff] [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.

Yep, makes sense.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/44d89de94eea

Note that Target Milestone tracks landing on mozilla-central and the tracking flags track branch uplifts.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44d89de94eea

Comment 17

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

Uplift request:

[User impact if declined]: There will be a more jarring transition when removing 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-ish risk because its a fairly small change impacting only reader mode.
[String/UUID change made/needed]: none

Comment 18

[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 19

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

Comment on attachment 8585621 [details] [diff] [review]
A better version of the patch.

Approval Request Comment

See Comment 17.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8585621 [details] [diff] [review]
A better version of the patch.

Low risk, we want Reading list to be polished, taking it.
Should be in 38 beta 2 or 3.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c37fc5d94a5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b42a539dcc29

Comment 23

[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 24

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