Change search engine from search app

VERIFIED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::Search
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: benfrancis, Assigned: daleharvey)

Tracking

unspecified
2.2 S6 (20feb)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe])

User Story

As a user I want to change the search engine directly from the search app

Attachments

(4 attachments)

Comment hidden (empty)
Whiteboard: [systemsfe]
(Assignee)

Updated

3 years ago
Assignee: nobody → dale
Blocks: 1098494
(Assignee)

Comment 1

3 years ago
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?
Flags: needinfo?(kgrandon)
Flags: needinfo?(alive)
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.
Flags: needinfo?(kgrandon)
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.
Flags: needinfo?(alive)
(Assignee)

Comment 4

3 years ago
Thats awesome thanks Kevin / Alive, injecting into the search app sounds like the best approach, cheers for this tips
(Assignee)

Comment 5

3 years ago
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?
Flags: needinfo?(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?
Flags: needinfo?(alive) → needinfo?(dale)
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.
(Assignee)

Comment 8

3 years ago
> 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
Flags: needinfo?(dale)
(Assignee)

Updated

3 years ago
Depends on: 1124216
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2+
(Assignee)

Comment 9

3 years ago
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
Flags: needinfo?(alive)
(Assignee)

Comment 10

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

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

Comment 11

3 years ago
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)
(Assignee)

Comment 12

3 years ago
I assume the select is showing, but in the wrong place, webIDE is broken for me right now so cant figure out where
Looks like it is dispatching to the wrong window, investigating...
Flags: needinfo?(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.
(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.
(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!
Filed bug 1129329; lemme know if you still have issues after the patch is applied.
(Assignee)

Comment 18

3 years ago
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
(Assignee)

Comment 19

3 years ago
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
Depends on: 1127939
(Assignee)

Comment 20

3 years ago
Created attachment 8559819 [details] [review]
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
Attachment #8559819 - Flags: review?(kgrandon)
Attachment #8559819 - Flags: review?(bfrancis)
Attachment #8559819 - Flags: review?(alive)
(Reporter)

Comment 21

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

Cool, looks good to me, with a couple of nits.
Attachment #8559819 - Flags: review?(bfrancis) → review+
(Assignee)

Comment 23

3 years ago
Addressed nits and merged, thanks

https://github.com/mozilla-b2g/gaia/commit/ed368bc76f6e2c4ac3817c90494a7b52b2be42d0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

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

Clearing review, merged
Attachment #8559819 - Flags: review?(kgrandon)
(Assignee)

Comment 25

3 years ago
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
I guess this should be re-opened? Tree is also closed, so let's wait for it to open before landing again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

3 years ago
Oh sorry was meant to reopen
Target Milestone: --- → 2.2 S6 (20feb)
Created attachment 8560857 [details] [review]
[PullReq] daleharvey:1117970-redux to mozilla-b2g:master
(Assignee)

Comment 29

3 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Could you please land the patch to v2.2? Thanks.
Flags: needinfo?(dale)
(Assignee)

Comment 32

3 years ago
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]:
Attachment #8559819 - Flags: approval-gaia-v2.2?
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-master: --- → verified
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Comment 34

3 years ago
Created attachment 8562482 [details]
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

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][QAnalyst-verify-]
status-b2g-v2.2: fixed → verified

Updated

3 years ago
QA Whiteboard: [QAnalyst-Triage+][QAnalyst-verify-] → [QAnalyst-Triage+][MGSEI-Triage+]
Keywords: verifyme

Updated

3 years ago
Attachment #8559819 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+

Comment 35

3 years ago
Created attachment 8562630 [details]
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"
You need to log in before you can comment on or make changes to this bug.