Brogan Zumwalt [Inactive] Reporter
	Description • 10 years ago

Description:
Tapping Rocketbar on homescreen and typing a string in search bar, then long pressing one of the results brings up the context page. Selecting "add to homescreen" opens the "Add to Homescreen" page in the background. User cannot see this page unless they switch to it by pressing the home button, then pressing Rocketbar again.

Once user switches to page, they can add the bookmark to the homescreen as expected. This process must be repeated for each subsequent bookmark added to homescreen from Rocketbar.
   
Repro Steps:
1) Update a Flame device to BuildID: 20140911000225
2) Tap Rocketbar
3) Type any string
4) Long press a result then select "Add to Homescreen"
  
Actual:
User has to press home button then re-enter rocketbar in order to access "Add to Homescreen" page.
  
Expected: 
"Add to Homescreen" page appears in foreground without further user input.
  
Environmental Variables:
Device: Flame 2.1 (319 mb)
BuildID: 20140911000225
Gaia: d61264cd0c1f797b6be11e33524d8d52983c87e4
Gecko: 1d44dfce2e5b
Version: 34.0a2 (2.1)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
  
Repro frequency: 3/3, 100%
Link to failed test case:
See attached: Youtube video clip & logcat
Youtube link: http://youtu.be/CmUGzzUcnzk

	Brogan Zumwalt [Inactive] Reporter
	Comment 1 • 10 years ago

Issue DOES occur on Flame 2.2 (319mb), OpenC 2.2, Flame 2.1 (512mb), and OpenC 2.1

Environmental Variables:
Device: Flame Master (319mb)
BuildID: 20140910040203
Gaia: 8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7
Gecko: 152ef25e89ae
Version: 35.0a1 (Master) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Open_C Master
BuildID: 20140910040203
Gaia: 8e02f689b0fc39cb6ccdc22d02ed7e219c58faa7
Gecko: 152ef25e89ae
Version: 35.0a1 (Master) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.1 (512mb) 
BuildID: 20140910000202
Gaia: 79dc972d637ff5ef7667b231e93118b4ed83ba9c
Gecko: 0890010015a2
Version: 34.0a2 (2.1)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Open_C 2.1
BuildID: 20140910000202
Gaia: 79dc972d637ff5ef7667b231e93118b4ed83ba9c
Gecko: 0890010015a2
Version: 34.0a2 (2.1) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Actual Results: User has to press home button then re-enter rocketbar in order to access "Add to Homescreen" page.


Issue does NOT occur on Flame 2.0 (319mb) and OpenC 2.0

Environmental Variables:
Device: Flame 2.0 (319mb)
BuildID: 20140910000203
Gaia: 3f4c635106c5364228782d12b1cb76b0c105b971
Gecko: 02a5b9234c13
Version: 32.0 (2.0) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Enviromental Variables:
Device: Open_C 2.0
BuildID: 20140910000203
Gaia: 3f4c635106c5364228782d12b1cb76b0c105b971
Gecko: 02a5b9234c13
Version: 32.0 (2.0)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Actual Results: "Add to Homescreen" page appears in foreground without further user input.

	Brogan Zumwalt [Inactive] Reporter
	Comment 2 • 10 years ago

This fails test case: https://moztrap.mozilla.org/manage/case/14599/

	Marcia Knous [:marcia]
	Comment 3 • 10 years ago

[Blocking Requested - why for this release]: Clear regression from 2.0 - pressing the "add to home screen" should take you directly to the page which allows you to add it without any other actions.

	Kevin Grandon :kgrandon Assignee
	Comment 4 • 10 years ago

There are a few problems here. On master (0f45989dfa25d15ad3d6e0bef41847e46d0da774), the contextmenu does not even show. Then on commit adcf405, we get the contextmenu, but do not get the dialog appearing. For this bisection, we'll just check on the contextmenu part of things.

[~/workspace/gaia ((no branch, bisect started on adcf405))]$ git bisect good adcf405
[~/workspace/gaia ((no branch, bisect started on adcf405))]$ git bisect bad 0f45989dfa25d15ad3d6e0bef41847e46d0da774
Bisecting: 172 revisions left to test after this (roughly 8 steps)
[77a475a68d3a17ca74cae64bc63e3263b353156a] Merge pull request #23348 from yzen/bug-1030346

git bisect good
Bisecting: 86 revisions left to test after this (roughly 7 steps)
[28266041a9e78abd9bc4ec6a1cc85e9d9097c573] Merge pull request #23851 from RudyLu/keyboard/Bug1062105

git bisect bad
Bisecting: 42 revisions left to test after this (roughly 6 steps)
[14709c76da02d7353f52cce8dafee9d08a6afc18] Merge pull request #23742 from DouglasSherk/1059087-contacts-separator

git bisect bad
Bisecting: 20 revisions left to test after this (roughly 5 steps)
[fe416cc9b2128c3b817134f527233a8bedf99b08] Merge pull request #23841 from empoalp/bug_1062749

git bisect good
Bisecting: 10 revisions left to test after this (roughly 3 steps)
[1bfc67f0e3ef9ef3056f9607392fbb762ae69017] Merge pull request #23731 from eeejay/bug-1062989

git bisect good
Bisecting: 5 revisions left to test after this (roughly 3 steps)
[bc549370546e6d2a8944f12a0e8d2d91b223ef5a] Merge pull request #23848 from daleharvey/1053261

git bisect bad
Bisecting: 1 revision left to test after this (roughly 1 step)
[3f5278adb843f2a8bf93d1890e35f2302cda7b74] Merge pull request #23885 from KevinGrandon/bug_1063694_search_unresponsive

