910185 - [CONTACTS] In export contacts, the 'Search box' can't be selected.

Status: VERIFIED FIXED

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

Description

[b.paloma]

Repro:

1. Go to Contacts -> Settings -> Export contacts
2. Tap on 'to SIM'
3. The list of contacts is shown, there is also a Search box
4. Type some chars in the Search box (ER1)


Expected:

ER1. It is possible to type characteres in the Search box and the list of contacts shown is filered whiley typing.

Actual: 

ER1. It isn't possible to type characteres in the Search box

Notes:
This bug has been checked on:
branch: master
Gaia:43cf923
Gecko:7dc9265

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

This should be a regression, since we had that working before :(

Comment 2

[rafael.marquez]

We have the same problem in the "export to vCard". We maintain this bug for two screens, but if necessary we can open a new bug.

Comment 3

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

Seems like a z-index issue.

The z-index of search view is constantly 1 whereas z-index of export-view set in navigationStack is
higher than that.

Wondering if I should use navigationStack to manage search view..

Comment 4

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

The WIP.
https://github.com/rexboy7/gaia/commit/27e3de656f45ad7b933fa1588d2088b7bd56bfdd

In detail, what this WIP do is mainly:
* Let navigationStack manage search view.
* When navigating panels, only z-index=0 and z-index=1 are used rather than setting z-index to stack length.
  By doing this, we can prevent confused values when duplicated panels are moved to top of the stack.

Not sure if we have better solutions.. I'll try cleanup the code before sending review.

Comment 5

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

Attachment: patch

Francisco, would you mind reviewing this patch?
It works for me but I still can't make the solution very clean..

Details below:
This patch tried to fix z-index using problems with navigating stack.
Several points are considered IMO:
1. The search-view is added into navigating stack since we need to change
   its z-index to a higher value when using search-view in import panel.
2. z-index of each panel is logged in stack for calculating z-index of
   next pushed panel. Since re-used panel may be removed from middle of
   the stack, I think stack.length is not precise on that and sometimes
   causes problem.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Hi @rexboy

I think the best solution is what you comment on 2.

Right now it's creating several problems since we added another dimension for navigation. Your patch almost works ok but found a case which is failing.

Settings > Export > Select an export option (it will display the list in select mode) > close the list in select mode > got to import

The import window appear in white, with a z-index that makes it doesn't appear. I guess that could be caused cause in the import option we create a new navigation object.

Great job! The navigation thing in contacts going back and forward several times is a pain in the ass, but you almost have it fixed!!

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Hi @rexboy

sorry, just requesting the needinfo to be sure you saw my comment about your PR.

Thanks!

Comment 8

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

Yes I did, sorry for the late reply. Thanks for your review Francisco!
I'm just working on another bug and I'll try this bug later.. Sorry for that.

The problem you mentioned also occurs on current master I think. I guess someone
just inserted a listener of going previous panel too much time without removing it.
I haven't traced it in detail but I guess it's another issue that causes navigation
works incorrectly. I'll trace it later.

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to KM Lee [:rexboy] from comment #8)
> Yes I did, sorry for the late reply. Thanks for your review Francisco!
> I'm just working on another bug and I'll try this bug later.. Sorry for that.
> 
> The problem you mentioned also occurs on current master I think. I guess
> someone
> just inserted a listener of going previous panel too much time without
> removing it.
> I haven't traced it in detail but I guess it's another issue that causes
> navigation
> works incorrectly. I'll trace it later.

Thanks!!

Comment 10

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

Sorry for the late reply. It was mid-autumn festival in Chinese last week so I was away for several days..

I was debugging and trying to solve the problem mentioned in comment #6, but after rebasing I found the problem mentioned in comment #6 has already been fixed in bug 910188. The problem seems to be unrelated to the z-index problem. The root cause is that event listener in list item is not removed correctly such that |navigation.back()| being called too many times. Seems German already fixed it perfectly so I don't have to add things on this patch. We can just fix the z-index problem here then.

So all I did is add some unit tests on the second commit.. ni? Francisco. May you take a look again?

Comment 11

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

Comment on attachment 800113 [details]
patch

Updated.

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Thanks :rexboy!

Will give it a try!

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Thanks :rexboy!

Will give it a try!

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Hi again,

I tried to test the patch but it doesn't apply to current master, could you rebase?

Comment 15

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

OK I've rebased it. (See comment on Github)
I'll squash these commits once it's completed. Set ni? as a reminder again.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 800113 [details]
patch

Hi,

r+plusing, just 2 comments:

- I had to rebase cause we had a commit from Kevin that entered 11 hours ago, but it's quite simple, so it didn't cost me much.
- We have an error in a unit tests:

Error: TypeError: Contacts.navigation.go is not a function (http://communications.gaiamobile.org:8080/contacts/js/views/search.js?time=1380109885039:230)
      at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959)

Could you solve the error in the tests, squash and merge this?

Thanks a lot for your help!

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

Hi :rexboy,

just left some info in comment 16

Thanks a lot!

Comment 18

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

OK, Thanks for reviewing! I've rebased and fixed the unit test error.
(I thought that error was unrelated, sorry for that.  Would you set plus on review?)

Travis down again unfortunately... I'll merge it as soon as it passes travis test.

Comment 19

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 800113 [details]
patch

Perfect then!

Thanks a lot for the work!

Comment 20

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

master:
https://github.com/mozilla-b2g/gaia/commit/342b260c7a716735d2dcafa6130890bca0402e67

Comment 21

[isabelrios]

Verified on 9/29 v1.2 build:
Gecko-e73e078
Gaia-1992915
It is possible to tap on the Search box and write characters

Comment 22

[James Lal [:lightsofapollo]]

Uplifted 342b260c7a716735d2dcafa6130890bca0402e67 to:
v1.2: 0eb0d8cc62eec8349393eb7bd57e81910e3ec3f7