Closed Bug 1137483 Opened 9 years ago Closed 9 years ago

Add a "quick search" bar of search engines

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

x86
Android
defect
Not set
normal

Tracking

(firefox41 fixed, relnote-firefox 41+)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed
relnote-firefox --- 41+

People

(Reporter: antlam, Assigned: mcomella)

References

Details

Attachments

(8 files, 2 obsolete files)

203.64 KB, image/png
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
This is something we've been tossing around for a while now and I thought I'd separate it from bug 1071730. Essentially it's what the Desktop side already does, and it'll search what the user has already typed.

In the mock, each engine's tap area is 48 dp x 72 dp and I just put in placeholders for their favicon. Tablet version to come.

Anyone care to take a stab at these improvements again? :D
Adding Bnicholson to stay in sync for the iOS side. 

We should keep these experiences in similar across platforms
See Also: → 1134796
QA Contact: flaviu.cos
Flags: needinfo?(michael.l.comella)
NI self: look into pre-landing strings so we can try to get this into 39.
We'll need a String for users on screen readers to tell them what the search engine selections do. I was thinking, "Search with X" where X is the search engine highlighted. There's somewhat related precedent in the Search Activity too.

What do you think, Anthony?
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
Assignee: nobody → michael.l.comella
Works for me!
Flags: needinfo?(alam)
/r/6145 - Bug 1137483 - Preland Strings for awesomescreen search engine bar. r=margaret

Pull down this commit:

hg pull review -r f37d76e3300b5731e3552aaa0f36ebee27213946
Attachment #8583911 - Flags: review?(margaret.leibovic)
Attachment #8583911 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

https://reviewboard.mozilla.org/r/6143/#review5157

::: mobile/android/base/locales/en-US/android_strings.dtd
(Diff revision 1)
> +<!ENTITY search_bar_item_desc "Search with &formatS;">

Nice l10n comment.
<antlam> as users remove or hide their engines, if it's less than the width of the phone, the group of engines should center
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

/r/6145 - Bug 1137483 - Add search engine bar to the bottom of BrowserSearch. r=liuche
/r/7419 - Bug 1137483 - Center SearchEngineBar when it doesn't fill the screen. r=liuche
/r/7421 - Bug 1137483 - Add telemetry for items selected from the quick search bar. r=liuche

Pull down these commits:

hg pull -r 342320b842a88462f3a5065c421cfa97d8e85c02 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8583911 - Flags: review+
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

/r/6145 - Bug 1137483 - Add search engine bar to the bottom of BrowserSearch. r=liuche
/r/7419 - Bug 1137483 - Center SearchEngineBar when it doesn't fill the screen. r=liuche
/r/7421 - Bug 1137483 - Add telemetry for items selected from the quick search bar. r=liuche

Pull down these commits:

hg pull -r 342320b842a88462f3a5065c421cfa97d8e85c02 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8583911 - Flags: review?(liuche)
Still TODO:
  * Set ContentDescription on engines
  * (?followup) If you long press and drag upwards off of a search engine, it remains highlighted as pressed. Probably a TwoWayView problem
  * Make the bar appear above the keyboard
  * (?followup to simplify working on the feature) Custom search engines (it works, but the appearance is kind of broken)
I commented out [1]

 setBackgroundDrawable(null);

in GeckoApp to set a background drawable all the time and we again have the issue we tried to fix with bug 933422 where, when the keyboard hides and the window resizes, there is a graphics artifacts under the keyboard. I haven't seen this from other apps so I'm not sure what we might do differently when we resize the window, or if other apps just don't resize the window when the keyboard appears.

We always see it in ListViews so perhaps it's a ListView glitch.

