Closed Bug 1095818 Opened 7 years ago Closed 7 years ago

[email] app startup by notification goes to message list instead of message reader

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S9 (21Nov)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: jrburke, Assigned: jrburke)

References

Details

Attachments

(4 files)

STR:

1) Set up account to receive notifications. To enable faster notifications (at the expense of your battery, network and sanity) you can enable a 20 second or 60 second sync interval for testing via the secret debug menu:
https://wiki.mozilla.org/Gaia/Email/SecretDebugMode#Getting_to_the_Secret_Debug_Menu

2) Completely close the email app by removing it from the recent apps UI.

3) Send email to the account, and wait for the sync to happen in the background.

4) When the notification pops up for the 1 message received, tap on it.

Expected:
You should go to the message reader card for that one message.

Actual:
You end up in the message list for the account.

Affects 2.0 and 2.1. Reproduces on flame. Does not affect master, 2.2+.

Hilarious Cause:

app_messages received two mozSetMessageHandler 'notification' callbacks, one for the click and one for the close. These come in the correct order, click first, close second. 

These get queued inside evt.js via emitWhenListener, because there are no listeners yet, mail_app module has not been executed yet. The queues inside evt.js are event ID based, not a globally ordered queue.

mail_app finally starts up, and registers two listeners, in this order:

appMessages.on('notificationClosed', ...);
appMessages.on('notification', ....);

Since notificationClosed was registered first, it processes its queue first. The default action for 'closed' is to show the message list. We do this in case the notification system starts up email for a 'close' event and then email gets shown in the process, we have something to show.

We gate these sorts of notification and activity notifications via the `gateEntry` functionality inside of mail_app. This was to fix double click entries mainly via activities, in bug 1010867.

The gate discards the 'notification' message, and the UI ends up on the message_list.

However, if appMessages.on('notificationClosed', ...) is moved after the other .on call, it all works out.

So that will be the fix. Funny enough, this does not happen on master. I am still investigating the precise reason, but I expect it is because the evt.emit infrastructure now async notifies, and so I expect actually looks for listeners after mail_app has registered listeners, so it works out.

I will still do the fix on master though, just to keep all the code as similar as possible. There will be a separate pull request for 2.1, and that one should be upliftable to 2.0.
Assignee: nobody → jrburke
Note on the master/2.2 behavior: it was not the evt changes. The app gets the notification message, the gets all the way to mail_app before the notification closed event comes in. So it could be either from changes in how the system sends events, maybe with a delay, or the alameda loader using native promises, so perhaps mail_app gets all set up before the next message comes into the app.

So for safety reasons, still useful to do the same code change on master.
Attached file GitHub pull request
Attachment #8519320 - Flags: review?(bugmail)
For reference, the v2.1 pull request, that should also apply cleanly for v2.0 if it is wanted:
https://github.com/mozilla-b2g/gaia/pull/25965

Cannot use the master pull request due to the cards refactor on master.
Comment on attachment 8519320 [details] [review]
GitHub pull request

(IRC in-joke reference:) That IS one weird trick.  And JS event emitters DO hate it!
Attachment #8519320 - Flags: review?(bugmail) → review+
Merged in master:
https://github.com/mozilla-b2g/gaia/commit/81201e97c502111eddfa506fd934b2a52898c96e

from pull request:
https://github.com/mozilla-b2g/gaia/pull/25964
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attached file v2.1 Gaia pull request
Carrying r=asuth from the master pull request to this pull request. This is the pull request that should be applied to 2.1. 

Master has drifted from 2.1/2.0 due to the card refactoring on trunk, but this change is equivalent, just moving the one listen binding below the other.

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Bug 1001629 introduced handling a notification close event, and that coupled with the gateEntry limit on too many mozMessageHandler events entering email at once, resulted in the behavior seen.

[User impact] if declined:
User will not go directly to the email message when tapping on a notification for a single message, but instead end up on the message list.

[Testing completed]:
On device on 2.1, confirms the fix.

[Risk to taking this patch] (and alternatives if risky):
Extremely low. Just a movement of a listener below the other listener.

[String changes made]:
none
Attachment #8520200 - Flags: approval-gaia-v2.1?
Attachment #8520200 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Comment on attachment 8520200 [details] [review]
v2.1 Gaia pull request

[v2.0 Approval Request Comment]

[Bug caused by] (feature/regressing bug #):
Bug 1001629 introduced handling a notification close event, and that coupled with the gateEntry limit on too many mozMessageHandler events entering email at once, resulted in the behavior seen.

[User impact] if declined:
User will not go directly to the email message when tapping on a notification for a single message, but instead end up on the message list.

Nominating for 2.0 given that a related partner bug has asked for a 2.0? blocking request.

Comment 7 has the commit that was applied to 2.1, and should also apply cleanly to 2.0.

[Testing completed]:
On device on 2.1, confirms the fix.

[Risk to taking this patch] (and alternatives if risky):
Extremely low. Just a movement of a listener below the other listener.

[String changes made]:
none
Attachment #8520200 - Flags: approval-gaia-v2.0?
Hi Kai-Zhen,
Please help to cherry pick to 2.0M.
Thanks!
Blocks: Woodduck
blocking-b2g: --- → 2.0M+
Flags: needinfo?(kli)
Duplicate of this bug: 1095394
Attachment #8520200 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Keywords: verifyme
QA Contact: ckreinbring
This issue has been successfully verified on Flame 2.1, 2.2
Reproducing rate: 0/5
1.Launch email and set Every 5 minutes to check for new message.
2.Send one email to test account.
3.5minutes later,new message notification pop up on top of screen.
4.Tap the notification.
**Email details view dislayed.Tap the notification in drop down list,message details view also be displayed.

Flame 2.1 build:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0

Flame 2.2 new build:
Gaia-Rev        824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d
Build-ID        20141125040209
Version         36.0a1
Status: RESOLVED → VERIFIED
Attached video video
This issue has been verified successfully on woodduck 2.0.
Verify steps: 
1.Launch email and set Every 5 minutes to check for new message.
2.Completely close the email app by removing it from the recent apps UI
3.Send one email to test account.
4.5minutes later,new message notification pop up on top of screen.
4.Tap the notification.
**Email details view displayed.
See attachment:  Woodduck_video.MP4
Reproducing rate: 0/3
Woodduck2.0 build:
Gaia-Rev        d742e375aca6dc1bf3a36638000ad7f5338ef457
Gecko-Rev       d049d4ef127844121c9cf14d2e8ca91fd9045fcb
Build-ID        20141126050313
Version         32.0
This issue has been verified successfully on Flame 2.0.
Verify steps: 
1.Launch email and set Every 5 minutes to check for new message.
2.Completely close the email app by removing it from the recent apps UI
3.Send one email to test account.
4.5 minutes later,new message notification pop up on top of screen.
4.Tap the notification.
**Email details view displayed.
See attachment:  Verify_Flame2.0.MP4
Reproducing rate: 0/5

build version:

Gaia-Rev        f76014fd2c7528493b90d759c68ec3070233d094
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/53ff92e647a0
Build-ID        20150107000201
Version         32.0
Attached video Verify_Flame2.0.MP4
You need to log in before you can comment on or make changes to this bug.