1117970 - Change search engine from search app

Status: VERIFIED FIXED

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

Comment 1

[Dale Harvey (:daleharvey)]

So this is turning out harder than expected

If I used the value-selector building blocks directly the dialog will only show inside the search apps appWindow (below the url bar etc).

I tried triggering a <select> from inside the search app, this will get picked up and displayed inside the system app, however the search app is a weird special case, the dialog is displayed inside the homescreen stack which still sits behind the search app (its displayed but not visible)

It isnt looking easy to get homescreen dialogs to display over the search app, will give it a shot though, would be nice if the search app acted more like a regular app, but since it has a transparent overlay over the homescreen that seems hard. 

Alive / Kevin, either of you have any ideas about this?

Comment 2

[Kevin Grandon :kgrandon]

Sounds like we should be using a standard <select> here, but we need to inject the content into the search app stack, and not home screen. There is likely some functionality that we can override to do this, like we override _handle_launchactivity to handle activity launches.

I'm not sure what we have for <select> elements, but maybe Alive knows. Otherwise I don't mind taking a further in-depth look here.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

What we need to do is make Rocketbar be able to get 'mozChromeEvent' with "inputmethod-contextchange".
HierarchyManager is responsible to dispatch this mozChromeEvent to the top most module so rocketbar doesn't need to addEventLister but only put _handle_mozChromeEvent() function inside it and redirect it to the SearchWindow.

See SystemDialogManager for reference. Lemme know if you need me to implement it.

Comment 4

[Dale Harvey (:daleharvey)]

Thats awesome thanks Kevin / Alive, injecting into the search app sounds like the best approach, cheers for this tips

Comment 5

[Dale Harvey (:daleharvey)]

Alive, the _handle_mozChromeEvent() handling worked great. I have a messy implementation where I build the dialog myself however it seems brittle and there is a lot of duplication. I would like to have value_selector still handle the rendering and behaviour of the dialog but have the UI it constructs sent to the searchWindow stack as Kevin suggested. It seems like in terms of dialog behaviour dialogs triggered while the search app is focused should be sent there.

Trying to figure out how to make it work like that now, but wondered if you had any ideas / opinions about that?

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Dale Harvey (:daleharvey) from comment #5)
> Alive, the _handle_mozChromeEvent() handling worked great. I have a messy
> implementation where I build the dialog myself however it seems brittle and
> there is a lot of duplication. I would like to have value_selector still
> handle the rendering and behaviour of the dialog but have the UI it
> constructs sent to the searchWindow stack as Kevin suggested. It seems like
> in terms of dialog behaviour dialogs triggered while the search app is
> focused should be sent there.
> 
> Trying to figure out how to make it work like that now, but wondered if you
> had any ideas / opinions about that?

My suggestion in comment 3 is what you need to re-use the value selector UI in system.
Not sure what's blocking you? I guess it's because we don't focus the searchWindow.. What's the timing that you triggers the select in search app?

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

FYI, inject value selector into search window is a blocker now in https://bugzilla.mozilla.org/show_bug.cgi?id=1124216 so I will implement it for you.

Comment 8

[Dale Harvey (:daleharvey)]

> My suggestion in comment 3 is what you need to re-use the value selector UI in system.
> Not sure what's blocking you? I guess it's because we don't focus the searchWindow.. What's the timing 
> that you triggers the select in search app?

I just put a <select> inside the search app and triggered it by clicking, it got shown in the homescreen, I could overide _handle_mozChromeEvent() and show the select where I wanted, but I couldnt figure out how to reused the existing value selector code to render it, it seemed like that code was fairly coupled to the app window manager and it picked where to render based on that

> FYI, inject value selector into search window is a blocker now in 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1124216 so I will implement it for you.

Thats lucky for me, thanks :) will take a look at that patch now

Comment 9

[Dale Harvey (:daleharvey)]

So I am having a weird issue with implementing this

https://github.com/daleharvey/gaia/commit/2fc17837ecd66982b8894304471490d239c15396

