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

RESOLVED INVALID

Status

RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 8615212 [details]
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!
Flags: needinfo?(felash)
Flags: needinfo?(azasypkin)
(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!
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?
Flags: needinfo?(felash)
(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.
(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
Flags: needinfo?(azasypkin)
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 ...
Flags: needinfo?(felash)
Flags: needinfo?(azasypkin)
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...
Flags: needinfo?(felash)
(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.
Created attachment 8615838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/30439
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Attachment #8615838 - Flags: review?(felash)
Removing ni? since Julien is on this already.
Flags: needinfo?(azasypkin)
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!
Attachment #8615838 - Flags: review?(felash) → review?(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!
Attachment #8615838 - Flags: review?(azasypkin) → review+
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.
OK, please ask if you need anything !
We no longer need this workaround because of bug 1176926.

master revert: https://github.com/mozilla-b2g/gaia/commit/c159de8bd59b993a9c726ff4424f53fee8acbc05
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.