[Contacts] navigation.js should support several concurrent instances of navigationStack

RESOLVED FIXED in 1.3 C2/1.4 S2(17jan)


Firefox OS
4 years ago
4 years ago


(Reporter: gtorodelvalle, Assigned: gtorodelvalle)


1.3 C2/1.4 S2(17jan)
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)



(1 attachment)

384 bytes, text/html
Jose Manuel Cantera
: review+
: feedback?
Currently the Contacts app creates 2 instances of the navigationStack object, one to deal with the navigation at the (so to call it) main Contacts app level and another one to deal with the navigation at the Contacts app Settings level. Due to how the z-index property is being set right now, there are cases when the navigation fails since it may end up setting a wrong z-index to the next view to show.

The navigationStack objects should share a common z-index value so that future navigations increase and decrease this common value properly.

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.
Assignee: nobody → gtorodelvalle
Blocks: 956219
Created attachment 8357128 [details]

At the end I chose the option which included less changes to avoid possible regressions.

I have confirmed that Ashay's patch to bug 956219 still works when pulling my patch on top of his so, Ashay, you may safely remove any changes to navigation.js from your pull request.
Attachment #8357128 - Flags: review?(jmcf)
Attachment #8357128 - Flags: feedback?(ashayb2g)

Comment 2

4 years ago
Comment on attachment 8357128 [details]

thanks Germán
Attachment #8357128 - Flags: review?(jmcf) → review+
IMHO we should not add more time to the navigation object, specially taking into account that will be (hopefully) replaced, or totally delete when Hayda comes to an end.

I know that we still need to work, but navigation.js is not the part that I feel more confortable with, and if we can get rid of it ASAP, the better :)

Comment 4

4 years ago
We are only tweaking it in order to support multiple contact deletion. I agree that we should minify the changes on that component.
Yeah, totally agreed with all you said! :-)

Merged in master: https://github.com/mozilla-b2g/gaia/commit/d3fdcc2d25ccd6a0ac4ca1c5266898f7e33027bd
Last Resolved: 4 years ago
Resolution: --- → FIXED


4 years ago
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)

Comment 6

4 years ago
@gtorodelvalle I tried this code and found some issue when in multiple delete screen if you go to search and then come back and do this process for some time you will find that screens are not in proper sequence, infact at times you'll see some altogether different screen. 

Please check the issue and kindly Reopen it.
Flags: needinfo?(gtorodelvalle)
Hi Ashay, nice catch! ;-) In fact, you just found a bug which is not really related to the code included in this patch or in yours regarding the multiple contact deletion. It was already there but I guess nobody tried it so far :-O

The problem is due to the fact that search.js was not considering the 2 possible navigationStack instances which may be controlling the application in the Contacts app. It was using 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).

I just created bug 958012 to deal with this issue and I'll include it as blocking to this very bug although it is not really a blocking issue but a related one to have everything working.
Depends on: 958012
Flags: needinfo?(gtorodelvalle)
status-b2g-v1.3: --- → affected
No longer depends on: 958012
status-b2g-v1.3: affected → ---
You need to log in before you can comment on or make changes to this bug.