1018494 - [B2G][Dialer] On the dialer app screen after the suggestions list is empty, tapping where the result count was, will still opens the overlay menu.

Status: RESOLVED FIXED

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

Description

[mhall]

Attachment: Log-cat

Description:
On the dialer app screen, when entering in number combinations that match numbers in the contacts app,  a suggestion list is created. To the right of this list is a small area with the result count listed. Tapping this result count will open the overlay menu with the contact info listed. Once the numbers entered into the dialer don't match anymore, the suggestion list and result count disappear. Tapping on the suggestion area now will produce no action, but tapping where the result count was will still bring up the overlay menu with the contact info listed.
Log-cat issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1010993.

Prerequisites:
Address book has 3 contacts (A:686532455, B:777686727 and C:446864476); only three digits (686) matches as a substring of the three contacts phone number
NOTE: These numbers are just examples.

Repro Steps:
1) Update a Flame to BuildID: 20140530040207
2) Tap the Dialer app from the home screen
3) Type in the three matching numbers (observe suggestion area populates)
4) Tap in a fourth number that doesn't match the contacts (observe suggestions are gone)
5) Tap the area where the result count was (right of suggestion area)
6) Observe device displays the overlay menu with the contact info listed.

Actual:
Tapping where the result count was will still bring up the overlay menu with the contact info listed

Expected:
Tapping where the result count was will result in no action occurring

2.0 Environmental Variables:
Device: Flame 2.0 MOZ
BuildID: 20140530040207
Gaia: 26d8fcab9b61f46451600f39c51e0387ef3c4f88
Gecko: e6f113c83095
Version: 32.0a1
Firmware Version: v1.2-device.cfg
User Agent: Mozilla/5.0(Mobile;rv:32.0) Gecko/32.0 Firefox/30.0

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/7320/
See attached: Logcat, Video

Comment 1

[mhall]

Flame 1.4:
1.4 Environmental Variables:
Device: Flame 1.4 MOZ
BuildID: 20140530000202
Gaia: fe612fd21389193a8e593aa718831602e5086a62
Gecko: 25011f9a8f26
Version: 30.0
Firmware Version: v1.2-device.cfg

User Agent:
Mozilla/5.0(Mobile;rv:30.0)
Gecko/30.0 Firefox/30.0
**********************************************
Buri 1.4:
1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140530000202
Gaia: fe612fd21389193a8e593aa718831602e5086a62
Gecko: 25011f9a8f26
Version: 30.0
Firmware Version: v1.2-device.cfg

User Agent:
Mozilla/5.0(Moble;rv:30.0)
Gecko/30.0 Firefox/30.0
***********************************************
Buri 1.3:
1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140530024008
Gaia: 5bd226b03a2d63dfe9df204f7c0afb9984e8fd42
Gecko: 57cd741e4d0b
Version: 28.0
Firmware Version: v1.2-device.cfg

User Agent:
Mozilla/5.0(Moble;rv:28.0)
Gecko/28.0 Firefox/28.0

Comment 2

[mhall]

After the dialer app screen suggestion list is empty, the overlay menu can still be displayed by tapping where the result count was. This result can be reproduced in Flame 1.4, Buri 1.4, buri 1.3 using the same STR.

Comment 3

[Maria Angeles Oteo (:oteo)]

Paco, can you have a look at it?

Comment 5

[Paco Rampas [:paco]]

Attachment: patch in github

The problem was that the delete button was very close to counter button, therefore when the count button was hidden and you tapped its area, you were really tapping on the delete button. This patch fix this.

Comment 6

[Anthony Ricaud (:rik)]

Comment on attachment 8439014 [details] [review]
patch in github

This is reducing the tapable area of the delete button so I don't really like this solution. We should use a solution similar to bug 1012545.

Comment 7

[Paco Rampas [:paco]]

Comment on attachment 8439014 [details] [review]
patch in github

I think that we can't fix this bug like bug 1012545 (or I don't found the way to do that), because delete button is clickable in the count button area when it is hidden. Therefore, the problem is with size of delete button not with the state :disabled or hidden of suggestion. In addition, this generate other issue. If you do click on area of count button with suggestion hidden first is fired the event of delete button and then the event of count button. Therefore, I have changed the event click for event touchstart of count button. This help to have more control about what event we want to fire.

