1171387 - With bug 1162360, Gij messagesApp.send(); fails because it taps the button while the keyboard slides away

Status: RESOLVED INVALID

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: marionette-mocha-gecko.log

See bug 1162360 comment 50. After reproducing manually and inspect the log (attached) you can see the test timed out because it is unable to find the sent message (because it was never sent).

Apparently bug 1162360 make the keyboard disappear too fast (I just remove a next tick!) so the tap did not actually happen.

I can't reproduce it manually, so I tend to think we should fix how we tap the button in the test. However, it might also be possible someone else can reproduce it manually and we should change the behavior of the button instead (i.e. never remove the focus from input when [send] is tapped.)

Oleg or Julien, wdyt? Please advice which option makes more sense and tell me the specific part of the code I should update. Thanks!

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(Clarify on the word *manually*)

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #0)
> See bug 1162360 comment 50. After reproducing manually and inspect the log
> (attached) you can see the test timed out because it is unable to find the
> sent message (because it was never sent).

* After running the tests manually ...

> Apparently bug 1162360 make the keyboard disappear too fast (I just remove a
> next tick!) so the tap did not actually happen.
> 
> I can't reproduce it manually, so I tend to think we should fix how we tap
> the button in the test. However, it might also be possible someone else can
> reproduce it manually and we should change the behavior of the button
> instead (i.e. never remove the focus from input when [send] is tapped.)

* I can't reproduce it manually with my finger on Flame...

> Oleg or Julien, wdyt? Please advice which option makes more sense and tell
> me the specific part of the code I should update. Thanks!

Comment 2

[Julien Wajsberg [:julienw]]

Sorry, I haven't checked the code, so bear with me.

Does it mean that when we press "tap" we get "blur" first, then the keyboard disappear, and so the send button never gets the "tap"?

Could this happen in real life ? Because I think I get the issue where my tap on the send button is not always working. I thought it was because I tapped besides the button, but maybe this is the same issue?

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Sorry, I haven't checked the code, so bear with me.
> 
> Does it mean that when we press "tap" we get "blur" first, then the keyboard
> disappear, and so the send button never gets the "tap"?

If the actual "send" is bind to "click", then yes, it's possible it will not be triggered.

>
> Could this happen in real life ? Because I think I get the issue where my
> tap on the send button is not always working. I thought it was because I
> tapped besides the button, but maybe this is the same issue?

Hum ... if this can happen in real life, should we bind to touchend instead? I have trouble locate the actual code path.

I vaguely remember we had dealt with similar issue all the way before v1.0. Not entire sure how that part of Gaia works these days.

Comment 4

[Oleg Zasypkin [:azasypkin]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #3)
> Hum ... if this can happen in real life, should we bind to touchend instead?
> I have trouble locate the actual code path.

We're bound to send button's click in two places [1] and [2];

But I'm wondering how bad/unsuitable (considering that the issue can't happen in real life of course) it'd be if we just switch to marionette click instead of marionette tap in [3] (seems it fixes integration tests for me)?

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Could this happen in real life ? Because I think I get the issue where my
> tap on the send button is not always working. I thought it was because I
> tapped besides the button, but maybe this is the same issue?

Are you experiencing this on master/v2.2?


[1] https://github.com/mozilla-b2g/gaia/blob/3c3d9206ca6de0f85a6df8c12c3aec8e2800912a/apps/sms/views/conversation/js/conversation.js#L135
 
[2] https://github.com/mozilla-b2g/gaia/blob/3c3d9206ca6de0f85a6df8c12c3aec8e2800912a/shared/js/multi_sim_action_button.js#L17

[3] https://github.com/mozilla-b2g/gaia/blob/3c3d9206ca6de0f85a6df8c12c3aec8e2800912a/apps/sms/test/marionette/lib/messages.js#L301

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks Oleg. I can confirm changing tap() to click() fixes the tests with my Gecko patch applied. So the question now is again what to change (the click event binding or the test call).

Is it possible to make a quick decision here? I don't want to block bug 1162360 any longer ...

Comment 6

[Julien Wajsberg [:julienw]]

Tim, sorry to be stubborn here, but do you think it's right that we don't get the "click" if we get all the touch events ? I think it's not...

To unblock you can use "click" in the marionette tests, however I think we should fix the underlying issue ASAP...

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Tim, sorry to be stubborn here, but do you think it's right that we don't
> get the "click" if we get all the touch events ? I think it's not...

The touchend event always fired from the same element where the touchstart fired, even if it had moved (or even if the finger have moved).

> To unblock you can use "click" in the marionette tests, however I think we
> should fix the underlying issue ASAP...

I'll submit a pull request for that.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/30439

Comment 9

[Oleg Zasypkin [:azasypkin]]

Removing ni? since Julien is on this already.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8615838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/30439

I'll defer to Oleg who already ran this change locally and knows the marionette tests better than me.

Please file a separate bug for the click-after-keyboard-hide issue, I really think we need to fix this. Please add the bug # near your change in the marionette tests too.

Thanks!

Comment 11

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8615838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/30439

LGTM, I've rerun Gij 11/12 ~30x times to make sure that we don't have any intermittents.

Please, make sure that they're all green before landing.

Thanks!

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

As green as St. Patrick's Day!

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=1f12cedbaa6abcf70dc703a18a1867aa3168a136

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

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Given the error in bug 1162360 comment 60 I don't think my proposed root cause in this bug is correct. Will identify the real root cause and back this patch out and INVALID this bug.

Comment 14

[Julien Wajsberg [:julienw]]

OK, please ask if you need anything !

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

We no longer need this workaround because of bug 1176926.

master revert: https://github.com/mozilla-b2g/gaia/commit/c159de8bd59b993a9c726ff4424f53fee8acbc05