Has the implementation and everything works as it should sometimes. However very regularly the select boxes from either the search application or the settings application will not be shown, the select boxes are built with .innerHTML, they seem to get written absolutely fine, they get displayed but when I click on them nothing happens.

Trying to debug now, but pinging Alive in case you have any ideas

Comment 10

[Dale Harvey (:daleharvey)]

Added a commit to make the select box within the search app more visible

https://github.com/mozilla-b2g/gaia/pull/27886

Comment 11

[Dale Harvey (:daleharvey)]

Is a log from a select box in the settings app that didnt display

E/HWComposer( 9642): Non-uniform vsync interval: 2724806041
E/HWComposer( 9642): Non-uniform vsync interval: 10510677
I/GeckoConsole( 9642): Content JS LOG: [AppWindow][Settings][AppWindow_6][624.658]  Handling mozbrowserasyncscroll event...
I/GeckoConsole( 9642):     at aw_debug (app://system.gaiamobile.org/js/app_window.js:1130:0)
I/GeckoConsole( 9642): Content JS LOG: [Search][app://search.gaiamobile.org][search][624.779] publishing internal event: inputmethod-contextchange
I/GeckoConsole( 9642):     at aw_debug (app://system.gaiamobile.org/js/app_window.js:1130:0)
I/GeckoConsole( 9642): Content JS LOG: [Search][app://search.gaiamobile.org][search][624.786] handling _inputmethod-contextchange
I/GeckoConsole( 9642):     at aw_debug (app://system.gaiamobile.org/js/app_window.js:1130:0)
I/GeckoConsole( 9642): Content JS LOG: [Search][app://search.gaiamobile.org][search][624.789] aria-hidden on browser element:true
I/GeckoConsole( 9642):     at aw_debug (app://system.gaiamobile.org/js/app_window.js:1130:0)
I/GeckoConsole( 9642): Content JS LOG: [Search][app://search.gaiamobile.org][search][624.791] select triggered{"type":"inputmethod-contextchange","inputType":"select-one","value":"yahoo","choices":{"multiple":false,"choices":[{"group":false,"inGroup":false,"text":"Google","disabled":false,"selected":false,"optionIndex":0},{"group":false,"inGroup":false,"text":"Yahoo","disabled":false,"selected":true,"optionIndex":1},{"group":false,"inGroup":false,"text":"Bing","disabled":false,"selected":false,"optionIndex":2},{"group":false,"inGroup":false,"text":"DuckDuckGo","disabled":false,"selected":false,"optionIndex":3}]},"min":"","max":""}
I/GeckoConsole( 9642):     at aw_debug (app://system.gaiamobile.org/js/app_window.js:1130:0)

Comment 12

[Dale Harvey (:daleharvey)]

I assume the select is showing, but in the wrong place, webIDE is broken for me right now so cant figure out where

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Looks like it is dispatching to the wrong window, investigating...

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Hmm, I am seeing the value selector if I click the "white square" in bottom of search window, and it works for me. I am recording a video.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Dale Harvey (:daleharvey) from comment #12)
> I assume the select is showing, but in the wrong place, webIDE is broken for
> me right now so cant figure out where

You could try Firefox Aurora and use its WebIDE. Nightly's WebIDE does not work for me either.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #15)
> (In reply to Dale Harvey (:daleharvey) from comment #12)
> > I assume the select is showing, but in the wrong place, webIDE is broken for
> > me right now so cant figure out where
> 
> You could try Firefox Aurora and use its WebIDE. Nightly's WebIDE does not
> work for me either.

But I found that if SearchWindow has an ActivityWindow (bug 1124216) it will not show :/
Might be a missing part from the patch, looking!

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

Filed bug 1129329; lemme know if you still have issues after the patch is applied.

Comment 18

[Dale Harvey (:daleharvey)]

Awesome, sorry for getting stuck on that, good catch

Pretty bad we dont have any integration tests that would have caught that, but I will add some in with this patch

Comment 19

[Dale Harvey (:daleharvey)]

Ok, this all implemented in https://github.com/mozilla-b2g/gaia/pull/27886, the dependency patch worked well. However to test it I need to test in the same way as https://bugzilla.mozilla.org/show_bug.cgi?id=1127939 so I need to get that fixed first

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/27886

Changes to window_layout.css are to show the select full screen (there are similiar clauses for the context menu etc)

Tests added, will run them extra to make sure, Cheers

Comment 21

[Ben Francis [:benfrancis]]

Comment on attachment 8559819 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/27886

Cool, looks good to me, with a couple of nits.

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8559819 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/27886

Nice feature!

Comment 23

[Dale Harvey (:daleharvey)]

Addressed nits and merged, thanks

https://github.com/mozilla-b2g/gaia/commit/ed368bc76f6e2c4ac3817c90494a7b52b2be42d0

Comment 24

[Dale Harvey (:daleharvey)]

Comment on attachment 8559819 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/27886

Clearing review, merged

Comment 25

[Dale Harvey (:daleharvey)]

aaaannnnnddd backed out ...

Green try run but still failed tests, expect its related to https://bugzilla.mozilla.org/show_bug.cgi?id=1127939

https://github.com/mozilla-b2g/gaia/commit/0244121522343877d65a69377226a836688e3004

Comment 26

[Kevin Grandon :kgrandon]

I guess this should be re-opened? Tree is also closed, so let's wait for it to open before landing again.

Comment 27

[Dale Harvey (:daleharvey)]

Oh sorry was meant to reopen

Comment 28

[GitHub Autolander Bot]

Attachment: [PullReq] daleharvey:1117970-redux to mozilla-b2g:master

Comment 29

[Dale Harvey (:daleharvey)]

Triggered 10 reruns of 1 and 3, no restarts as far as I can see, the fix to https://bugzilla.mozilla.org/show_bug.cgi?id=1127939 looks to have fixed this as well - https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=3857f9094fdf

https://github.com/mozilla-b2g/gaia/commit/6fb78df35017173bf055c6a5511dd91d2c874598

Comment 30

[Hermes Cheng (inactive after July 27, 2015)]

Could you please land the patch to v2.2? Thanks.

Comment 31

[Dale Harvey (:daleharvey)]

Pushed to 2.2 in https://github.com/mozilla-b2g/gaia/commit/a59d7b70f0d2ac8da4e4e3c3740b44b9463d00a5

Green try run @ https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=4a6c9879b64e

Comment 32

[Dale Harvey (:daleharvey)]

Comment on attachment 8559819 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/27886

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 2.2 Feature Development, accidentally already landed without approval, proactively asking for approval
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 33

[Brogan Zumwalt [Inactive]]

Issue verified fixed in Flame 3.0

With a search term present in the Rocketbar, tapping the default search engine header in results section brings up a value selector screen with Google, Yahoo, Bing, and DuckDuckGo listed as options. Current engine is highlighted and changes are reflected. Tapping OK after making changes shows that search engine header adjusts to reflect changed search engine. Tapping enter on keyboard with search term present directs user to correct results page for selected search engine. Likewise, tapping on search suggestions brings user to correct results page as well. 

Search results do not auto-update once search engine is changed within search app, but it seems this is outside the scope of this bug and does not block verification. Filed new bug 1131761 to address this issue.

Added verifyme keyword for 2.2 verification

Device: Flame 3.0 Master
Build ID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
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

Comment 34

[SandKing]

Attachment: verified video: verify_1117970.MP4

This issue has been verified successfully on latest Flame2.2
STR:
1. Launch "Settings"->"Search"
2. Change the search engine.
3. Search something.
**You can see the default search engine is what you set
Verify video:"verify_1117970.mp4".

Flame 2.2 build:
Build ID               20150210002516
Gaia Revision          b30c8e4303595a0fcb5b640d673cf8503b954701
Gaia Date              2015-02-10 04:09:47
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3e9fa4e70a1b
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150210.041059
Firmware Date          Tue Feb 10 04:11:10 EST 2015
Bootloader             L1TC000118D0

Comment 35

[SandKing]

Attachment: Verified video2: Verified.MP4

Sure, we can change the search engine from search app too, and the result is follow the comment 34.
See verified video2: "Verified.MP4"