Comment 8

[Anthony Ricaud (:rik)]

Comment on attachment 8439014 [details] [review]
patch in github

Redirecting to Étienne since he worked on something similar.

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8439014 [details] [review]
patch in github

I'm not sure we want to change the UX here, and show the overlay on touchstart instead of waiting for a click.

The root cause of this bug has nothing to do with the suggestion bar. When tapping in the empty space left by the bar, the event fluffing causes the delete button to get triggered, then we remove 1 digit from the number and match some contacts again, by the time we get to the click we display the overlay.
You can follow the STR but add 2 extras digits and you'll notice that the first tap on the empty space removes 1 digit, the seconds removes another one and displays the overlay.

I think the proper fix would be:
* keep the bar visible at all time (with opacity: 0 when it's empty)
* switch an aria-hidden attribute on it in order not to break accessibility
* keep a click listener on the bar at all time, this will prevent the fluffing from triggering the delete button
* when a tap on the bar occurs while we have to match, early return right away

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8439014 [details] [review]
patch in github

Great! Works like a charm :)
Can you update the suggestion_bar_test.js to reflect the change? Then we'll be good to go!

Comment 11

[Paco Rampas [:paco]]

> Great! Works like a charm :)
> Can you update the suggestion_bar_test.js to reflect the change? Then we'll
> be good to go!

Thanks you :)

I am updating the tests, but this is my first time with this stuff and I haven't much confident. Could you review if all is right? And if is ok, could you merge?

Many thanks.

Comment 12

[Etienne Segonzac (:etienne)]

Comment on attachment 8439014 [details] [review]
patch in github

All good, but I can't merge :/
This PR is making an integration test fail [1].

It's the reflow test for the keypad [2], we're doing 10 reflow instead of 9, probably because we use to lazy load nodes inside a display:none container which is now display:flex+opacity:0.

Now the reflow count should always go down, never up.
But this test wasn't actually created to cover suggestion bar stuff, the fact that we type 3 digits (and not 2 or 4) is a happy accident.

We could:
* add the extra complexity to make sure loading the suggestion bar dom causes no reflow at all, but it will make stuff less readable and bring no benefit while using the phone
* just edit the test to say "10" instead of "9", but that's a bit of a cop-out 
* make the test independent from the suggestion bar

I think the last one is our best bet, and it's a great opportunity for you to discover gaia's marionette tests :)
First you can go here [3] to get set up and running.

Then we'll want to modify marionette/keypad_test.js, we should add a function called |loadSuggestionDOM()| which will do the following
- call typeNumber() to cause the suggestion dom to load
- clear the phone number view, this will look exactly like [4]

Finally, you'll just have to to add a call to |loadSuggestionDOM()| at the beginning of the failing test at it will make it pass since we will only measure the reflows caused by the keypad/number formatting code.

Cheers!

[1] https://tbpl.mozilla.org/?tree=Gaia-Try&rev=bc30d95ffddfa5efe593582aec60676575b03172
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/marionette/keypad_test.js#L66
[3] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_integration_tests
[4] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/marionette/keypad_test.js#L96-102

Comment 13

[Paco Rampas [:paco]]

Merged: ec22d14e2e5c75fd0c22d7772b36095530c13391

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8439014 [details] [review]
patch in github

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: See STR. This will also make the uplift of bug 1035183 cleaner.
[Testing completed]: It has stuck on master for several days.
[Risk to taking this patch] (and alternatives if risky): Low.
[String changes made]: None.

Comment 15

[Loli (:lolimartinezcr)]

Tested and working
2.1
Flame
Gecko-d2e992a
Gaia-bbcf53d

Pending 2.0, 1.4 and 1.3

Comment 16

[Anthony Ricaud (:rik)]

Bhavana: Could you take a look at this approval request?

Comment 17

[Doug Sherk (:drs) (inactive)]

v2.0: https://github.com/mozilla-b2g/gaia/commit/60ad2d7f96c060aea6d66e7718799d439b35311f

Comment 18

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0 & 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/10

Flame v2.0 version:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141208000206
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141208.035628
FW-Date         Mon Dec  8 03:56:38 EST 2014
Bootloader      L1TC00011880

Flame v2.1 version:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880