893269 - [dialer][contacts] wrong action will occur after opening the contacts through dialer and then tapping on a contact call log

Status: RESOLVED FIXED

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

Description

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

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988
BuildID 20130712070210
Version 18.0
leo

1. launch dialer
2. select contacts option
3. select call log option
4. select contact
5. switch back to the call log (don't hit back)
6. tap on the middle contact log person

Expected: contact will open
Actual: contact will open and the favorite button will activate

Note:
There's other variations of this bug where the wrong contact will open.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #0)
> 6. tap on the middle contact log person

Can you elaborate on what you mean by this?  Is this the "contacts option" button from step 2?  Or a different person from the middle of the call log list?

So far I have not been able to reproduce on a fresh b2g18+v1-train using reference-workload-heavy.

Comment 3

[(Inactive after June) George Duan [:gduan] [:喬智]]

I can reproduce this bug by below steps.

Precondition: 2~3 call logs, 1 contact at least

1. launch dialer
2. select contacts option
3. click one contact
4. select call log option
5. click the log in the middle of the screen (same position as "Add as Favorite" button of contact detail

Expected: contact is openen 
Actual: contact is opened and favorite button is activated 

This is because the click event is propagate to old contact iframe improperly.

Comment 4

[(Inactive after June) George Duan [:gduan] [:喬智]]

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

This patch should be capable to solve this problem.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to George Duan [:gduan] from comment #3)
> 5. click the log in the middle of the screen (same position as "Add as
> Favorite" button of contact detail

Ah, that makes sense.  With the reference-workload data on my Buri the "Add as Favorite" button was scrolled off the screen, so it was not in the same position.

Based on your pull request it looks like the issue is in Dialer.  Moving other to that category.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 775307 [details]
https://github.com/mozilla-b2g/gaia/pull/10961

I think Etienne is the owner for Dialer, so he's probably a good choice for reviewer.

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 775307 [details]
https://github.com/mozilla-b2g/gaia/pull/10961

Wow, nice bug :)

Thanks George for debugging this !

I'm going to send an alternative CSS patch because I want to fix the root cause, sorry for highjacking the bug :)

Comment 8

[Etienne Segonzac (:etienne)]

Attachment: Pointer to gaia PR

Here's a simple css fix, what do you think?

Comment 9

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 775581 [details]
Pointer to gaia PR

Hi Etienne,

I tried your solution and found another bug.

pre-condition: dialer is close, two call log form known contacts at least, two contacts at least

1. click dialer
2. click calllog tab first( not contact )
3. select one log in the middle (then go to this known contact)
4. click the left-top button (then go to calllog)
5. click one log in the middle again

the favorite button is activated unexpected.

Comment 10

[Etienne Segonzac (:etienne)]

I can reproduce the scenario from Comment 9.
I'm a bit puzzled by this...

But it looks like it's not reproducible if I remove the mouse_event_shim.js (used for the keypad and the call screen but not here so it's not preventing us from going through the STR).

Any pointers?
(The event actually making the contact a favorite is a trusted event, so it might be more of a timing issue.)

Comment 11

[Anthony Ricaud (:rik)]

I think this is a good opportunity to remove mouse_event_shim.js from this app. It will improve load time and responsiveness. I'll take a look at this tomorrow.

Comment 12

[David Flanagan [:djf]]

Anthony: see also bug 861735. I've seen an email from the developer working on that bug indicating that she has already removed the shim from the communications apps.  Mahsa is that correct?

Etienne: I haven't looked carefully at the bug details or the patches, so I don't really know what is going on. Replacing the shim seems like the right thing to do, though, so I'm not going to try to figure this one out.

The shim registers a capturing event handler on the window to discard trusted mouse events. Do the dialer or contacts apps use iframes? Maybe event dispatch is different in that case and the capturing listener doesn't get a chance to run.

In any case, removing the shim, here or in bug 861735 seems like the right way to go.

Comment 13

[(Inactive after June) George Duan [:gduan] [:喬智]]

I trace the log and found things as below..
1. all events.isTrusted are false
2. click one of log will generate two click events (first from click itself, second from touchend)
3. the handleTouchEnd generate click event to #contacts-view (that's why it toggle favorite button)

Comment 14

[Mahsa Mojtahedi]

Yes, it is correct. I'm still working on it though. So I don't have a SIM card to make calls, may I ask if there is any way or trick to import some call log? I would like to produce this bug and see how it works without shim. Because in the code, I see contacts uses iframe.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Mahsa Mojtahedi from comment #14)
> Yes, it is correct. I'm still working on it though. So I don't have a SIM
> card to make calls, may I ask if there is any way or trick to import some
> call log? I would like to produce this bug and see how it works without
> shim. Because in the code, I see contacts uses iframe.

I believe the reference workloads include call log data.  From a gaia master checkout run:

  make reference-workload-heavy

This will work even if your target device is 1.1, although data may be slow to render at first while databases are updated.

More info here:

  https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking_Tips_And_FAQ#Reference_Workloads

Comment 16

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

I am sorry but Etienne's patch does not work for me either... I get the same behavior as George mentions in comment 9. When going back to the Call Log using the back key and getting the details of the second entry in my Call Log (the one in the same position as the Favorite button in fact), the Favorite "tag" gets activated or deactivated :-(

BTW, the original bug is solved by Etienne's patch ;-)

How would you like to proceed, Etienne? Should I r+ this bug and open a new one for the back button case. Would you like to extend your patch to cover that case too? Thanks!

Comment 17

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

Since Etienne already submitted a patch ;-)

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

