855814 - Calendar: Notification: Tapping doesn't always launch app and show event.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Mike Lee [:mlee]]

Steps
1. Create a calendar event with a reminder.
2. Wait for the event's reminder notification.
3. Tap on the event's notification.

Result
+ Sometimes calendar app isn't loaded.
+ Sometimes app is loaded but in blank Month View.

Expected
+ Calendar app loads showing the notified event.

Comment 1

[Jason Smith [:jsmith]]

This works for me btw on 4/2/2013 build. Can retest on the latest build?

Comment 2

[Mike Lee [:mlee]]

Still occurs. Retested on:

Device: Unagi
OS: 1.1.0.0-prerelease
Build: 20130329070203

STEPS:

1. Create/accept 2 calendar events, each with a reminder.
2. Wait for both event notifications.
3. Pull down notifications area.
4. If they're duplicate calendar notifications, swipe away the duplicates.
5. Tap on 1 of the events that was duplicated.

ACTUAL:
Notifications view dismissed, home view is shown, nothing else visually happens.

EXPECTED:
Notifications view dismissed, home view is shown, calendar opened and tapped event shown.

Not sure if having duplicate notifications and swiping them away needs to be done to see the problem but that's what I saw and did. I noticed later when pressing and holding the Home button that calendar was running but the preview of the app showed month view not the tapped event's details. Later more reminders occurred with duplicates, I accidentally tapped the bottom most event which was duplicated and saw the same behavior described in the steps above.

Comment 3

[Jason Smith [:jsmith]]

Talked this over with Michael in person and saw a demo of this - this is definitely happening with his calendar. We are getting duplicate notifications and clicking one of them doesn't work at all.

Comment 4

[James Lal [:lightsofapollo]]

Micheal, can you delete your account & resync? Curious to see if its related to state or your events.

Comment 5

[Mike Lee [:mlee]]

I'll try James' suggestion and reply here.

Comment 6

[Mike Lee [:mlee]]

James,
I was able to remove my account but haven't been able to re-add it. The app stays on its Account page with the "Please wait while your account is setup" message and spinner. It's been in this state for more than 10 minutes and I'm still waiting for it to complete. I had this problem earlier today as well but aborted it.

I'll file separate bugs for the problems I ran into try to re-add my calendar account.

Comment 7

[Mike Lee [:mlee]]

(In reply to Michael Lee [:mlee] from comment #6)

2.5 hrs later the Calendar app still shows the "Please wait..." message. I haven't dismissed the app since trying to re-add my account at 1:16pm PDT today. This on it's own is a really bad bug...

Comment 8

[Mike Lee [:mlee]]

Okay, I was finally able to re-add my calendar account, will report back about this bugs main app launch from event notification problem once a few event notifications occur.

FYI, the hanging "Please wait..." message seen when adding an account happens when adding a CalDav account without entering a protocol in the URL field, i.e. just entering a domain; see bug 858866 for details.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

ni? from reporter to confirm the bug

Comment 10

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #9)
> ni? from reporter to confirm the bug

FWIW, Michael confirmed this bug in front of me post triage on Thursday. Additionally, Casey confirmed this same bug as well.

Maybe this is a bug on a per calendar basis?

Comment 11

[Mike Lee [:mlee]]

Bug still occurs on:

Device: Unagi
OS: 1.1.0.0-prerelease
Build: 20130403070204

Launched calendar into blank month view (white page with no day grid shown).

Comment 12

[Mike Lee [:mlee]]

Bug still happening on:

Device: Unagi
OS: 1.1.0.0-prerelease
Build: 20130404070202

Dismissed the last instance of 3 duplicated event notifications via right-swipe on on its notification row then tapped new last duplicated instance. Calendar app didn't launch.

Comment 13

[Wayne Chang [:wchang]]

Triage 4/10 - Leo+ as reproducible and it's an UI failure.

Comment 14

[Wayne Chang [:wchang]]

James, can you take this bug? It is still reproduced.

Comment 15

[James Lal [:lightsofapollo]]

Will take this this week... this likely is present in other branches not really sure what the root cause is yet.

Comment 16

[Jason Smith [:jsmith]]

Here's a simple STR to reproduce this:

1. Create an event that will happen in a few minutes with an alarm reminder that will fire immediately upon creation of the event
2. After the alarm fires, kill the calendar process
3. Try clicking the calendar notification

Result - the calendar app will not launch to show the event

Comment 17

[Mike Lee [:mlee]]

Attachment: Calendar launched with blank Month View

Comment 18

[Evan Tseng [:evanxd]]

Hi James, Jason,

I found out that as user kill the calendar process and then click the calendar notification, the window manager didn't receive the mozChromeEvent from Gecko for launching the app.

If user didn't kill the calendar process, the window manager could receive the mozChromeEvent from Gecko.

So I though that this issue might be a Gecko issue.

How do you think?

Comment 19

[Gene Lian (I already quit Mozilla)]

After tracing the Gecko codes (b2g/chrome/content/shell.js), I found that the Gecko would fire a system message when the app is not launched (instead of through the mozChromeEvent). I'm wondering it's Gaia's responsibility to register a "notification" system message type to catch that?

The Dialer App seems working well for the same notification clicking behaviour. The Calendar App should play the same trick.

Comment 20

[Evan Tseng [:evanxd]]

Hi James, 

I'm interesting in this bug.

Could I take this bug?

Thanks. :)

Comment 21

[James Lal [:lightsofapollo]]

Please do!

Comment 22

[Evan Tseng [:evanxd]]

Hi James,

Thanks. :)

Comment 23

[Evan Tseng [:evanxd]]

Hi Gene,

In screenshot notification case, window manager in Gaia didn't received the mozChromeEvent from Gecko.

