978726 - Tapping on notifications in notification tray triggers homescreen icons

Status: RESOLVED DUPLICATE of bug 1041383

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

+++ This bug was initially created as a clone of Bug #978546 +++

When dragging the utility-tray the finger can moved fast enough so that the finger is not over it, but instead is over the current application. As a result touchmove events are dispatched to the applications while dragging the utility tray.

Comment 1

[:gerard-majax]

While working on bug 980701, when I tap on the notification tray with dialer in background, it can triggers dialer actions. This clearly matches the description of this bug.

STR:
 0. Simulate three voicemail notifications, each one with a different voicemail number (hacking voicemail.js's init()). Let's send 'Voicemail1', 'Voicemail2' and 'Voicemail3' notifications in that order. Voicemail1 will be the most down in the notification tray, Voicemail3 the top one. Do nothing in the 'onclick' handler of the notification
 1. Start B2G desktop, you have three pending voicemail notifications
 2. Start dialer, place a call to 111
 3. Place a second call to 222
 4. First call is put on hold, second is running
 5. Pull down the notification tray, tap on one notification

Expected:
 Nothing should happen

Actual:
 Calls are switching state (on hold/not on hold).

It can be reproduced in B2G Desktop using bug 989926. This does not happens if you tap on the tray itself.

Comment 2

[:gerard-majax]

Does not looks like I can reproduce this on real device (tested on Inari).

Comment 3

[:gerard-majax]

Working on bug 1017821, I noticed this is happening at least with Voicemail notifications and now it's getting quite upsetting.

Comment 4

[:gerard-majax]

STR:
 0. Build a PRODUCTION=0 gaia profile
 1. Start Mulet on this profile
 2. Start UI Tests
 3. Send a new Notification()
 4. Tap Home to go back to homescreen
 5. Open notification tray, tap on the notification

Expected:
 Notification is tapped and click handler is triggered

Actual:
 App icon below the notification is receiving the click and the app starts.

Comment 5

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/20850

Etienne, you implemented this code a while back, but git revealed everything to me :)

It's back from https://github.com/mozilla-b2g/gaia/blame/509d752ad053b87f0c9129c144dfe8d3b5c2f302/apps/system/js/GestureDetector.js#L130

The fix has been suggested by Vivien, and as far as I can explain from my memory of this issue, what happens is a race condition between the moment we tap the notification, the moment the tray is starting to move to get hidden, and the event that we generate. Adding the setTimeout(, 0) makes sure that the ordering will be right.

However, I'd need help to see how to write tests for this.

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 8444008 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/20850

I probably copied it from the gallery because we weren't using shared/ so much at the time.

But the GestureDetector is Djf's work IIRC :)

It's way too well commented to be mine ;)

Comment 7

[Jason Smith [:jsmith]]

This is a long-standing bug & not a regression, so I don't think we should block.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8444008 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/20850

I haven't taken the time to carefully read through this bug, so I don't understand exactly what the bug is or how it is arising. If you are mixing click events with events from the gesture detector, then that is probably the fundamental issue.  You would probably get better results if you used gesture detector for everything and listened for 'tap' events instead of 'click'.

In any case I'm giving r- to this patch because of the risk of breaking other clients of gesture detector that expect events to be dispatched synchronously rather than asynchronously.

If you really need this setTimeout, you'll have to add it by adding a new option to the options object that is passed to the GestureDetector() constructor.  Allow a client to pass in asyncEvents: true in that object and then do you setTimeout fix only if that option is passed.  Then, of course, add appropriate documentation at the top of the file explaining what the new option does and under what circumstances a client might want to use that option.

Comment 9

[:gerard-majax]

Sure, sorry for lacking more explanations we investigated it quite some time ago already and I don't remember clearly the reason we found back then.

I'll dig deeper. Meanwhile, one can already notice this reproduces when the notification does not have a click handler.

Comment 10

[:gerard-majax]

Dumping the emitEvent with and without setTimeout():

> Content JS DEBUG at app://system.gaiamobile.org/shared/js/gesture_detector.js:167 in GD.prototype.emitEvent: GestureDetector: emitEvent: tap on notification [link]

Where I'm dumping this.target.className and this.target.getAttribute('role'). This means the emitEvent() is done against the proper target, i.e., the notification itself.

Comment 11

[:gerard-majax]

I'm wondering if bug 1044406 and bug 1041383 will not fix this.

Comment 12

[:gerard-majax]

So after checking, it turns out bug 1041383 fixed this.