903278 - [Buri][Call log][Contact]Skip to wrong screen when press contact in call log.

Status: RESOLVED FIXED

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

Description

[sync-1]

AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.179
 Firefox os  v1.1
 Mozilla build ID:20130730070228
 
 Created an attachment (id=483305)
 log
 
 DEFECT DESCRIPTION:
 [Call log][Contact]Skip to wrong screen when press contact in call log.
 
  REPRODUCING PROCEDURES:
 1.Open Dialer shortcut
 2.press contact tab
 3.Press a contact A, and enter edit mode
 4.press call log tab
 5.Press contact B in call log
 6.Skip to edit mode of contact A with input keyboard--KO
 
  EXPECTED BEHAVIOUR:
 It should pop up contact B when press in call log
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 Medium
  REPRODUCING RATE:
 3/3
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: log

Comment 2

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

ni? Ayman for the correct flow.

Comment 3

[ayman maat :maat]

(In reply to sync-1 from comment #0)
> AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.179
>  Firefox os  v1.1
>  Mozilla build ID:20130730070228
>  
>  Created an attachment (id=483305)
>  log
>  
>  DEFECT DESCRIPTION:
>  [Call log][Contact]Skip to wrong screen when press contact in call log.
>  
>   REPRODUCING PROCEDURES:
>  1.Open Dialer shortcut
>  2.press contact tab
>  3.Press a contact A, and enter edit mode
>  4.press call log tab
>  5.Press contact B in call log
>  6.Skip to edit mode of contact A with input keyboard--KO
>  
>   EXPECTED BEHAVIOUR:
>  It should pop up contact B when press in call log
>  
>   ASSOCIATE SPECIFICATION:
>  
>   TEST PLAN REFERENCE:
>  
>   TOOLS AND PLATFORMS USED:
>  
>   USER IMPACT:
>  Medium
>   REPRODUCING RATE:
>  3/3
>   For FT PR, Please list reference mobile's behavior:

User should not be able to get into this position.

**PATH**

1) open Dialer app
2) select contact tab
3) select a contact

At this point the contact card should overlay the 'call log' tab, 'contact' tab and 'keypad' tab… so the user should not be able to get to step 4. 5 and 6

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Try implementing by comment #3, but some issues:

If we overlay the tab icons right after entering contact detail view,
we also end up hiding icons right after clicking a call log record. Is this
expected? In other word, the following use case would no longer present (which works):
1. tap call log record A, entering detail view of A
2. *switch back to call log*
3. tap call log record B, switch to detail view of B

Just want to confirm on this since overlay tabs after entering edit view also
behaves good. Thanks a lot!

Comment 5

[ayman maat :maat]

(In reply to KM Lee [:rexboy] from comment #4)
> Try implementing by comment #3, but some issues:
> 
> If we overlay the tab icons right after entering contact detail view,
> we also end up hiding icons right after clicking a call log record. Is this
> expected? In other word, the following use case would no longer present
> (which works):
> 1. tap call log record A, entering detail view of A
> 2. *switch back to call log*
> 3. tap call log record B, switch to detail view of B
> 
> Just want to confirm on this since overlay tabs after entering edit view also
> behaves good. Thanks a lot!

before i answer, do you have a patch of your current solution that i can put on my test phone review please. Thanks 

not clearing ni? request to self pending reference to patch

Comment 6

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hi Ayman,
WIP here: 
https://github.com/rexboy7/gaia/commit/0f5f541d525981c7495bf808e6cc7fd15e191243

You may ping me if you need any help on applying patch. Thanks!

Comment 7

[ayman maat :maat]

(In reply to KM Lee [:rexboy] from comment #6)
> Hi Ayman,
> WIP here: 
> https://github.com/rexboy7/gaia/commit/
> 0f5f541d525981c7495bf808e6cc7fd15e191243
> 
> You may ping me if you need any help on applying patch. Thanks!

thanks, testing.

Comment 8

[ayman maat :maat]

So on the version of you patch i have download i get the following behavior:

**PATH**
0.1) prerequisite: Contacts list contains contacts with valid phone numbers
0.2) prerequisite: Call log contains calls from contacts in contacts list
1) Select dialer icon
2) Select contacts list icon within dialer
3) Select a contact in contact list

**EXPECTED**
contact detail card overlays dialer buttons 'cal log, contact list, keypad'

**ACTUAL**
contact detail card overlays dialer buttons 'cal log, contact list, keypad'
…works for me

4) Select arrow in top left hand corner of contact detail card

**EXPECTED**
contact list is presented along with dialer buttons 'cal log, contact list, keypad'

**ACTUAL**
contact list is presented along with dialer buttons 'cal log, contact list, keypad'
…works for me

5) Select call log

**EXPECTED**
call log is presented along with dialer buttons 'cal log, contact list, keypad'

**ACTUAL**
call log  is presented along with dialer buttons 'cal log, contact list, keypad'
…works for me

6) Select item in call log

**EXPECTED**
(at this point i think it would actually be more practical to call the number that is associated to the selected item in the call log - but i believe this got changed by someone, somewhere, at some time so we will go with what it is doing now)

contact detail card overlays dialer buttons 'cal log, contact list, keypad'

**ACTUAL**
contact detail card overlays dialer buttons 'cal log, contact list, keypad'
…works for me

7) Select arrow in top left hand corner of contact detail card

**EXPECTED**
call log is presented along with dialer buttons 'cal log, contact list, keypad'

**ACTUAL**
call log is presented along with dialer buttons 'cal log, contact list, keypad'
…works for me

so as far as i can see the basic functionality is working fine. 

