961637 - [email/UI] Quick advance regressed envelope display

Status: RESOLVED FIXED

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

Description

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

Bug 918303 regressed envelope display so that we no longer show the e-mail address of the sender or to/cc/bcc recipients.  clearDom() collapses all of the display lines but no logic was added to un-collapse them.

I also noticed that in the DOM there are actually two copies of all of the e-mail addresses of interest.  It would appear that the following sequence happens in the case where a header is directly passed:
- constructor calls headerCursor.setCurrentMessage(this.header)
 - setCurrentMessage causes onCurrentMessage to trigger
  - clearDom gets called
  - postInsert() which calls this.buildHeaderDom()
- postInsert gets called by the normal card setup process

Note that the DOM gets cleared once but then we build it twice without suppression.  This is by inspection right now, but it makes sense, so I'll assume I'm right.


This will need a fix and a test.

I considered backing bug 918303 out to immediately address the problem, but I think too many changes like theme stuff has been layered on top.

Comment 1

[James Burke [:jrburke]]

Attachment: GitHub pull request

The fix was two-fold:

* `clearDom` was a bit aggressive with the innerHTML clearing, just the peep bubbles needed to be removed, and make sure addHeaderEmails removes any previously collapsed state on a peep section if it means for it to be displayed.
* `postInsert` is no longer the right conceptual hook to use to display the message, onCurrentMessage is, so just moved the postInsert work into onCurrentMessage, as the other work done in onCurrentMessage needs to be done for each message displayed anyway. So, no more doubling of the peeps.

I did not attach a test as I feel we are still stabilizing the tests. I would prefer land a fix vs. creating more test code that may cause intermittents, and this is a very noticeable regression that impacts people dogfooding on master.

Comment 2

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

Comment on attachment 8366282 [details] [review]
GitHub pull request

Agree on the test front.  Possible comment for consideration to add up on the pull request.

Comment 3

[James Burke [:jrburke]]

Comment on attachment 8366282 [details] [review]
GitHub pull request

Instead of a comment, I decided to opt for a code change to enforce that the DOM is there, diff from previous review point:

 apps/email/js/cards/message_reader.js | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/apps/email/js/cards/message_reader.js b/apps/email/js/cards/message_reader.js
index 2489605..05e1be6 100644
--- a/apps/email/js/cards/message_reader.js
+++ b/apps/email/js/cards/message_reader.js
@@ -132,6 +132,17 @@ MessageReaderCard.prototype = {
     this.emit('header');
   },
 
+  postInsert: function() {
+    this._inDom = true;
+
+    // If have a message that is waiting for the DOM, finish
+    // out the display work.
+    if (this._afterInDomMessage) {
+      this.onCurrentMessage(this._afterInDomMessage);
+      this._afterInDomMessage = null;
+    }
+  },
+
   told: function(args) {
     if (args.messageSuid) {
       this.messageSuid = args.messageSuid;
@@ -185,6 +196,13 @@ MessageReaderCard.prototype = {
    *     email we're currently reading.
    */
   onCurrentMessage: function(currentMessage) {
+    // If the card is not in the DOM yet, do not proceed, as
+    // the iframe work needs to happen once DOM is available.
+    if (!this._inDom) {
+      this._afterInDomMessage = currentMessage;
+      return;
+    }
+
     // Set our current message.
     this.messageSuid = null;
     this._setHeader(currentMessage.header);

Rebased to include this change, and flipping review back since the code is different enough, with new properties on the object.

Comment 4

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

Comment on attachment 8366282 [details] [review]
GitHub pull request

Yes, this is definitely safer.  Not sure if there would have been a race before because of how getBody works and is likely to continue to work, but explicit is better than the horrors of refactoring exploding everything! :)

Comment 5

[James Burke [:jrburke]]

Merged in gaia master:
https://github.com/mozilla-b2g/gaia/commit/09ae464cb04f056aaf8272636a9d4f51f5f9a395

from pull request:
https://github.com/mozilla-b2g/gaia/pull/15745