1011573 - [B2G][Tarako][Messaging] Keyboard auto closes when editing the 'To' field in the messaging app

Status: VERIFIED FIXED

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

Description

[Josh Schmitt [Joshs]]

Attachment: log.txt

Description:
When trying to edit the phone number in the 'to' field the keyboard auto closes.

Repro Steps:
1) Update a Tarako to BuildID: 20140516014003
2) Open the Messaging app
3) Create a new message
4) Type a number into the 'to' field and press enter
5) Select the 'to' field and delete some numbers

Actual:
The keyboard auto closes when editing the 'to' field.

Expected:
The keyboard does not auto close.

1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140516014003
Gaia: 9699b942b6d0c79c64aa33c2c2ed533ee2c9c852
Gecko: dbcefcec570b
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-4-29

Notes:
Repro frequency: 100%
See attached: https://www.youtube.com/watch?v=MfiigErGJnc and logcat

Comment 1

[Josh Schmitt [Joshs]]

Issue does NOT repro in 1.3 Buri. I also tested on Master/trunk and the issue does NOT occur.

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140516024020
Gaia: 96e3fa769a436a2182e6d54088fb41386eb2b5b5
Gecko: b391e28877b3
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Works :
Gaia      d07f2dff31e71aed9711d988677fbc5b2c6e83e4
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/59b3ea7e75fb
BuildID   20140509014003
Version   28.1
ro.build.version.incremental=eng.cltbld.20140509.053424
ro.build.date=Fri May  9 05:34:30 EDT 2014

Busted:
Gaia      fabc8154b31178515795b0a32068257fd5a16269
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/06be0598a307
BuildID   20140510014002
Version   28.1
ro.build.version.incremental=eng.cltbld.20140510.052846
ro.build.date=Sat May 10 05:28:52 EDT 2014

Possibly with : https://github.com/mozilla-b2g/gaia/commit/3be9b8aed46d941576d90c7a14932a1e3e8fbcdd ?

Julien, could you take a look please?

Comment 3

[Julien Wajsberg [:julienw]]

Yeah, this is the best bet. Won't be able to look before monday.

That's exactly why we should not crash land features. I'm really upset now.

Comment 4

[Josh Schmitt [Joshs]]

Correction to comment 0

Correct variables for 1.3T

1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140516014003
Gaia: 9699b942b6d0c79c64aa33c2c2ed533ee2c9c852
Gecko: dbcefcec570b
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-4-29

Comment 5

[Julien Wajsberg [:julienw]]

I think we need to remove this line: [1]

It's unnecessary now that we listen on the 'add' and 'remove' event. The issue here is that when we receive the 'remove' event because we removed the already assimilated recipient, we call this line, which assimilate the recipient again.

Besides closing the keyboard, it also makes it difficult if not impossible to edit an already added recipient. I think it should block for this reason, but I won't work more on this until it actually blocks te release.

[1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/sms/js/smsdraft.js#L76

Comment 6

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

is it a regression? typing in #s in the TO: field does seem slow though
ni? Julien

Comment 7

[Julien Wajsberg [:julienw]]

Yes, regression from the simple draft implementation in bug 989600.

Comment 8

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

triage: 1.3T+ for regression

Comment 9

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

Julien, do you mind taking this? Thanks

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: github PR

Hey Oleg, since you reviewed the simple draft patch, would you kindly review this one too?

I didn't write any unit test because I'm removing a piece of code that wasn't tested (probably because it was not needed). Still, do you think it would make sense to check it's not called?

Comment 11

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Created attachment 8425613 [details] [review]
> github PR
> 
> Hey Oleg, since you reviewed the simple draft patch, would you kindly review
> this one too?
> 
> I didn't write any unit test because I'm removing a piece of code that
> wasn't tested (probably because it was not needed). Still, do you think it
> would make sense to check it's not called?

Hey Julien, it looks fine and I don't see much value in the unit test for that too. But I've noticed one issue:

1. Enter any invalid number eg. "abc" and press Enter;
2. Enter any valid phone number eg. "123" and press Enter;
3. Close app (I closed via app carousel initiated on long Home button press);
4. Open app and observe recipient panel;

Expected result: "abc" (marked as invalid number), "123" or at least "123";
Actual result: "abc" (marked as invalid number), "abc" (looks like it's valid number)

Comment 12

[Julien Wajsberg [:julienw]]

Ok, since I don't have a 1.3 phone here, I'll look at it tomorrow.
Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8425613 [details] [review]
github PR

Hey Oleg,

it's ready for your review again :)

I really thought I tried this in the previous patch, because I included specific code to handle it... that was not working. I also remember we handled this differently on master, but I couldn't find how :(

More information on the PR.

Comment 14

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Comment on attachment 8425613 [details] [review]
> github PR
> 
> Hey Oleg,
> 
> it's ready for your review again :)
> 
> I really thought I tried this in the previous patch, because I included
> specific code to handle it... that was not working. I also remember we
> handled this differently on master, but I couldn't find how :(
> 
> More information on the PR.

Hey Julien,

Issue mentioned in comment 11 is fixed, but I've noticed another one:

1. Enter three invalid numbers in a row (I used "a" "b" "c");
2. Enter message text;
3. Consequently tap on every invalid number to move it to edit mode. I tapped on "a", then "b", then "c" and then press Enter, so that you have the same invalid "a" "b" "c" numbers;
4. Close app (I closed via app carousel initiated on long Home button press);
5. Open app and observe recipient panel.

Expected result: "a" "b" "c" (all marked as invalid numbers) or at least empty recipient panel;
Actual result: "a" (looks liked valid number) "b" (looks like valid number) "c" (marked as invalid number). So if I remove invalid "c" I can send message to invalid "a" and "b".

On v1.4 and master branchs it behaves differently, but also strange:
* If you don't tap on invalid numbers before closing the app, you'll have draft restored without recipients at all;
* If you do the steps above, you'll have draft restored only with two recipients ("a" "b"), both marked as invalid.

Comment 15

[Julien Wajsberg [:julienw]]

For v1.4 and master I've actually seen the issue already (and I think I have a bug opened too), so maybe I'm wrong when I say that it works correctly on master.

I'm looking for the v1.3t patch.

Comment 16

[Julien Wajsberg [:julienw]]

The issue comes from the fact we don't update the list if there is something currently typed (see [1]). This check is necessary because we don't really know the right index to change, we only guess it should be the last one, which is right in most cases, but not here.
As a result, the "to be invalid" recipient is never marked as invalid in the JS object, but it is _visually_ invalid because the class is correctly set, because it was invalid before.

Given the same issue exists on master too (see how the header changes when you do the steps in comment 14, and the draft saving behaviour you found), I'd suggest we don't focus on this for this v1.3t+ patch.
Now I remember that on master we don't save invalid recipients (there is a WIP patch in bug 952519, and that's where I was faced with this issue in the past, and that's why I couldn't find the code in master ;) ). So we can either leave it as is, or don't save invalid recipients like we do on master.