git bisect bad
Bisecting: 1 revision left to test after this (roughly 1 step)
[11e8238cc214e13ec244f15f0978a67f41926198] Bug 1063694 - [System] Fix SearchWindow prototype r=alive

git bisect bad
11e8238cc214e13ec244f15f0978a67f41926198 is the first bad commit
commit 11e8238cc214e13ec244f15f0978a67f41926198
Author: Kevin Grandon <kevingrandon@yahoo.com>
Date:   Tue Sep 9 23:29:32 2014 -0700

    Bug 1063694 - [System] Fix SearchWindow prototype r=alive

Caused by me in bug 1063694.

	Kevin Grandon :kgrandon Assignee
	Comment 6 • 10 years ago

Comment on attachment 8489084 [details]
Github pull request

Alive - could you give this a quick review? The problem is that rocketbar is detecting the open-app request and closing rocketbar. By removing/adding the listener we can get around this. Any better ideas?

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 7 • 10 years ago

Comment on attachment 8489084 [details]
Github pull request

My opinion is the same as bug 1065658; it seems we could just drop the open-app listener because the search window will be in the long run closed if the app is asking to open by appopened event. What do you think?

	Kevin Grandon :kgrandon Assignee
	Comment 8 • 10 years ago

Alive - we can do that, but then we run into bug 1060045 - which is about launching apps. Right now we rely on the system app to manage the search window visibility for us when we receive launch events. We could probably do that in rocketbar.js instead and manually close ourself, but I think that we would probably see some flashing of the backgrounded app which might not be too good.

I think I looked at using appopened before, but this did not solve 1060045. Alive - can you think of some way to solve this and bug 1060045 while using the appopened event?

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 9 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #8)
> Alive - we can do that, but then we run into bug 1060045 - which is about
> launching apps. Right now we rely on the system app to manage the search

> 
> I think I looked at using appopened before, but this did not solve 1060045.
> Alive - can you think of some way to solve this and bug 1060045 while using
> the appopened event?

Sorry I didn't notice and think of that, hmm...
I think we made some mistake in that bug without a deep consideration, now let's try to resolve it in another way.

The reason of appopened is not published for an already opened app is because this will trigger too many handlers to do redundant work.

The reason of not using open-app is already described above.

> We could probably do
> that in rocketbar.js instead and manually close ourself, but I think that we
> would probably see some flashing of the backgrounded app which might not be
> too good.

Honestly we don't do anything (wait the active app window to repaint) right now when rocketbar is shown/hidden.  We will do in bug 1055299 but still not waiting. Flashing is expected and it's a gecko visibility API design issue.

So the only way to resolve this bug and bug 1055299 at the same time is
* do not use open-app listener.
* do window.close() in search app once it fires the activity to invoke marketplace.
* do not do window.close() in search app if it's adding bookmark.

Please note you need to take care if AppWindow.prototype._handle_mozbrowserclose work for searchWindow. If not we may need to override.

To prevent flashing it's another big issue later in the hierarchy management so let's not address it in this bug.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 10 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> So the only way to resolve this bug and bug 1055299 at the same time is

I mean bug 1060045 ;)

	Kevin Grandon :kgrandon Assignee
	Comment 11 • 10 years ago

Comment on attachment 8489084 [details]
Github pull request

I think your suggestion about not processing open-app is the safest thing to do here, and least amount of code to introduce. 

Also added a unit + marionette test to catch this in the future. Could you take a look again? Thanks!

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 12 • 10 years ago

Comment on attachment 8489084 [details]
Github pull request

r=me

	Gregor Wagner [:gwagner]
	Comment 13 • 10 years ago

Regression

	Kevin Grandon :kgrandon Assignee
	Comment 14 • 10 years ago

Master: https://github.com/mozilla-b2g/gaia/commit/ea06ae56236a06322d8dfe8dd1488b09293dd571

	Kevin Grandon :kgrandon Assignee
	Comment 15 • 10 years ago

Comment on attachment 8489084 [details]
Github pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature work.
[User impact] if declined: Users will be unable to use this feature.
[Testing completed]: Manual and unit testing.
[Risk to taking this patch] (and alternatives if risky): Low risk. Smallish patch and it's already broken.
[String changes made]: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/29aa75cea8d3903f4749944f702c647a5a0567ec

	Kevin Grandon :kgrandon Assignee
	Comment 17 • 10 years ago

I've finally landed the test that was originally part of the PR (but split out due to intermittent issues). It turns out that the intermittent issues were solved in bug 1071349.

https://github.com/mozilla-b2g/gaia/commit/861993dfe6af49435ecb8cf6d1ec2fde0b10ccce

	Kevin Grandon :kgrandon Assignee
	Comment 18 • 10 years ago

A test was added for this here: https://github.com/mozilla-b2g/gaia/blob/master/apps/search/test/marionette/bookmark_web_result_test.js

	cnelson
	Comment 19 • 10 years ago

This issue is verified fixed on Flame 2.1 and Flame 2.2.

The "Add to Homescreen" page will appear correctly when the user long taps on a search result.

Flame 2.2 

Device: Flame 2.2 Master KK (319mb) (Full Flash)
BuildID: 20141017040208
Gaia: abef62c0623e5504a97b4fd411e879a67b285b52
Gecko: ae1dfa192faf
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 36.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Flame 2.1 

Device: Flame 2.1 KK (319mb) (Full Flash)
BuildID: 20141017001201
Gaia: 1ea74943cfe525c76a074ca1d7de8e51a70f6b98
Gecko: 2befa902ff5c
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0