I have come across some other spurious behavior whilst testing this, but as the patch is WIP I am thinking I will wait until you have finished the patch and test again before discussing or raising any bugs… please needinfo me when you are ready for me to test again or if you see any problems with the main flow i have not spotted.

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Ayman:
Thanks for the detailed test!
I fixed some UI race condition tha may unexpectedly hide the navigation tabs if you tap on call log icon
right after tapping a contact. Not sure whether this solves the spurious behavior you came across.. Would
you take a look again on the following WIP?
https://github.com/rexboy7/gaia/commit/75b8df61c01adfb422271bb80ecb0f7e5e9ec8a1

I think the WIP is almost done for me, though the code communicating contacts app and dialer app may
need assent from owner.

Comment 10

[ayman maat :maat]

(In reply to KM Lee [:rexboy] from comment #9)
> Ayman:
> Thanks for the detailed test!
> I fixed some UI race condition tha may unexpectedly hide the navigation tabs
> if you tap on call log icon
> right after tapping a contact. Not sure whether this solves the spurious
> behavior you came across.. Would
> you take a look again on the following WIP?
> https://github.com/rexboy7/gaia/commit/
> 75b8df61c01adfb422271bb80ecb0f7e5e9ec8a1
> 
> I think the WIP is almost done for me, though the code communicating
> contacts app and dialer app may
> need assent from owner.

testing... will feedback

Comment 11

[ayman maat :maat]

Hi :rexboy

please can i ask you to rebase the patch.. i had some technical issues with my phone which prevented me from testing it, and now i have resolved those there have been modifications to the files you changed so the patch is failing when applied.

Sorry about that. Please needinfo me when you have rebased and i will test as a priority.

Thanks

ni? to Rexboy

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

It's OK. I Rebased on to the latest master. Does it solve the issue?
The commit:
https://github.com/rexboy7/gaia/commit/64177a287fb16d34fe6b39d8c21c96b7e81b6aeb

and the branch if you need: 
https://github.com/rexboy7/gaia/tree/fix-bug903278

Comment 13

[ayman maat :maat]

(In reply to KM Lee [:rexboy] from comment #12)
> It's OK. I Rebased on to the latest master. Does it solve the issue?
> The commit:
> https://github.com/rexboy7/gaia/commit/
> 64177a287fb16d34fe6b39d8c21c96b7e81b6aeb
> 
> and the branch if you need: 
> https://github.com/rexboy7/gaia/tree/fix-bug903278

Thanks Rexboy

From UX PoV the main issue is solved in your patch. nice work

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: patch

Thanks for your feedback Ayman!

Let's put it into review. There are small pieces of modification on both
Contact app and Dialer app so I guess I need reviewers on two sides.
Jose and Anthony would you mind reviewing this patch?

Comment 15

[Jose Manuel Cantera]

for the contacts part I have made the review. I have also put a general comment on Github concerning the display of the nav bar. 

https://github.com/mozilla-b2g/gaia/pull/11981#issuecomment-23935599

Comment 16

[Anthony Ricaud (:rik)]

Comment on attachment 800617 [details]
patch

I discussed the solution here with Ayman and Rex. I think it's better to only hide the navbar when we are in Edit mode.

Comment 17

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 800617 [details]
patch

Just updated the patch. Now I hide navbar as soon as we enter the edit form.
With some modification to clarify the code, and fix a nit that breaks unit test..
Rik would you take a look again?

Comment 18

[Anthony Ricaud (:rik)]

Comment on attachment 800617 [details]
patch

This works as expected. But the patch is missing new tests. I also added some comments on the pull request.

I feel like a transition on translateY(100%) would look better but that could be confirmed with Ayman.

Comment 19

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 800617 [details]
patch

Thanks for reviewing. I just made some changes and added unit tests.

Travis is down now unfortunately :-O
I just ran all tests on my laptop and they works fine. We may
check if it works well on Travis as soon as it comes back.
Rik would you take a look again?

Comment 20

[Anthony Ricaud (:rik)]

Comment on attachment 800617 [details]
patch

Overall this looks good. I'd still want some changes to the tests and a few stylistic changes.

Comment 21

[Anthony Ricaud (:rik)]

Needinfoing Étienne so he can take a look at the question in the Pull request: https://github.com/mozilla-b2g/gaia/pull/11981#discussion_r6588536

Comment 22

[Etienne Segonzac (:etienne)]

To be fair, Rex isn't introducing the issue here, he just happens to be the first brave developers to add tests to `dialer.js` :)

I think it's fair to open of follow up for this since we have the issue in other places:

We could have 1 js file for each window (`index.js` and `oncall.js`) registering 1 listener to "load" and doing everything in it.
And those files wouldn't be loaded in the tests.

What do you guys think? Open to suggestions :)

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 800617 [details]
patch

I was just looking for feedback in case there was a way an easy way to improve the situation. But it's too big to do in this so let's land it this way.

Thanks Rex!

Ayman: After this lands, if you think the transitions could be improved, please file a depending bug.

Comment 24

[Etienne Segonzac (:etienne)]

(In reply to Anthony Ricaud (:rik) from comment #23)
> so let's land it this way.

And file a follow up bug ;)

Comment 25

[KM Lee [:rexboy] (inactivate after Sep. 22)]

OK, thanks for reviewing and helping on this issue Rik and Etienne!
I'll file another bug and paste it here for unit test of dialer.js.

master:
https://github.com/mozilla-b2g/gaia/commit/7b96c4d7791d334298a28f5e3139154391a1c239

Comment 26

[James Lal [:lightsofapollo]]

Uplifted 7b96c4d7791d334298a28f5e3139154391a1c239 to:
v1.2: 3a30a070c154bbe51b253fb4bb55bc32f5f53f2d