I would suggest to simply remove deps of mouse_event_shim.js for dialer app.
This can prevent this bug and comment 9 .

Comment 19

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

Perfectly fine with the solution if Etienne is also still fine with it as mentioned in comment 10 ;-) Are you Etienne? :-)

Comment 20

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

Should we create a unique PR combining attachment 775581 [details] and attachment 777697 [details] ?

Comment 21

[(Inactive after June) George Duan [:gduan] [:喬智]]

Ok, as I know , Etienne Segonzac (:etienne) (PTO 07/18 -> 08/05) ,

I will combine them into one patch.

Comment 22

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

Great! Thanks!

Comment 23

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

Comment on attachment 777697 [details]
PR to master

Hi George, I just noticed that in the Dialer the long press deletion (to delete the whole number typed) does not work. Either the long press of the '0' key to get a '+'. On the other hand, during a call I no longer hear the long pressing tones when activating the keypad and pressing keys.

All these features work in the latest master so I guess we are kind of breaking something here ;-)

Would you have a look at it please?

Comment 24

[Anthony Ricaud (:rik)]

Clearing the needinfo for Etienne as he is on PTO and he is ok with removing mouse_event_shim.js.

In attachment 777697 [details], you're only removing mouse_event_shim.js. But you also need to convert all mouse* events to touch* events. Things like mouseup, mousedown, mousemove, mouseenter, mouseleave, etc.

Comment 25

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

Since Etienne is on PTO and George is taking care of this bug ;-)

Comment 26

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 777697 [details]
PR to master

Replace all mouse event with touch event.

Comment 27

[(Inactive after June) George Duan [:gduan] [:喬智]]

I found another bug when switching pages, and it happens no matter add this patch or not.

STR:
pre condition: at least one contact , one calllog for that contact

1. click contact bottom tab
2. click contact A
3. click calllog bottom tab
4. click call log from/to A
5. click top-left bottom 
6. click call log from/to A
7. repeat 5, 6

expected: we can see contact A's information
actual:   only have contact list, and click event doesn't work anymore on contact page

I think I should file a new bug for it.

Comment 28

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

Comment on attachment 777697 [details]
PR to master

Looking good to me ;-) Thanks George!

Comment 29

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

+1 to filling a new bug for comment 27 ;-)

Comment 30

[Anthony Ricaud (:rik)]

Comment on attachment 777697 [details]
PR to master

I don't think we should use touchleave. Even though it looks supported in Gecko, it has been removed from the spec.

Also, you need to update other parts of the code. For example, in swiper.js, you can't use evt.pageX directly because touch events don't have this property. It's on a touch object. With your current patch, you can't answer a call if the phone was locked, you can't swipe up.

I also think all changes in oncall.js should be click events. But I'm not really sure.

