1029313 - [Dialer][DSDS] With 2 SIMs; The matching result disappears after typing in the last correct digit when dialing

Status: RESOLVED WONTFIX

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

Description

[Eric Chang [:ericcc] [:echang]]

### STR
1. Put 2 SIMs in flame.
2. Have an entry with number in Contacts
3. Open Dialer.
4. Typing the number of the contact in step 2.
https://www.youtube.com/watch?v=EaH6ELnQ-uA

### Actual
5. When putting the last digit, the matching result disappears.

### Expected.
1. The matching result stays.

### Reproduce rate 
100% on Flame with 2 SIMs inserted, both SIM pin unlocked.
100% on Flame with 2 SIMs, and 1 SIM pin unlocked, another one locked. 
Not reproducible on Flame with only 1 SIM inserted.
Not reproducible on Buri.

### Version
Flame aurora v2.1
Gaia      7bb5daea0f37d4371e9daa08045d45fef3b30344
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/304ea70beed9
BuildID   20140623160203
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
t2m.sw.version=B1TC00011220

Buri aurora v2.0
Gaia      7bb5daea0f37d4371e9daa08045d45fef3b30344
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/304ea70beed9
BuildID   20140623160203
Version   32.0a2
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013

Comment 1

[Jason Smith [:jsmith]]

QA Wanted to check 1.4.

Comment 2

[Eric Chang [:ericcc] [:echang]]

Oh, need to make some correction..

### Reproduce rate 
100% on Flame v1.4, v2.0, v2.1 with 2 SIMs inserted, both SIM pin unlocked.
100% on Flame v1.4, v2.0, v2.1 with 2 SIMs, and 1 SIM pin unlocked, another one locked. 

Not reproducible on Flame v1.4, v2.0 with only 1 SIM inserted.
Not reproducible on Buri v1.4, v2.0.

v1.4 & v2.0 video
https://www.youtube.com/watch?v=EaH6ELnQ-uA

### Version
Flame aurora v2.1
Gaia      41cc1de26e4edbe12add0009cdc0bd292f2e94fe
Gecko     https://hg.mozilla.org/mozilla-central/rev/31de1a84b27f
BuildID   20140623160208
Version   33.0a1
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
t2m.sw.version=B1TC00011220

Flame aurora v2.0  <----- 
Gaia      7bb5daea0f37d4371e9daa08045d45fef3b30344
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/304ea70beed9
BuildID   20140623160203
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
t2m.sw.version=B1TC00011220

Buri aurora v2.0
Gaia      7bb5daea0f37d4371e9daa08045d45fef3b30344
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/304ea70beed9
BuildID   20140623160203
Version   32.0a2
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013

Comment 3

[Jason Smith [:jsmith]]

After talking with Wilfred on this, we concluded this was a minor bug because the number is fully known at this point, so the bug is minor overall. On that regard, this isn't a blocker.

Comment 4

[Doug Sherk (:drs) (inactive)]

This is intended behavior. See bug 983054 comment 14. If anyone thinks this still a problem or disagrees, feel free to reopen this.

Comment 5

[Anthony Ricaud (:rik)]

I don't think this is really intended behaviour. We should only clear the suggestion bar when the user taps it. In the video, the user only types on the keypad.

Comment 6

[Doug Sherk (:drs) (inactive)]

Ok, makes sense, and I agree.

Comment 7

[Jorge Prudencio [:jorgep]]

Attachment: [patch] With 2 SIMs the matching result disappears after typing in the last correct digit when dialing.png

Patch's result

Comment 8

[Jorge Prudencio [:jorgep]]

Attachment: 23081.html

Comment 9

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8475857 [details]
23081.html

Unfortunately, while this seems to work on the surface, it's not the correct fix. This will cause the suggestion item to be shown after the user taps on it. This goes against what we decided in bug 983054 comment 14. In addition, this is lacking unit tests. We try to always include at least one new test whenever we fix a regression. Ideally, it should fail without the fix, and pass with the fix.

The way I envision fixing this is the following (if DSDS):
* When a user taps on a suggestion item, the current phone number field is filled with that number. (this is already done and has test coverage)
* The suggestion item is then hidden.
* If the current phone number field changes, we unhide it.
* Only the first step should happen if there is more than one suggestion.

I wrote this out in such a way where each step can be individually unit tested, and I think these are the tests we need.

We should try to leverage as much of the existing code as possible. In particular, we must be sure that we don't break a11y, so we should re-use the code that sets 'aria-hidden' and the 'display'/hidden class/attribute.

Let me know if you have any questions.

Comment 10

[Jorge Prudencio [:jorgep]]

Attachment: 24174.html

Hello Doug, what do you think about this patch?

Thanks!

Comment 11

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8491423 [details]
24174.html

This is ok, but I would prefer storing the last used phone number for searching and compare it to the current one. That way, if _searchCallback() gets called multiple times for some reason, it won't unhide the suggestions on the second one.

Also, instead of clearing the suggestion, we should prevent _updateByContacts() from being called instead of early-returning in _searchCallback().

Finally, the test coverage is not enough. But I assume you weren't asking for feedback on that yet, so that's ok.

Comment 12

[Jorge Prudencio [:jorgep]]

Comment on attachment 8475857 [details]
23081.html

><!DOCTYPE html>
><html>
>  <head>
>    <meta title="Pull Request">
>    <meta http-equiv="refresh" content="5;url=https://github.com/mozilla-b2g/gaia/pull/23081">
>  </head>
>  <body>
>    You will be automatically redirected to <a href="https://github.com/mozilla-b2g/gaia/pull/23081">https://github.com/mozilla-b2g/gaia/pull/23081</a> in 5 seconds... Enjoy the ride! ;-)
>  </body>
></html>

Comment 13

[Jorge Prudencio [:jorgep]]

Comment on attachment 8491423 [details]
24174.html

Hello Doug :-)

I have updated the patch and the tests. Can you review them?

Thanks!

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8491423 [details]
24174.html

Please see my comments in the PR.

Comment 15

[Jorge Prudencio [:jorgep]]

Hello Doug,

I have updated the code and I have left a comment. Can you review it?

Thanks a lot :-)

Comment 16

[Doug Sherk (:drs) (inactive)]

(In reply to Jorge Prudencio [:jorgep] from comment #15)
> Hello Doug,
> 
> I have updated the code and I have left a comment. Can you review it?
> 
> Thanks a lot :-)

After manually testing this, I wasn't able to find a case where the `self._lastSearch != self._phoneNumber.substring(0,self._phoneNumber.length-1)` condition was needed. Just the `contact.tel[firstMatch].value == self._phoneNumber` check seemed to be sufficient to me. Could you give me an exact example where this is needed?

Otherwise, a few of the problems that I pointed out from the last iteration are still there. So I'm going to hold off on another real round of review until they're addressed.

Comment 17

[Germán Toro del Valle (:gtorodelvalle)]

Setting it as unassigned since Jorge won't be working on them for the time being ;)

Comment 18

[BMO Automation]

Firefox OS is not being worked on