1028661 - [Rocketbar] Overlay of homescreen and search result view after a cancel search

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[camel.aissani]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140608211622

Steps to reproduce:

1. Tap on search bar from home screen
2. Tap on Close in search bar



Actual results:

You see an overlay of homescreen and search result view


Expected results:

We should see only the homescreen

Comment 1

[camel.aissani]

Attachment: 1028661.patch

Fixed issue by not calling the method handleInput from handleCancel

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Let's reopen this one as it as a patch.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8444059 [details] [diff] [review]
1028661.patch

Review of attachment 8444059 [details] [diff] [review]:
-----------------------------------------------------------------

While you're doing this, can you also handle the 'home' event? Another very easy way to reproduce this is to tap the home button after entering a character.

I would also like to see a unit test for this, possibly one that checks for a class after firing the 'home' event, but if you're not familiar with the unit testing framework I can gladly follow it up.

Comment 5

[Kevin Grandon :kgrandon]

Camel - let me know if you need help with the unit test, but let's land the fix fix for pressing 'home', or maybe move it into the deactivate function. Thanks!

Comment 6

[camel.aissani]

Kevin we (Vivien and I) made the test that you describe on the phone (I mean type a character and tap on the home button). It works well.

I've just took a look at the code. In the handleHome function, we have the same call of "this.hideResults();".
I think we should keep the fix as it is to avoid any regressions. 
Also, I suggest to not move the code into deactivate function. I don't know if another workflow can be negatively impacted by the fix.

Yes please I need assistance to make a new unit test.

Let me know if you want that I modify the patch.

Comment 7

[Kevin Grandon :kgrandon]

Right, that makes sense. For some reason I was still able to reproduce with pressing the home button, but I will take anohter look at your patch soon.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8444059 [details] [diff] [review]
1028661.patch

Review of attachment 8444059 [details] [diff] [review]:
-----------------------------------------------------------------

Dale - if you get a chance could you review this one? I was still seeing it with the 'home' button press, but not sure if that should be solved in another bug. Thanks!

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Can't reproduce the issue with the home button and this patch on a fresh device. Kevin, are you sure you applied the patch ? :)

Comment 11

[camel.aissani]

Kevin, I think that the method that opens the result view after a cancel is handleInput (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/rocketbar.js#L734)

HandleInput is called at L#751

Comment 12

[camel.aissani]

Actually, It is what I saw in the debugger ;-)

Comment 13

[Kevin Grandon :kgrandon]

I was definitely seeing it when pressing the 'home' button, but then again I was testing on a device with software home button, and not the flame, so that could be one of the problems. Let's see what Dale says on this one.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #13)
> I was definitely seeing it when pressing the 'home' button, but then again I
> was testing on a device with software home button, and not the flame, so
> that could be one of the problems. Let's see what Dale says on this one.

Software home button. Yeah it could do some fun focus/blur thing I believe.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #14)
> (In reply to Kevin Grandon :kgrandon from comment #13)
> > I was definitely seeing it when pressing the 'home' button, but then again I
> > was testing on a device with software home button, and not the flame, so
> > that could be one of the problems. Let's see what Dale says on this one.
> 
> Software home button. Yeah it could do some fun focus/blur thing I believe.

OK. I investigate a bit and the issue with the software home button is an orthogonal issue. I opened bug 1029568 with a patch for the issue of the software home buton.

Basically the 'home' event was not triggered because the touch was not hitting the soft button but the rocketbar result pane. Fixing the z-index solve the issue with the software button.


So Kevin, can you reproduce still reproduce the issue with this patch, and the patch for the soft button in bug 1029568 ?

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8444059 [details] [diff] [review]
1028661.patch

Review of attachment 8444059 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, sounds good to me in that case then. Thanks!

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Will merge it once it is green.

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=642b695dc8b71f203e0130e0e7a2e9d0bad60dbc

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #19)
> Will merge it once it is green.
> 
> https://tbpl.mozilla.org/?tree=Gaia-
> Try&rev=642b695dc8b71f203e0130e0e7a2e9d0bad60dbc

Crap I broke a test on Gaia-Try:

gaia-unit-tests TEST-UNEXPECTED-FAIL | verticalhome/test/unit/contextmenu_ui_test.js | contextmenu_ui.js > Hide the context menu when the document is hidden | expected false to be true

Will dig into it.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #20)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #19)
> > Will merge it once it is green.
> > 
> > https://tbpl.mozilla.org/?tree=Gaia-
> > Try&rev=642b695dc8b71f203e0130e0e7a2e9d0bad60dbc
> 
> Crap I broke a test on Gaia-Try:
> 
> gaia-unit-tests TEST-UNEXPECTED-FAIL |
> verticalhome/test/unit/contextmenu_ui_test.js | contextmenu_ui.js > Hide the
> context menu when the document is hidden | expected false to be true
> 
> Will dig into it.

Could be an intermittent. Can't reproduce locally. I retriggered the job on Gaia-Try to double check.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #21)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #20)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> > needinfo? please) from comment #19)
> > > Will merge it once it is green.
> > > 
> > > https://tbpl.mozilla.org/?tree=Gaia-
> > > Try&rev=642b695dc8b71f203e0130e0e7a2e9d0bad60dbc
> > 
> > Crap I broke a test on Gaia-Try:
> > 
> > gaia-unit-tests TEST-UNEXPECTED-FAIL |
> > verticalhome/test/unit/contextmenu_ui_test.js | contextmenu_ui.js > Hide the
> > context menu when the document is hidden | expected false to be true
> > 
> > Will dig into it.
> 
> Could be an intermittent. Can't reproduce locally. I retriggered the job on
> Gaia-Try to double check.

Not intermittent :(

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #22)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #21)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> > needinfo? please) from comment #20)
> > > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> > > needinfo? please) from comment #19)
> > > > Will merge it once it is green.
> > > > 
> > > > https://tbpl.mozilla.org/?tree=Gaia-
> > > > Try&rev=642b695dc8b71f203e0130e0e7a2e9d0bad60dbc
> > > 
> > > Crap I broke a test on Gaia-Try:
> > > 
> > > gaia-unit-tests TEST-UNEXPECTED-FAIL |
> > > verticalhome/test/unit/contextmenu_ui_test.js | contextmenu_ui.js > Hide the
> > > context menu when the document is hidden | expected false to be true
> > > 
> > > Will dig into it.
> > 
> > Could be an intermittent. Can't reproduce locally. I retriggered the job on
> > Gaia-Try to double check.
> 
> Not intermittent :(

OK. That's finally my bad (again). When rebasing the PR I added some unwanted changes and create a new test in contextmenu UI that are unrelated to this patch.

Should be green once TBPL is back.

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/1d2a0148e977d0560977887df9b95b2ec12f37cf

Thanks Camel for the patch!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/bb487bc79705bd601fbcfdc545f8482569b50c63

Comment 27

[Elie]

Attachment: VIDEO0100.mp4

This issue has been successfully verified on Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141201000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2


This issue has been successfully verified on Flame 2.1:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2