Closed Bug 958012 Opened 8 years ago Closed 8 years ago

[Contacts] search.js not dealing with the correct navigationStack instance

Categories

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

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)

People

(Reporter: gtorodelvalle, Assigned: gtorodelvalle)

References

Details

Attachments

(2 files)

Right now there are 2 possible instances of navigationStack to deal with navigation at the Contacts app level: 1) one for the (so to call it) "main Contacts app" navigation and 2) one for the "Contacts app Settings" navigation.

Although list.js is correctly dealing with each of both possible instances, this is not the case of search.js with uses the "main Contacts app" navigationStack instance by default (see https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/search.js#L123 and https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/search.js#L245)

The search.js should properly use the correct instance of navigationStack.
Assignee: nobody → gtorodelvalle
In fact, you can cause this misbehaviour without including Ashay's patch for bug 956219 by just opening the Contacts app, clicking on the Settings button, clicking on the "Export Contacts" button, clicking on any of the "Export Contacts" options, clicking on the "Search" area, selecting some contact and cancelling. From that time on, you won't be able to re-enter the Search mode :-)
Attached file 15155.html
Attachment #8357721 - Flags: review?(jmcf)
Can you provide a unit test for this change. I'm pretty afraid of the changes on the navigation.js and I've seen several regresions with that, so better to have then under control.

Thanks a lot German!
Attachment #8357721 - Flags: feedback?(ashayb2g)
Ashay, I tried your patch to bug 956219 plus the patch to bug 957577 plus this patch (yeah, this is getting complex :-O) and the issues regarding entering the Search mode from the list of contacts in the main Contacts app, from the list of contacts to export or for your brand new "Bulk Delete" (multiple contacts deletion) functionality seem to have been solved :-) Anyhow, more testing from your side is more than appreciated ;-) Thanks!
Comment on attachment 8357721 [details]
15155.html

Francisco, I have modified a previous test case we had (including some cleaning of unneeded code) and included a new one to check that the search.js library deals correctly with the right navigationStack instance.

As you will see, I also updated the mock_navigation.js library not only to make it more similar to the "real" navigation.js library but also to be able to create several instances of the mockNavigationStack object for the testing. The new mockNavigationStack object is also instantiated by the mockContact object which made me include it as a dependency in already existent tests.

If you need further details, please do not hesitate to ask ;-) Thanks!
Attachment #8357721 - Flags: review?(francisco.jordano)
@gtorodelvelle This problem is actually because of zIndex, I would like to give you a one liner patch for this issue. Please refer the attached patch and provide your feedback.

I have a doubt about navigationStack._zIndex = nextView.zIndex, as we are having two navigation stack so on back() functionality assigning navigationStack._zIndex to the next view zIndex is having some issues in some particular scenarios, when we have one screen from main stack and another from different stack and again one screen from main stack, on back() It may skip some depends the size of stack, and again if you try to push a new one, it may have lesser or same zIndex of current view. And doing forward and back again on to different navigation stack results in improper zIndex.

Regards
Ashay
Attached patch Bug_958012.patchSplinter Review
Attachment #8358205 - Flags: review?(francisco.jordano)
Attachment #8358205 - Flags: feedback?(gtorodelvalle)
Yeah, that's why I mentioned in bug 957577 that "It is important also to note that successive navigationStack instances sit on top of previous ones and consequently all its views have to be "closed" before interacting (go, back, home) with the lower one."

In fact, the case you mention (even including your proposal) can even cause more weird behaviours. Imagine this:
1) Create the first instance of navigationStack with view 'view-1' -> 'view-1' with z-index = 1 in navigationStack #1' stack.
2) Using navigationStack #1 navigate to 'view-2' using go -> 'view-2' with z-index = 2 in navigationStack #1' stack.
3) Using navigationStack #1 navigate to 'view-3' using go -> 'view-3' with z-index = 3 in navigationStack #1' stack.
4) Instantiate the second instance of navigationStack with 'view-3' -> 'view-3' with z-index = 4 in navigationStack #2' stack.
5) Using navigationStack #2 navigate to 'view-4' using go -> 'view-4' with z-index = 5 in navigationStack #2' stack.
6) Using navigateStack #2 navigate to 'view-5' using go -> 'view-5' with z-index = 6 in navigationStack #2' stack.
7) Using navigationStack #1 navigate to 'view-3' using go -> 'view-3' with z-index = 7 in navigationStack #1' stack.
8) Using navigationStack #1 go back using back -> 'view-2' with z-index = 2 (according to https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/navigation.js#L197) in navigationStack #1' stack whereas navigationStack would be 6. There is a good chance that 'view-2' is not shown according to its z-index :-)
9) Using navigationStack #1 go back using back -> 'view-1' with z-index = 1 (according to https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/navigation.js#L197) in navigationStack #1' stack whereas navigationStack would be 5. There is a good chance that 'view-1' is not shown according to its z-index :-)
10) Using navigationStack #2 navigate to 'view-3' using go -> 'view-3' with z-index = 6 in navigationStack #2' stack.

Apart from the problems mentioned in 8 and 9, as you can see you end up with both 'view-3' and 'view-5' with z-index = 6 in navigationStack #2' stack and 'view-3' will only appear on screen if it has been included in the DOM after view-5 as siblings (I guess you get my point).

This is the reason why I included the limitation and the comment in the bug and I opted for my option to stand it out somehow ;)

