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

RESOLVED FIXED in Firefox OS v1.2

Status

P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sync-1, Assigned: rexboy)

Tracking

unspecified
1.2 C2(Oct11)

Firefox Tracking Flags

(blocking-b2g:koi+, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 fixed)

Details

(Whiteboard: [u=commsapps-user c=contacts p=0])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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:
(Reporter)

Comment 1

5 years ago
Created attachment 787967 [details]
log
Assignee: nobody → rexboy
blocking-b2g: --- → koi?
ni? Ayman for the correct flow.
blocking-b2g: koi? → koi+
Flags: needinfo?(aymanmaat)
Whiteboard: [u=commsapps-user c=contacts p=0]

Comment 3

5 years ago
(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
Flags: needinfo?(aymanmaat)
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!
Flags: needinfo?(aymanmaat)

Comment 5

5 years ago
(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
Flags: needinfo?(rexboy)
Hi Ayman,
WIP here: 
https://github.com/rexboy7/gaia/commit/0f5f541d525981c7495bf808e6cc7fd15e191243

You may ping me if you need any help on applying patch. Thanks!
Flags: needinfo?(rexboy)

Comment 7

5 years ago
(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.
Flags: needinfo?(aymanmaat)

Comment 8

5 years ago
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.
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.
Flags: needinfo?(aymanmaat)

Comment 10

5 years ago
(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

5 years ago
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
Flags: needinfo?(aymanmaat) → needinfo?(rexboy)
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
Flags: needinfo?(rexboy)

Comment 13

5 years ago
(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
Created attachment 800617 [details]
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?
Attachment #800617 - Flags: review?(jmcf)
Attachment #800617 - Flags: review?(anthony)

Comment 15

5 years ago
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 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.
Attachment #800617 - Flags: review?(anthony) → review-
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?
Attachment #800617 - Flags: review- → review?(anthony)
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.
Attachment #800617 - Flags: review?(anthony) → review-

Updated

5 years ago
Attachment #800617 - Flags: review?(jmcf)
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?
Attachment #800617 - Flags: review- → review?(anthony)
Comment on attachment 800617 [details]
patch

Overall this looks good. I'd still want some changes to the tests and a few stylistic changes.
Attachment #800617 - Flags: review?(anthony) → review-
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
Flags: needinfo?(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 :)
Flags: needinfo?(etienne)
Target Milestone: --- → 1.2 QE1(Oct11)
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.
Attachment #800617 - Flags: review- → review+
(In reply to Anthony Ricaud (:rik) from comment #23)
> so let's land it this way.

And file a follow up bug ;)
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 922006
status-b2g18: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
status-b2g-v1.2: --- → affected
Uplifted 7b96c4d7791d334298a28f5e3139154391a1c239 to:
v1.2: 3a30a070c154bbe51b253fb4bb55bc32f5f53f2d
status-b2g-v1.2: affected → fixed
You need to log in before you can comment on or make changes to this bug.