1132262 - [Search] Search screen overlaps with other screen.

Status: RESOLVED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20150211_SearchResultOverlapping.txt

Description:
When the user quickly opens the notification tray after tapping the search bar, the search results are displayed underneath the notification tray. And the search result overlaps with the opened app or homescreen afterwards.

Pre-requisite: Have a Wi-fi or cell data connected.
Repro Steps:
1) Update a Flame to 20150211010216.
2) Tap a rocket bar, and enter any character.
3) Press the home button to dismiss the search result.
4) Open any app, such as Calendar or Contact.
5) Tap the search bar, anc quickly pull down the notification tray.

Actual:
6) Observe that the search results appear undearneath the notification tray.
7) When the search results are fully loaded, press the home button.
8) Observe the search screen overlaps with the app opened on step #4.
9) Press the home button again.
10) Observe the search screen still remains on top of the homescreen.

Expected:
The search result does not appear, and the notification tray appears properly.

Environmental Variables:
Device: Flame 3.0 (319mb, full flash)
Build ID: 20150211010216
Gaia: 8c7865486a1b11076b849bbf8f7fccbaffbfafe7
Gecko: ee093ca70666
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Device: Flame 2.2 (319mb, full flash)
Build ID: 20150211002505
Gaia: 943be6fd146017dcd9d4c9d1027be1e43bad13eb
Gecko: e614443583e7
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Keywords:

Repro frequency: 5/5
See attached: screenshot, logcat

Comment 1

[Yeojin Chung [:YeojinC]]

Attachment: SearchScreen.png

Comment 2

[Yeojin Chung [:YeojinC]]

Flame 2.1 displays a slightly different behavior. The notification tray does not appear after step #5, but the search results appear on top of the opened app, same error as step #8. When the user presses the Home button, the homescreen appears properly, and the search results do not remain on the screen. 

Device: Flame 2.1  (319mb, full flash)
Build ID: 20150211001259
Gaia: af0131a6ba7bf12ac3dc4d94225a7f4e62589ecb
Gecko: b6b59143a192
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 3

[Yeojin Chung [:YeojinC]]

Unable to reproduce on Flame 2.0 as the search function on other apps (minimized rocket bar) was not implemented prior to v.2.1.

Environmental Variables:
Device: Flame 2.0
Build ID: 20150211000203
Gaia: 2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko: d47ec000ee85
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 32.0 (2.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

Is this a regression? If so, can we get a regression-window?

Comment 5

[Brogan Zumwalt [Inactive]]

Issue DOES occur on 2.1

Following STR 1-5 in comment 0 the issue does repro, but timing is difficult and much harder to reproduce on this build. Issue is not a regression.

Easier STR to repro on 2.1 is to replace step 5 with: Tap search bar, then quickly press same area where collapsed rocketbar was as soon as rocketbar begins to expand.

Device: Flame 2.1
Build ID: 20150212002013
Gaia: 88084bc7ef5ba6627dd09c774ef2f7fa96cbed71
Gecko: e395bfad7bc9
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1132262-search-overlap > mozilla-b2g:master

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8570481 [details] [review]
[gaia] albertopq:1132262-search-overlap > mozilla-b2g:master

So the code looks good here, but this isn't quite working for me, so clearing review for now.

Alberto - question, with this patch is the intention that we should not have the search window + utility rendered at the same time? Currently by tapping on the rocketbar then immediately sliding the utility tray down I can still see through the utility tray to the rocketbar.

I thought that we would handle this in utility-tray-overlayopening, but it appears that something is causing it to appear after the fact perhaps?

Comment 8

[Alberto Pastor [:albertopq]]

Despite the PR fixed most of cases, there was still a race condition in which, if we receive the event 'iac-search-results' after the 'utility-tray-overlayopening', the rocketbar was reactivated. 

I added a eventSafety call for waiting to the 'iac-search-results' event (with a safety timer) for deactivating the rocketbar when the 'utility-tray-overlayopening' event is received.

Let me know if that makes more sense.

Thanks!

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8570481 [details] [review]
[gaia] albertopq:1132262-search-overlap > mozilla-b2g:master

Hey Alberto - I left a comment on github, could you take a look and let me know what you think? In this case I think it would be better to avoid the eventSafety library and instead listen to utility-tray-overlay* events. Let me know what you think.

Comment 10

[Alberto Pastor [:albertopq]]

Yeah, that makes sense. I wanted to avoid a local variable, but it's true that the delay in all the other cases is worse. Thanks for the feedback!

Comment 11

[Alberto Pastor [:albertopq]]

Comment on attachment 8570481 [details] [review]
[gaia] albertopq:1132262-search-overlap > mozilla-b2g:master

I ended up using the UtilityTray.active flag. Everything is cleaner and faster now. Thanks!

Comment 12

[Kevin Grandon :kgrandon]

Attachment: Screenshot with patch applied

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8570481 [details] [review]
[gaia] albertopq:1132262-search-overlap > mozilla-b2g:master

Hey Alberto - I attached a screenshot of what I'm seeing with this patch applied - and I think the idea is that we want to avoid that type of situation, no?

I think the problem is that we can still get the utility tray opened event after opening the rocketbar, so I would suggest listening for events still. When we get the utility-tray-overlayopened event we can hide the search window and set a local tracking variable.

What do you think?

Comment 14

[Alberto Pastor [:albertopq]]

I'm still not convinced that listening events and keeping the state is the best idea. Be aware that we have several events depending on dragging the utility but not releasing (utilitytraywillhide, utilitytraywillshow, utility-tray-opening/closing/opened/closed).

I couldn't repro at any time this bug again after applying the patch, but then I realized I was never releasing the utility tray (probably you were dragging and relasing quickly when you saw the bug with the patch).

I checked what UtilityTray.active really means, and that's only true when you still keep the handle pressed. UtitlityTray.shown then keeps when the tray has been totally shown. So, I made this change, and couldn't repro again [1]. What do you think?

[1] https://github.com/mozilla-b2g/gaia/pull/28518/files#diff-5699b1000bc807368718bfddf7ef58c3R516

Comment 15

[Kevin Grandon :kgrandon]

I'm still seeing the same behavior as I'm seeing in this image: https://bug1132262.bugzilla.mozilla.org/attachment.cgi?id=8572177

I believe the idea is that we no longer want to support having the utility tray & search visible at once, correct?

My thought was that going with events would help this as it would defeat asynchronous races - you just close the search app when you get the proper event.

Comment 16

[Alberto Pastor [:albertopq]]

The problem is that, even if you close the search at that point, you can still receive events that would make activate the search again (focus, iac-search-results), and we need to avoid that. Can we talk about it offline? Feel free to steal it if you want! You know the code better then I do :) Thanks!