If Anthony wants the search engines under the keyboard, I think we'll have this problem again.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?rev=487d4236c73f#1930
It could be that we handle our own configuration changes, particularly for keyboard|keyboardHidden [1].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in?rev=b4bf97300f2c#114
(In reply to Michael Comella (:mcomella) from comment #13)
> I commented out [1]
> 
>  setBackgroundDrawable(null);
> 
> in GeckoApp to set a background drawable all the time and we again have the
> issue we tried to fix with bug 933422

Actually, I just built again and things seem to be working correctly - probably had a bad build before.
Relies on the changes in bug 1157534 because we start using transparent views which rely on the window background color.
Depends on: 1157534
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

/r/6145 - Bug 1137483 - Add search engine bar to the bottom of BrowserSearch. r=liuche
/r/7419 - Bug 1137483 - Center SearchEngineBar when it doesn't fill the screen. r=liuche
/r/7421 - Bug 1137483 - Add telemetry for items selected from the quick search bar. r=liuche
/r/7549 - Bug 1137483 - Set content description of SearchEngineBar items. r=liuche
/r/7551 - Bug 1137483 - Move search engine bar under keyboard when shown. r=liuche

Pull down these commits:

hg pull -r 0e3e93a619be861252a05b6e51eba3b15463a5be https://reviewboard-hg.mozilla.org/gecko/
bug 1156917 sounds like it'll be a chore - let's do it after this lands.
No longer depends on: 1156917
[Tracking Requested - why for this release]:
  Anthony says Product requested 39.
Yeah, I'd like to put this in 39. Desktop already has this, iOS will too, and I don't want us to lag behind. Even if at the cost of some lesser-impact UX polish issues.
https://reviewboard.mozilla.org/r/6145/#review6503

Nice, this looks good.

::: mobile/android/base/home/SearchEngineBar.java:38
(Diff revision 3)
> +    public SearchEngineBar(final Context context, final AttributeSet attrs) {

Making these final args makes sense to me, since we are passing these arguments to the super. However, this doesn't seem to be standard in our code - I'm undecided as to whether it should be, or if it's just Java verbosity.
https://reviewboard.mozilla.org/r/7419/#review6509

I'm seeing an extra file for search_engine_item, but nothing in it. Just wondering if reviewboard is doing something weird to that commit?
https://reviewboard.mozilla.org/r/7551/#review6515

Asked mcomella about why this was a tradeoff (i.e., why didn't we do this originally) and it's because there is more overdraw behind BrowserToolbar, because the white background is set to the entire window (as opposed to just forcing the BrowserSearch view to not resize). I'm okay with the slight overdraw increase.

This has the odd effect of not working on Gingerbread anymore, though. I say "anymore" because before this commit, the search-icons-bar did show up above the keyboard on GB devices (although only on subsequent times bringing up the keyboard, not the first time).

I'm also okay with filing a follow-up for GB.
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

https://reviewboard.mozilla.org/r/6143/#review6519

Ship It!
Attachment #8583911 - Flags: review?(liuche) → review+
https://reviewboard.mozilla.org/r/6145/#review6561

> Making these final args makes sense to me, since we are passing these arguments to the super. However, this doesn't seem to be standard in our code - I'm undecided as to whether it should be, or if it's just Java verbosity.

I am of the opinion that if verbosity does not negatively affect (or improves!) readability but improves the code, it's worthwhile - what's a few typed characters to time saved on regressions down the road?

I've never found final variables to decrease readability and they're safer because they show explicit intent - I feel it should be our convention.
https://reviewboard.mozilla.org/r/7419/#review6563

`hg log` shows I added the file with content - I think it's reviewboard being weird.
If necessary, bug 1158282, bug 1156917, and bug 1159360 should be uplifted to ride the trains with this.
No longer blocks: 1158282
Depends on: 1158282, 1156917
Hi Anthony, I have received approval from Amazon on the logo you sent.  I am still waiting for feedback from Bing, Google and Yahoo.  I think you're all good on DDG, as well.  Thanks, Joanne
Michael and Anthony: It is late in the aurora cycle at this point. I'd prefer to see this stabilized on Nightly and for it to ride the trains, as we expect with most features, and to release with 40.  What is driving this to need uplift to 39?
Flags: needinfo?(alam)
I'm ok to push this to 40, let's quickly check with Karen though.
Flags: needinfo?(alam) → needinfo?(krudnitski)
40 is ok, but no later (if we can help it, of course!)
Flags: needinfo?(krudnitski)
I added a `Thread.sleep(1000);` between the two Android back button presses [1] and it seems to have fixed the issue - I guess my new feature has just aggrivated this pre-existing issue (perhaps because it takes longer for the UI to draw?).

Looking at UITest, it looks like we use robotium's `Solo.goBack` which (after looking at the source) waits 500 seconds after the back button is pressed. I'll switch the test over to using that method (but it'd probably be pretty easy to make this a UITest...).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testInputUrlBar.java#91
Flags: needinfo?(michael.l.comella)
Depends on: 1161150
testInputUrlBar started crashing frequently after this check-in -- see bug 1161150.
sorry had to back this out since after this landing we had a extrem high failure rate in rc2 tests on android. Backing this out (and Bug 1154053) made this green and so we are able to reopen the trees
Flags: needinfo?(michael.l.comella)
Assignee: michael.l.comella → gfritzsche
Status: NEW → ASSIGNED
Sorry for the noise here, bzexport tripped on me.
Assignee: gfritzsche → michael.l.comella
Attachment #8603828 - Attachment is obsolete: true
Attachment #8603828 - Flags: review?(rvitillo)
Attachment #8583911 - Flags: review?(margaret.leibovic)
Attachment #8583911 - Flags: review?(liuche)
Attachment #8583911 - Flags: review+
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

/r/6145 - Bug 1137483 - Add search engine bar to the bottom of BrowserSearch. r=liuche
/r/7419 - Bug 1137483 - Center SearchEngineBar when it doesn't fill the screen. r=liuche
/r/7421 - Bug 1137483 - Add telemetry for items selected from the quick search bar. r=liuche
/r/7549 - Bug 1137483 - Set content description of SearchEngineBar items. r=liuche
/r/7551 - Bug 1137483 - Move search engine bar under keyboard when shown. r=liuche
/r/8679 - Bug 1137483 - Rewrite testInputUrlBar to extend UITest. r=margaret

Pull down these commits:

hg pull -r 6665c3586d66bf5bbe1f5af2b518978d44519a8a https://reviewboard-hg.mozilla.org/gecko/
Flags: needinfo?(michael.l.comella)
Attachment #8583911 - Flags: review?(liuche) → review+
As a fix for this broken & brittle test, I rewrote it to extend UITest and it works locally: try push in comment 50.
After investigating the failures in comment 50, nalexander concluded ProGuard was removing the internal fields of AppConstants.Versions. Without knowing anything about ProGuard, I tried editing the file to prevent this; try in comment 52.
The try run in comment 54 failed because the Solo.typeText ignored selection state. However, I wonder if this is actually a race condition where the time between entering editing mode and typing, the EditText hasn't become focused, and thus selected, yet (I assume this would happen at the end of the animation).
The test in comment 56 waits until the text is selected before returning from ToolbarComponent.enterEditingMode.
The test in comment 56 failed. I think there's a bigger issue going on under the hood that Solo.typeText doesn't work as expected, but I don't think it's worth taking the time researching. Instead, I'm just going to use sendKeys directly (which has been working on GB) and wait after typing just to be safe (because it works at inhuman speeds). try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22018b1628fb
Attachment #8583911 - Flags: review?(nalexander)
Attachment #8583911 - Flags: review?(liuche)
Attachment #8583911 - Flags: review+
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

/r/6145 - Bug 1137483 - Add search engine bar to the bottom of BrowserSearch. r=liuche
/r/7419 - Bug 1137483 - Center SearchEngineBar when it doesn't fill the screen. r=liuche
/r/7421 - Bug 1137483 - Add telemetry for items selected from the quick search bar. r=liuche
/r/7549 - Bug 1137483 - Set content description of SearchEngineBar items. r=liuche
/r/7551 - Bug 1137483 - Move search engine bar under keyboard when shown. r=liuche
/r/8679 - Bug 1137483 - Add AppConstants.Versions to ProGuard config to prevent removal. r=nalexander
/r/8801 - Bug 1137483 - Rewrite testInputUrlBar to extend UITest. r=margaret

Pull down these commits:

hg pull -r 775ce2fb0cdae1fb7c851d8ae07baf0dfb9c97f5 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8583911 - Flags: review?(liuche) → review+
(In reply to Michael Comella (:mcomella) from comment #59)
> Green!

To be explicit, GB and 4.3 haven't finished yet, and I just added some retriggers for 4.0.
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

I hate RB with the fire of a thousand frustrated suns so here's my r+ for the Proguard configuration: if it works for you it works for me.
Attachment #8583911 - Flags: review?(nalexander) → review+
The try in comment 58 fails more often than I'd like on 2.3 and 4.3, both of which run on an emulator. 2.3 crashes and I'm not sure why. 4.3 fails on waiting for the urledittext to be the input method target, notably after some animations - I wonder if these emulators are just too damn slow and we're failing because of that.

Screw it, I'm disabling the original test on 2.3 - we can figure out the new testInputUrlBar in a new bug.
Attachment #8583911 - Flags: review?(nalexander)
Attachment #8583911 - Flags: review?(liuche)
Attachment #8583911 - Flags: review+
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

/r/6145 - Bug 1137483 - Add search engine bar to the bottom of BrowserSearch. r=liuche
/r/7419 - Bug 1137483 - Center SearchEngineBar when it doesn't fill the screen. r=liuche
/r/7421 - Bug 1137483 - Add telemetry for items selected from the quick search bar. r=liuche
/r/7549 - Bug 1137483 - Set content description of SearchEngineBar items. r=liuche
/r/7551 - Bug 1137483 - Move search engine bar under keyboard when shown. r=liuche
/r/8679 - Bug 1137483 - Add AppConstants.Versions to ProGuard config to prevent removal. r=nalexander
/r/8801 - Bug 1137483 - Disable testInputUrlBar on 2.3. r=margaret

Pull down these commits:

hg pull -r 8ec1244cfaef423c877924ff72194144a5c4c6c8 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8583911 - Flags: review?(nalexander)
Attachment #8583911 - Flags: review?(liuche)
Attachment #8583911 - Flags: review+
https://reviewboard.mozilla.org/r/8801/#review7605

::: mobile/android/tests/browser/robocop/robocop.ini:55
(Diff revision 2)
> +# disabled on 2.3 bug 1137483

Instead of the bug number from this bug (which you can get at from blame), I would add the bug number of whatever bug would fix this.
Comment on attachment 8583911 [details]
MozReview Request: bz://1137483/mcomella

https://reviewboard.mozilla.org/r/6143/#review7607

Ship It!
Attachment #8583911 - Flags: review?(margaret.leibovic) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → Firefox 41
Depends on: 1170724
Flaviu, re IRC, this bug should not remove the old method of searching - that's bug 1158275 - and we can ship without removing it (according to Antlam).
Attachment #8583911 - Attachment is obsolete: true
Attachment #8619608 - Flags: review+
Attachment #8619609 - Flags: review+
Attachment #8619610 - Flags: review+
Attachment #8619611 - Flags: review+
Attachment #8619612 - Flags: review+
Attachment #8619613 - Flags: review+
Attachment #8619614 - Flags: review+
Release Note Request (optional, but appreciated)
[Why is this notable]: Easier to search with different search providers
[Suggested wording]: Quickly search with different search providers from the search panel
[Links (documentation, blog post, etc)]:
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.