Closed Bug 961637 Opened 9 years ago Closed 9 years ago

[email/UI] Quick advance regressed envelope display

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
tracking-b2g backlog

People

(Reporter: asuth, Assigned: jrburke)

References

Details

(Keywords: regression, Whiteboard: [p=3])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
asuth
: review+
Details | Review
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.
blocking-b2g: --- → 1.4?
Assignee: nobody → jrburke
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Whiteboard: [p=3]
Attached file 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.
Attachment #8366282 - Flags: review?(bugmail)
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.
Attachment #8366282 - Flags: review?(bugmail) → review+
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.
Attachment #8366282 - Flags: review+ → review?(bugmail)
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! :)
Attachment #8366282 - Flags: review?(bugmail) → review+
Merged in gaia master:
https://github.com/mozilla-b2g/gaia/commit/09ae464cb04f056aaf8272636a9d4f51f5f9a395

from pull request:
https://github.com/mozilla-b2g/gaia/pull/15745
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → backlog
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.