Comment 17

[Kevin Grandon :kgrandon]

Ok, I am fairly sure that the reason this is not working is due to not checking UtilityTray status in global-search-request. You might want to check the utility tray status in that handler, or inside of the activate() method.

Comment 18

[Alberto Pastor [:albertopq]]

I'm sorry, but I'm still not able to repro with the patch (even when accessing from an app). In theory, even if it comes from an app, it will end up in the handleInput method, so should be fine. It must be a device thing. Passing the bug to you. Thanks!

Comment 19

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:albertopq-1132262-search-overlap > mozilla-b2g:master

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8574065 [details] [review]
[gaia] KevinGrandon:albertopq-1132262-search-overlap > mozilla-b2g:master

Alberto - I've added a commit in this pull request which I think solves the remaining issues. This guards us from actually opening the utility tray until the search app is shown, fixing some races. Please take a look and let me know what you think. Thanks!

Comment 21

[Alberto Pastor [:albertopq]]

I'm not convinced that disabling the utility tray until the search is shown is the best approach... That would mean that only the search requests created from that part of the code would work. Clicking the url bar in the browser, for example, is creating a global-search-request as well but the utility tray won't be disabled.

I think we should fix the problem in rocketbar.js, in order to avoid depending on where the search request has been originated. What do you think about checking the Utility tray status on [1] instead?

[1] https://github.com/KevinGrandon/gaia/blob/albertopq-1132262-search-overlap/apps/system/js/rocketbar.js#L302

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8574065 [details] [review]
[gaia] KevinGrandon:albertopq-1132262-search-overlap > mozilla-b2g:master

Hmm, I think you're right. I think we need to abort the activate() call here. I believe that's the intention, but we must've regressed it at some point. Taking another look and will re-request review when I have an updated patch.

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8574065 [details] [review]
[gaia] KevinGrandon:albertopq-1132262-search-overlap > mozilla-b2g:master

Alberto - could you take a look again? I've updated this to basically store an activation promise, and wait on that to close the rocketbar if needed. We close the rocketbar either when we receive the utility tray opened event, or when trying to activate and the rocketbar is visible. I found that we could encounter both cases. Thanks!

Comment 24

[Alberto Pastor [:albertopq]]

Comment on attachment 8574065 [details] [review]
[gaia] KevinGrandon:albertopq-1132262-search-overlap > mozilla-b2g:master

LGTM! Thanks!

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8574065 [details] [review]
[gaia] KevinGrandon:albertopq-1132262-search-overlap > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Occasionally poor UX when using search.
[Testing completed]: Manual and unit testing.
[Risk to taking this patch] (and alternatives if risky): Fairly low risk as it's only within the rocketbar code and was not a large change.
[String changes made]: No.

Comment 27

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QA NOTE: Please verify bug 1141372 when this gets fixed in the verification process

Comment 28

[Kevin Grandon :kgrandon]

We ran into a problem where autolander was missing messages from taskcluster, I think it missed this bug. This landed here: https://github.com/mozilla-b2g/gaia/commit/6ba618c6c45c021b74aa3956245a45068d663d4e

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/f19fa4d99aa4d1fcc89b7f1079a2111f3e3a9b03
v2.2: https://github.com/mozilla-b2g/gaia/commit/3b3a65aefdd47e475422afd31b470cbae345ac85

Comment 30

[Shine(Leave from Mozilla)]

Attachment: video

This problem is verified pass on latest build of Flame 3.0. In step 5, after quickly pulling down the notification tray, the rocket bar and search result will be hidden automatically.
See attachment: Flame3.0_video.MP4
Rate:0/5

Flame 3.0 build (Pass):
Build ID               20150312160232
Gaia Revision          eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gaia Date              2015-03-12 18:01:49
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/42afc7ef5ccb
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150312.194521
Firmware Date          Thu Mar 12 19:45:32 EDT 2015
Bootloader             L1TC000118D0

Leaving verifyme for 2.2 uplift/verification.

Comment 31

[Sue]

This issue has been verified succeffully on Flame 2.2, after quickly pulling down the notification tray, the rocket bar and search result will be hidden automatically.
Rate:0/5

Flame 2.2 build: pass
Build ID               20150315162500
Gaia Revision          a6b2d3f8478ec250beb49950fecbb8a16465ff6f
Gaia Date              2015-03-15 14:33:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/18619f8f6c5c
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.195030
Firmware Date          Sun Mar 15 19:50:42 EDT 2015
Bootloader             L1TC000118D0