The conclusion is clear: navigationStack was not conceived for this (in fact, it was not even conceived to be able to manage distinct instances as you may have already seen) :-p

In conclusion, if we really want to support the option you suggest (being able to navigate using one stack, create another stack and navigate using that one, get back to navigating the first stack without "closing" (going back) all the views of the second stack and so on) we should really rethink navigation.js and as Francisco said it may have it days counted :-) The point is that for the time being it is not possible to do what you mention when using the Contacts app, which does not mean that it won't be needed in the future, of course ;-)

So, my vote would be to leave navigation.js as it is right now after merging bug 957577 's patch

No need to say that I would be fine with any decision we may agree on.

Thank you very much for your comment!
Then, Germán we don't even need to merge any of the proposed patches here, right?
Flags: needinfo?(gtorodelvalle)
Considering the current implementation.

-> Main Stack consists of only Contact-List-View page.    ZIndex=1, navigationStack._zIndex = 1.
-> Main Stack pushing Setting View on top of Contact-List-View Page. ZIndex=2, navigationStack._zIndex = 2.
       ->A new Stack got Created for Setting View. zIndex = 3, navigationStack._zIndex = 3.
       ->A new view got pushed to the setting view.( Say Contact Export/Multiple Contact Delete) zIndex=4, navigationStack._zIndex = 4,
-> Main Stack pushing Search Screen on top of Existing View. zIndex=5, navigationStack._zIndex = 5,

As per your proposal on going back on Search Screen, zIndex will be 2 and navigationStack._zIndex = 2. In Main Stack its assigned to Setting View. 
But why setting view is not shown is because setting view is having its own stack so it'll show its top most zIndex view. i.e. either ContactExport/Multiple Contact Delete which is having higher zIndex =4.
so Ideally "navigationStack._zIndex" should 4 not 2.

Again if you'll try to push any view on top of current view zIndex will be 3, so again chances of any new screen comming up will go off as the top view ZIndex=4.

Your Implementation is fine incase if the stacks are individually handled. But here the Setting stack is sandwiched in Main Stack causing all the trouble.

As per my understanding "navigationStack._zIndex" is used to have the top View ZIndex. So that pushing any new view on top of current will not have any problem interms of ZIndex.

Please correct me if I am wrong any where.

Regards
Ashay
What are those 2 patches for?

Why 2 patches in the same bug, what's the difference and the one that solves this bug? Are they 2 different proposals for the same problem?
Hey! No, no, the main patch is attachment 8357721 [details] :-) The other one (attachment 8358205 [details] [diff] [review]) is a proposal by Ashay to include a change but as I mention in comment 8 and I will in this comment it is not needed as far as my understanding ;-)

Ashay, I think that the case you mention in comment 10 is not possible and it is the reason of the patch I included for this bug. Once this patch (attachment 8357721 [details]) lands, the search.js library will manage the correct instance of the navigationStack object. It will use the navigationStack instance of the main app or the one of the Settings when appropriate.

So the case you mention will behave the following way:
-> Main Stack consists of only Contact-List-View page. ZIndex=1, navigationStack._zIndex = 1.
-> Main Stack pushing Setting View on top of Contact-List-View Page. ZIndex=2, navigationStack._zIndex = 2.
       ->A new Stack got Created for Setting View. zIndex = 3, navigationStack._zIndex = 3.
       ->A new view got pushed to the setting view.( Say Contact Export/Multiple Contact Delete) zIndex=4, navigationStack._zIndex = 4,
(THIS WILL NOT POSSIBLE ONCE MY PATCH LANDS) -> Main Stack pushing Search Screen on top of Existing View. zIndex=5, navigationStack._zIndex = 5,
       -> The user clicks on Search and enters Search mode. Index=5, navigationStack._zIndex = 5.
       -> The user exits Search mode. The Contact Export/Multiple Contact Delete is shown. Index=4, navigationStack._zIndex = 5.
Etc., etc., etc.

If we consider the option to mix views from distinct navigationStacks, as I mention in comment 8, we should rethink navigation.js since many other issues may arise and your proposal to decrease the shared _zIndex sadly would not solve them :-(
Flags: needinfo?(gtorodelvalle)
Sorry, one minor typo in my last comment:
       -> The user exits Search mode. The Contact Export/Multiple Contact Delete is shown. Index=4, navigationStack._zIndex = 4. (It would be 4 and not 5, I miss-copy-pasted it :p )
Comment on attachment 8357721 [details]
15155.html

Looking good to me, thanks a lot for adding the tests, tried it on the phone and is working great.

Just left 2 comments on the tests.

Thanks a lot German!
Attachment #8357721 - Flags: review?(francisco.jordano) → review+
I talked to José Manuel via Skype and he is also fine with merging :-)

So, I would say you should not find any more issues when fixing or implementing bug 956219 regarding navigation :) Yeah, yeah, I know that it a lot to say :-p

Thank you very much everyone!
Merged in master: https://github.com/mozilla-b2g/gaia/commit/78ad712e64221073337c73de388196f38b6002aa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #8358205 - Flags: feedback?(gtorodelvalle)
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
No longer blocks: 957577
Attachment #8357721 - Flags: review?(jmcf)
Comment on attachment 8358205 [details] [diff] [review]
Bug_958012.patch

Review not needed, thanks for the help.
Attachment #8358205 - Flags: review?(francisco.jordano)
You need to log in before you can comment on or make changes to this bug.