Closed Bug 1147444 Opened 5 years ago Closed 5 years ago

Improve the transition when deleting 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

(1 file, 3 obsolete files)

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.
Flags: firefox-backlog+
Attached patch bug1147444.diff (obsolete) — Splinter Review
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…  ;)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #8583254 - Flags: ui-review?(mmaslaney)
(Two points, because of the risk of the double-transitionend code not being up to snuff.)
Points: --- → 2
Blocks: 1132074
Iteration: --- → 39.3 - 30 Mar
Flags: qe-verify?
Attachment #8583254 - Flags: ui-review?(mmaslaney) → ui-review+
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…
Attachment #8583254 - Flags: review?(jaws)
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.
Attached patch Patch with fixed height. (obsolete) — Splinter Review
Yeah, the new height was necessary everywhere, so here it is.
(Redirecting to Unfocused, because Jaws isn't accepting reviews currently.)
Attachment #8583254 - Attachment is obsolete: true
Attachment #8583254 - Flags: review?(jaws)
Attachment #8584665 - Flags: ui-review+
Attachment #8584665 - Flags: review?(bmcbride)
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 :\
Attachment #8584665 - Flags: review?(bmcbride) → review-
Oh, and FWIW, normal weight on Windows is 52px, and on Linux is 59.0667px (*sigh*)
(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....
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.
I agree!  I'll definitely do that with the event.propertyName.
Attached patch The next version of the patch. (obsolete) — Splinter Review
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.
Attachment #8584665 - Attachment is obsolete: true
Attachment #8585054 - Flags: ui-review+
Attachment #8585054 - Flags: review?(bmcbride)
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
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.
Attachment #8585054 - Flags: review?(bmcbride) → review-
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.
Attachment #8585054 - Attachment is obsolete: true
Attachment #8585621 - Flags: ui-review+
Attachment #8585621 - Flags: review?(bmcbride)
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.
Attachment #8585621 - Flags: review?(bmcbride) → review+
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Keywords: checkin-needed
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.
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/44d89de94eea
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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
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 8585621 [details] [diff] [review]
A better version of the patch.

Approval Request Comment

See Comment 17.
Attachment #8585621 - Flags: approval-mozilla-beta?
Attachment #8585621 - Flags: approval-mozilla-aurora?
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.
Attachment #8585621 - Flags: approval-mozilla-beta?
Attachment #8585621 - Flags: approval-mozilla-beta+
Attachment #8585621 - Flags: approval-mozilla-aurora?
Attachment #8585621 - 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.