Closed Bug 1008477 Opened 6 years ago Closed 6 years ago
[email/UI] Virtual list regression; attachment icon only ever gets set and is never cleared
. Because of DOM node reuse results in misleading attachment icon status .
Description: The paper clip attachment icon can be placed on Emails without attachments when scrolling quickly through a list of emails Prerequisite: Have a large number of emails and at least 1 attachment on one of those emails Repro Steps: 1) Update a Tarako to BuildID: 20140509014003 2) Open Email app> setup Email 3) Scroll rapidly up and down to the top and bottom of the email list Actual: Emails without attachments will show a paper clip next to them indicating that there are attachments on them Expected:Only emails with attachments on them will show the paper clip next to them 1.3t Environmental Variables: Device: Tarako 1.3t BuildID: 20140509014003 Gaia: d07f2dff31e71aed9711d988677fbc5b2c6e83e4 Gecko: 59b3ea7e75fb Version: 28.1 Firmware Version: SP6821a-Gonk-4.0-4-29 Keywords: Scrolling, Swiping, Fast, Quick, Speed, Incorrect, Icon Notes: This will not happen with a small E-mail list Repro frequency: 100% See attached: Logcat, Firewatch, Video - https://www.youtube.com/watch?edit=vd&v=QHyJz8DtU4c
The issue above DOES occur on Buri 1.4 1.4 Environmental Variables: Device: Buri 1.4 MOZ BuildID: 20140508000201 Gaia: 4ce973ef0732b0d52cb043210db598aa176b2ce9 Gecko: 16ab7f6b18f8 Version: 30.0 Firmware Version: v1.2-device.cfg
Good find! We should update the updateMessageDom variants to use toggle(class, booleanStatus) in all cases rather than using add/remove. This is an interaction of the existing assumption in updateMessageDom that possessing attachments is an immutable state and the change to reuse DOM nodes. This was not caught during the review process because it was a secondary implication and we're currently without automated coverage on this aspect; as I finish cleaning up the automated tests I'll ensure that invariants like this are automatically checked. I'm not sure I'd call this a blocker for v1.4 since we can request approval for it (and I will), but it is a regression, so anyone with blocker+ powers who wants to plus it should feel free.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Summary: [B2G][E-mail]The paper clip attachment icon can be placed on Emails without attachments → [email/UI] Virtual list regression; attachment icon only ever gets set never, cleared. Because of DOM node reuse results in misleading attachment icon status.
Note that I suspect comment 0's mention of v1.3T may be erroneous. The video indicates a version of email with bug 796474 fixed which has not landed on v1.3T Tarako and absolutely must not be uplifted there.
I reproduced on my buri running trunk-ish gecko and master. This fixes it. I did an audit of other classList.add/remove stuff within the file. While there are other spots of code that I think would benefit from use of toggle(class, boolean), they are not message related and so I haven't touched them. (It's probably worth noting that while add/remove is potentially more intuitive in some cases versus passing a negated boolean to toggle, the advantage of it being harder to screw things up would seem to outweigh that. It may be worth adopting a policy of introducing a named temporary in the cases where toggle's argument requires too much thought. Or MailAPI could just grow alternate aliases to avoid the need for negation in the first place.)
Attachment #8420490 - Flags: review?(jrburke)
Summary: [email/UI] Virtual list regression; attachment icon only ever gets set never, cleared. Because of DOM node reuse results in misleading attachment icon status. → [email/UI] Virtual list regression; attachment icon only ever gets set and is never cleared. Because of DOM node reuse results in misleading attachment icon status.
Attachment #8420490 - Flags: review?(jrburke) → review+
Whiteboard: [openc-1.4-exploratory] → [openc-1.4-exploratory][c= p=0.5]
Target Milestone: --- → 2.0 S2 (23may)
Whiteboard: [openc-1.4-exploratory][c= p=0.5] → [openc-1.4-exploratory]
Blocking 1.4 for regression from bug 796474.
blocking-b2g: --- → 1.4+
[Environment] Gaia 917174ee3812a43758bf43f7ba5f9416dcdb2ca8 Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a3a14fb5ad51 BuildID 20140519164002 Version 28.1 ro.build.version.incremental=eng.cltbld.20140519.201203 ro.build.date=Mon May 19 20:12:10 EDT 2014 [Result] PASS
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.