729429 - The search button should not dismiss the awesomescreen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Paul Feher]

Build: Nightly:Fennec/13.0a1 (2012-02-21)
Device: HTC Desire Z 
OS: Android 2.3

Steps to reproduce:
1. Open Fennec.
2. Tap on the awesome bar to open the awesome panel.
3. Tap on the search button of your android phone.

Expected Result:
This should open up the Fennec search menu that should list the following search options: Google, Amazon.com, Twitter, Wikipedia.

Actual Result: 
The fennec home page is displayed.

Comment 1

[Martijn Wargers (dead)]

Ux feedback wanted here.

Comment 2

[Matt Brubeck (:mbrubeck)]

This doesn't block the first release, though I think it's a reasonable thing to implement.  If anyone wants to work on this I can help out.

Comment 3

[daletron]

Hi, me and two other students at Cal Poly Pomona would like to work on this bug for an assignment.

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to daletron from comment #3)
> Hi, me and two other students at Cal Poly Pomona would like to work on this
> bug for an assignment.

Great -- you can find links to the code and build instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

There are a lot of steps and lots of information involved, so feel free to ask questions here, or email me, or come to the #mobile channel on irc.mozilla.org if you need help.

As a starting point, you should be able to add a key event handler to this activity to detect when the search key is pressed:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java

Comment 5

[daletron]

thanks matt for the info!  that helps alot, will be in contact

Comment 6

[daletron]

Please un-assign me from this bug.  Had many problems trying to even build Fennec in Linux.  Thank you Matt for the help.

Comment 7

[Matt Brubeck (:mbrubeck)]

Okay, no problem.  Feel free to find me on IRC if you'd like any help getting started with Fennec or other development.

Comment 8

[van51]

Hello, I would like to get started with this bug.

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Paul Feher from comment #0)
> Expected Result:
> This should open up the Fennec search menu that should list the following
> search options: Google, Amazon.com, Twitter, Wikipedia.

Unlike older versions of Fennec where you had to type a query and then press a button to view the list of search engines, the current Fennec UI automatically displays the search engines as you type.  So I don't think we need to display a search menu anymore...

> Actual Result: 
> The fennec home page is displayed.

...but this is still the wrong thing to do, and should be fixed.  Perhaps instead we should clear the address bar input field and bring up the keyboard when you press the search button.

Comment 10

[van51]

Attachment: Proposed fix to updated bug

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 638877 [details] [diff] [review]
Proposed fix to updated bug

Thanks for the patch!  Note: When you are attaching a patch that's ready for review, you can set the "review" flag to "?" and enter the email of a reviewer.  (For mentored bugs, the mentor is usually a good reviewer.)  If the patch is not fully ready but you'd like some feedback, set the "feedback" flag instead.

This looks good.  Some minor notes below:

>+        } else if (keyCode == KeyEvent.KEYCODE_SEARCH ){
>+             mText.setText("");
>+             InputMethodManager imm = (InputMethodManager) mText.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
>+             imm.showSoftInput(mText, InputMethodManager.SHOW_IMPLICIT);

We should probably focus the text field too, in case it is un-focused.

The showSoftInput call is not working on my device (the keyboard does not appear), possibly for the reasons discussed in bug 762309.  Does it work on your device?

>+        }else {

Minor style note: There should be a space after the "}".

Comment 12

[van51]

(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> Comment on attachment 638877 [details] [diff] [review]
> Proposed fix to updated bug
> 
> Thanks for the patch!  Note: When you are attaching a patch that's ready for
> review, you can set the "review" flag to "?" and enter the email of a
> reviewer.  (For mentored bugs, the mentor is usually a good reviewer.)  If
> the patch is not fully ready but you'd like some feedback, set the
> "feedback" flag instead.

> >+        }else {
> Minor style note: There should be a space after the "}".

Ok noted!

> This looks good.  Some minor notes below:
> 
> >+        } else if (keyCode == KeyEvent.KEYCODE_SEARCH ){
> >+             mText.setText("");
> >+             InputMethodManager imm = (InputMethodManager) mText.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
> >+             imm.showSoftInput(mText, InputMethodManager.SHOW_IMPLICIT);
> 
> We should probably focus the text field too, in case it is un-focused.
> 
> The showSoftInput call is not working on my device (the keyboard does not
> appear), possibly for the reasons discussed in bug 762309.  Does it work on
> your device?
> 

Yes actually it does work on my device and I can also reproduce the situation of bug 762309. 
So how do we approach this now? Should iI upload possible fixes and someone else can test them?

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to van51 from comment #12)
> > The showSoftInput call is not working on my device (the keyboard does not
> > appear), possibly for the reasons discussed in bug 762309.  Does it work on
> > your device?
> 
> Yes actually it does work on my device and I can also reproduce the
> situation of bug 762309. 
> So how do we approach this now? Should iI upload possible fixes and someone
> else can test them?

If this works on your device, then I think we could just check it in like this.  The problem on my device could be fixed in a follow-up bug if necessary.  (If it only affects a small number of devices, I might not even bother.)

You can upload a patch that addresses the other comments, and request review for it.

Comment 14

[van51]

Attachment: Patch addressing last comments

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 638923 [details] [diff] [review]
Patch addressing last comments

Great!  I'll check this in shortly.  Thank you very much.

Comment 16

[Matt Brubeck (:mbrubeck)]

Pushed to mozilla-inbound.  This will be merged to mozilla-central today or tomorrow, and will appear in nightly builds the next day.  Thanks again!
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f1b29d806b

Comment 17

[Tom S [:evilpie]]

Comment on attachment 638923 [details] [diff] [review]
Patch addressing last comments

Review of attachment 638923 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/AwesomeBar.java
@@ +376,4 @@
>              keyCode == KeyEvent.KEYCODE_VOLUME_UP ||
>              keyCode == KeyEvent.KEYCODE_VOLUME_DOWN) {
>              return super.onKeyDown(keyCode, event);
> +        } else if (keyCode == KeyEvent.KEYCODE_SEARCH ){

Whitespace before the ).

Comment 18

[Matt Brubeck (:mbrubeck)]

(In reply to Tom Schuster [:evilpie] from comment #17)
> Whitespace before the ).

Oops!  Sorry I missed that in review.  Fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c820391d9a2e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66f1b29d806b
https://hg.mozilla.org/mozilla-central/rev/c820391d9a2e

Comment 20

[Paul Feher]

Verified on:
Firefox Nightly 18.0a1 (2012-09-17)
Firefox Aurora 17.0a2 (2012-09-17)
Mobile Firefox 16.0b3
Device: HTC Desire Z
OS: Android 2.3.3

The search button now has the behavior described in comment9 marking this as verified.