Closed Bug 1028661 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: camel.aissani, Assigned: camel.aissani)

References

Details

(Keywords: regression, Whiteboard: [systemsfe])

Attachments

(2 files)

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
Attached patch 1028661.patchSplinter Review
Fixed issue by not calling the method handleInput from handleCancel
Attachment #8444059 - Flags: review?(kgrandon)
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1027850
Let's reopen this one as it as a patch.
Blocks: 1015336
Status: RESOLVED → REOPENED
blocking-b2g: --- → 2.0+
QA Whiteboard: [QAnalyst-Triage+][lead-review+][VH-FL-blocking-][VH-FC-blocking+]
Component: Gaia::System → Gaia::System::Window Mgmt
Ever confirmed: true
Keywords: regression
Resolution: DUPLICATE → ---
Whiteboard: [systemsfe]
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.
Attachment #8444059 - Flags: review?(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!
Flags: needinfo?(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.
Flags: needinfo?(kgrandon)
Flags: needinfo?(camel.aissani)
Flags: needinfo?(21)
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.
Flags: needinfo?(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!
Attachment #8444059 - Flags: review?(dale)
Duplicate of this bug: 1027850
Can't reproduce the issue with the home button and this patch on a fresh device. Kevin, are you sure you applied the patch ? :)
Flags: needinfo?(21)
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
Flags: needinfo?(kgrandon)
Actually, It is what I saw in the debugger ;-)
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.
Flags: needinfo?(kgrandon)
(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.
(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 ?
Flags: needinfo?(kgrandon)
Duplicate of this bug: 1029398
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!
Attachment #8444059 - Flags: review?(dale) → review+
Flags: needinfo?(kgrandon)
Target Milestone: --- → 2.0 S5 (4july)
Duplicate of this bug: 1029734
Summary: [Rocketbar] Overlay of homescreen and searh result view after a cancel search → [Rocketbar] Overlay of homescreen and search result view after a cancel search
(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.
(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.
(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 :(
Duplicate of this bug: 1030806
(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.
https://github.com/mozilla-b2g/gaia/commit/1d2a0148e977d0560977887df9b95b2ec12f37cf

Thanks Camel for the patch!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attached video 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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.