So It might be a Gecko issue.
You could check this.

Thanks. :)

Comment 24

[Evan Tseng [:evanxd]]

Attachment: Point to GitHub pull request: https://github.com/mozilla-b2g/gaia/pull/9426

Hi James,

Please help me review this patch.

Thanks. :)

Comment 25

[Gene Lian (I already quit Mozilla)]

Wait! Isn't that a Gecko issue?

Comment 26

[Jason Smith [:jsmith]]

(In reply to Gene Lian [:gene] from comment #25)
> Wait! Isn't that a Gecko issue?

Looks like it. The patch here looks quite odd - it's making use mozApps here to launch the app in the calendar app, which makes no sense.

Comment 27

[James Lal [:lightsofapollo]]

@Gene: How would we actually fix this in gecko? When would we launch the app on tap? How would we correlate that tap to the callback registered in the app?

@Jason: This is actually correct.. if you look closely this obtains a reference to itself (the app) and calling launch simply brings the app into the foreground. I have not tried it yet but this looks sane to me so far...

@Evan: Thanks for picking this up so quickly! I will review this today... My general comment is I would like to see some kind of test that covers this case?

Comment 28

[James Lal [:lightsofapollo]]

Comment on attachment 742270 [details]
Point to GitHub pull request: https://github.com/mozilla-b2g/gaia/pull/9426

Requested some minor structural changes to the code / tests... Looks sane to me... I didn't realize we had a system message for notification though it seems kinda obvious retrospectively... Is this documented somewhere?

Please set r? again after comments are addressed.

Comment 29

[Evan Tseng [:evanxd]]

Hi James,

Sorry, as I knew, there is no document for the notification system message.

But we could see the code handled the notification system message in SMS app.

Please refer the GitHub page: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L175

And I'm updating the code for your comments in GitHub.
Please review my code again after I done that.
Thanks. :)

Comment 30

[Evan Tseng [:evanxd]]

Comment on attachment 742270 [details]
Point to GitHub pull request: https://github.com/mozilla-b2g/gaia/pull/9426

Hi James,

Please help me review the patch.

Thanks. :)

Comment 31

[James Lal [:lightsofapollo]]

Evan,

I reviewed the code itself on github (and left comments) there... Did you forget to push the code or something? As far as I can tell its in the same state as when I reviewed it last time... Will check again my time Monday.

Comment 32

[Evan Tseng [:evanxd]]

Hi James,

It's weird.
I already updated my code.

You could see the code I already updated in the GitHub page:
https://github.com/mozilla-b2g/gaia/pull/9426/

Could you check again.
Thanks. :)

Comment 33

[James Lal [:lightsofapollo]]

Evan,

This looks good but can you please add a test ? Should be easy enough to write one now that the code is in the alarm controller and we have similar test cases there.

Comment 34

[Evan Tseng [:evanxd]]

Hi James,

I already added test for notification message handler in alarm_test.js.

Could you check it again? Thanks. :)

Comment 35

[James Lal [:lightsofapollo]]

Thanks Evan, you still need one more test (that when we get the notification we actually fire app.go is really what I am looking for)

Comment 36

[Evan Tseng [:evanxd]]

Hi James,

I already added the #handleNotificationMessages test.
I learned a lot.
Thanks. :)

Comment 37

[James Lal [:lightsofapollo]]

Great work! Thanks for writing the tests :) r=me +++

Comment 38

[James Lal [:lightsofapollo]]

in master https://github.com/mozilla-b2g/gaia/commit/c580c47da1f7cc421c7aa586b177f7dab881baae

Comment 39

[Evan Tseng [:evanxd]]

Hi James,

Thanks. :)

Comment 40

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted c580c47da1f7cc421c7aa586b177f7dab881baae to:
v1-train: 5a8f90c579aebc326f42eff005382cf7314730de

Comment 41

[Julien Wajsberg [:julienw]]

Unfortunately, this broke the linter (gjslint).

Should I back out or will you fix this very quick on both branches ?

Comment 42

[Evan Tseng [:evanxd]]

Hi Julien,

I'm working on this.
I'll fix this very quick.

Thanks. :)

Comment 43

[Evan Tseng [:evanxd]]

Hi Julien,

I already fixed the issues.
And I'm waiting for review.

After we merge the patch in Bug 868340 to master branch, you could merge it to v1-train branch.

Thanks. :)

Comment 44

[Gareth Aye [:gaye] (back from PTO)]

Attachment: Lint fix pull request

Comment 45

[Gareth Aye [:gaye] (back from PTO)]

The other commit 678025d16f0cdc832e1d1193e7ba8989a068b613 broke lint. This merge cc04e015ed434ee9de4d210524695a2782a1c8c5 should be uplifted with the bug work.

Comment 48

[James Lal [:lightsofapollo]]

Bug 867652- covers very similar ground and is tef+ I think we should uplift this as well since it effects similar feature stability and this patch touches similar areas of the code which also causes conflicts... Uplifting this prior to the other would greatly simplify the uplift and keep this code path consistent in master/v1.0.1

Comment 49

[James Lal [:lightsofapollo]]

in v1.0.1: https://github.com/mozilla-b2g/gaia/commit/60f22bf386163c5866ba9ffdb1c4b2af74afec59

Comment 50

[Jason Smith [:jsmith]]

Verified with one bug caught by caveat - if you select a notification to see the event while the calendar app is in the background or foreground, then the back button becomes non-functional in the UI view two times in a row. The third click will work.

Comment 51

[Jason Smith [:jsmith]]

Verified on b2g18 as well. No issues on b2g18, so the issue seen in comment 50 likely is a 1.01 specific bug. Not a blocker, so I'm wont fixing that bug.