Comment 32

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 777697 [details]
PR to master

This patch has been modified for below items.
1. support both of touch and mouse event (will depends on window.ontouchstart)
2. listen click events in oncall.js
3. remove redundant variable
4. remove mouseleave event listener

Comment 33

[Mahsa Mojtahedi]

@BenKelly thank you for help. I'm on the latest version of master. I imported call log data with both heavy and light workloads. I was trying all the scenarios above in comments 1,3 and 9 and I'm not able to reproduce this bug w/o shim.

Comment 34

[Anthony Ricaud (:rik)]

Mahsa: If you have a working patch, could you paste it here for comparison? That would help me review George's patch.

George: Thanks for the summary of the changes in your latest patch, it's really helpful.
I'm not sure why we want to support mouse events with your latest patch. Is there any good reason? I don't think any other app does this and it makes the code harder to read. So unless you have a good reason to do that, I'd rather only support touch events.
Also, have you tested DTMF tones after removing the mouseleave event listener? I'm worried that it might regress bug 829406.

Comment 35

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 777697 [details]
PR to master

Patch updated for below items.

1. support both of touch and mouse event (will depends on window.ontouchstart)
2. listen click events in oncall.js
3. remove redundant variable
4. remove mouseleave/touchleave event
5. detect key change by position when touchmove/mousemove (#829406)
6. add method un utils to get x/y position of mousemove/touchmove event
7. replace if else with switch in keypad.js
8. remove comment 8 css, since it cause pb of comment 27 .

In reply to comment 34,
Hi Anthony, you're right, I miss bug 829406, so I update this patch for it. And the test works fine, Please let me know if any other bug you find.

As I know, desktop build doesn't support touch event, so I still keep them in code as most of our other apps do.

In reply to comment 33,
I also tried only remove mouse_event_shim.js in latest gaia, however, we may still have problems like bug 829406 and comment 23 .

Comment 36

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

Hi George! Currently reviewing your new patch ;-)

Should we set attachment 777697 [details] as obsolete?

Comment 37

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

And probably to close the associated PR to avoid "quick fingers" to click on the yummy green merge button :-p

Comment 38

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

Ups, sorry!!! I meant to set attachment 775581 [details] to obsolete and closing Etienne's PR. In fact, I am going to do it right now ;-)

Comment 39

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

Comment on attachment 777697 [details]
PR to master

Looking and working good to me! Thanks George!

Comment 40

[Anthony Ricaud (:rik)]

(In reply to George Duan [:gduan] from comment #35)
> As I know, desktop build doesn't support touch event, so I still keep them
> in code as most of our other apps do.
If B2G desktop doesn't work with touch events, we should fix that. Kevin Grandon sent a mail to dev.gaia a week or two ago. Please remove the code that handles mouse events and abstracts pageXY.

Comment 41

[Anthony Ricaud (:rik)]

Comment on attachment 777697 [details]
PR to master

r- to remove the mouse abstractions. Other than that, the patch looks ok.

Comment 42

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 777697 [details]
PR to master

Patch updated for comment 40.

Comment 43

[Anthony Ricaud (:rik)]

Comment on attachment 777697 [details]
PR to master

Great! Thanks a lot.

Comment 44

[(Inactive after June) George Duan [:gduan] [:喬智]]

Merge to master.
https://github.com/mozilla-b2g/gaia/commit/a883087e04418ff8a7d44f05a3e074388ce5b57b

Comment 45

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

Is it me or using the latest master (as for now) the Dialer key pressing tones are no longer played?

Comment 46

[Leo]

Please lend this bug to v1-train

Comment 47

[(Inactive after June) George Duan [:gduan] [:喬智]]

In reply to comment 45 ,
I tried with latest gaia from master and tested with other devices... key pressing tones work fine both offline and online

Comment 48

[(Inactive after June) George Duan [:gduan] [:喬智]]

Merge to v1-train

https://github.com/mozilla-b2g/gaia/commit/a64f37c3327e4cf2105a16eedaf68727bb2adbde

Comment 49

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: a64f37c3327e4cf2105a16eedaf68727bb2adbde

Comment 50

[Jinyoon]

I just added milestone & whiteboard comment for sync-up with vendor build.