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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Logcat

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

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Firewatch

Comment 2

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

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

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: correct non-search and search DOM updating to use toggle; still only in the `firstTime` case

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

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

landed in gaia/master:
https://github.com/mozilla-b2g/gaia/pull/19127
https://github.com/mozilla-b2g/gaia/commit/e4d250f043e8d9334e3094103a46ccf1811a6114

Comment 7

[Dylan Oliver [:doliver]]

Blocking 1.4 for regression from bug 796474.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/b40103dec34a147c9018a1af76eb21c3184f2f93

Comment 9

[Edward Chen[:edchen]]

[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