My advice here would be to leave it as is (because it's easier) but I could as well not save invalid recipients if you think it's better. So give me your thoughts!

[1] https://github.com/mozilla-b2g/gaia/blob/44aeb45296737299a988b4d91e3f0c1059caec08/apps/sms/js/thread_ui.js#L2291-L2293

Comment 17

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> The issue comes from the fact we don't update the list if there is something
> currently typed (see [1]). This check is necessary because we don't really
> know the right index to change, we only guess it should be the last one,
> which is right in most cases, but not here.
> As a result, the "to be invalid" recipient is never marked as invalid in the
> JS object, but it is _visually_ invalid because the class is correctly set,
> because it was invalid before.
> 
> Given the same issue exists on master too (see how the header changes when
> you do the steps in comment 14, and the draft saving behaviour you found),
> I'd suggest we don't focus on this for this v1.3t+ patch.
> Now I remember that on master we don't save invalid recipients (there is a
> WIP patch in bug 952519, and that's where I was faced with this issue in the
> past, and that's why I couldn't find the code in master ;) ). So we can
> either leave it as is, or don't save invalid recipients like we do on master.
> 
> My advice here would be to leave it as is (because it's easier) but I could
> as well not save invalid recipients if you think it's better. So give me
> your thoughts!
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> 44aeb45296737299a988b4d91e3f0c1059caec08/apps/sms/js/thread_ui.js#L2291-L2293

Agree that we shouldn't touch that in this particular patch\bug, so let's leave this patch as is (r+, btw :)). 

But I think we should file another bug and confirm that it's (not) a 1.3t blocker. I'm not that concerned about visual indication of valid\invalid recepients, I'm mainly worried about possible consequences of the fact that user can send message to invalid recepient. I'm not sure whether user may have any serious problems because of that (like broken SMS app state or unexpected operator charges). How do you think?

Comment 18

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8425613 [details] [review]
github PR

Travis is red, it seems that it's not our problem though, but can you please ensure that it's green before landing? :)

Comment 19

[Julien Wajsberg [:julienw]]

> I'm mainly worried about possible consequences of the fact that user can send message to invalid recepient.

So, what would happen in that case is that the phone will try to send to a number that results from a letter -> number conversion using the digit where the letter appears on the dialer (abc -> 2, def -> 3, etc). If that results in a valid number in the country, then the SMS will be actually sent, otherwise the network will send back an error.

Comment 20

[Julien Wajsberg [:julienw]]

v1.3t: ffa388c3c00ee7a31216b397acaa07e1cf94bb39

Comment 21

[Montoya Clemmons (MClemmons)]

Verified expected results occurred following the STR from Comment 0, Comment 11, and Comment 14 on latest 1.3T build. Keyboard does not auto close during editing and correct messaging appears when entering specific, unique characters within App. 

1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140602014001
Gaia: 335486c42498fa7a93c21e4d6121199728602ab8
Gecko: 55e4d83019e5
Version: 28.1
Firmware Version: sp6821a-gonk-4.0-5-12
User Agent: Mozilla/5.0 (Mobile; rv:28.1) Gecko/28.1 Firefox/28.1