862069 - Change AwesomeBar's default input mode from Search mode to URL mode for gesture keyboards

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Chris Peterson [:cpeterson]]

After bug 859212 landed, the AwesomeBar's default input mode is Search (text) mode for gesture keyboards. Unfortunately, that VKB layout has no '/' key! We should probably revert to defaulting to URL mode and switching into Search mode after the user enters a space.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: updateKeyboardInputType.patch

Toggle gesture keyboards' input type when user enters a search query in the AwesomeBar.

I also tested this change for non-gesture keyboards, but most either did not change their keyboard layout or just took away the ".com" key and made the space bar a little bigger. To minimize risk from IME bugs, I think we should just keep this patch focused on the few gesture keyboards we've tested.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 738914 [details] [diff] [review]
updateKeyboardInputType.patch

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

::: mobile/android/base/AwesomeBar.java
@@ +756,5 @@
>          // If the AwesomeBar has a composition string, don't call updateGoButton().
>          // That method resets IME and composition state will be broken.
>          if (!hasCompositionString(s)) {
>              updateGoButton(text);
> +            updateKeyboardInputType();

If you put this above updateGoButton(text), updateGoButton will call restartInput at the right time for you. See below.

@@ +786,5 @@
> +        int newInputType = mIsUsingGestureKeyboard && StringUtils.isSearchQuery(text, false)
> +                           ? (currentInputType & ~InputType.TYPE_TEXT_VARIATION_URI) // Text mode
> +                           : (currentInputType | InputType.TYPE_TEXT_VARIATION_URI); // URL mode
> +        if (newInputType != currentInputType) {
> +            mText.setInputType(newInputType);

Did you change setRawInputType to setInputType because setInputType restarts the input? If so, I think you can change this back to setRawInputType, and simply let updateGoButton above call restartInput for you.

Comment 3

[Chris Peterson [:cpeterson]]

I see now why setRawInputType() was not working for me. Bug 836589 changed updateGoButton() to not always call restartInput(), so I had to call setInputType() to force the restart.

When I enter "hello world" and then remove the space (leaving "helloworld"), isSearchQuery() sees this as an ambiguous case and does not switch the go/search mode. Why do we want that behavior? Does the fix for bug 836589 depend on that behavior?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #3)
> I see now why setRawInputType() was not working for me. Bug 836589 changed
> updateGoButton() to not always call restartInput(), so I had to call
> setInputType() to force the restart.
> 
> When I enter "hello world" and then remove the space (leaving "helloworld"),
> isSearchQuery() sees this as an ambiguous case and does not switch the
> go/search mode. Why do we want that behavior? Does the fix for bug 836589
> depend on that behavior?

Yes. I think in general we want to call restartInput sparingly, so for ambiguous cases we don't want to call restartInput right away.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: updateKeyboardInputType-v2.patch

Patch v2 changes:
 * Revert setInputType() to setRawInputType()
 * Call updateKeyboardInputType() before updateGoButton() which will decide whether to restartInput() based on isSearchQuery()

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 739773 [details] [diff] [review]
updateKeyboardInputType-v2.patch

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

Thanks!

Comment 7

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6953e922b5c7

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6953e922b5c7

Comment 9

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 23.0a2 (2013-05-16)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1
-Keyboard: Swype Beta 1.4

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Patch for beta uplift

[Approval Request Comment]

Bug caused by (feature/regressing bug #): N/A

User impact if declined: Bug 861943, unable to select all of URL for copying, bad user experience

Testing completed (on m-c, etc.): Locally, Aurora, Nightly

Risk to taking this patch (and alternatives if risky): Very little; the patch has been tested in Aurora and Nightly for many weeks.

String or IDL/UUID changes made by this patch: None

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 758056 [details] [diff] [review]
Patch for beta uplift

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

LGTM with change:

::: mobile/android/base/AwesomeBar.java
@@ +375,5 @@
> +        // spaces between words.
> +        String text = mText.getText().toString();
> +        int currentInputType = mText.getInputType();
> +        int newInputType = mIsUsingGestureKeyboard && StringUtils.isSearchQuery(text, false)
> +                           ? (currentInputType & ~InputType.TYPE_TEXT_VARIATION_URI) // Text mode

Please add parens around `mIsUsingGestureKeyboard && StringUtils.isSearchQuery(text, false)` because to ensure there is no confusion about the operator precedence of && and ?:. btw, this code is correct because && does have higher precedence than ?:. :)

Comment 12

[Alex Keybl [:akeybl]]

Please update the patch so that we can approve for uplift for b5 to prevent this new regression.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Patch for beta uplift

[Approval Request Comment]

Bug caused by (feature/regressing bug #): N/A

User impact if declined: Bug 861943, unable to select all of URL for copying, bad user experience

Testing completed (on m-c, etc.): Locally, Aurora, Nightly

Risk to taking this patch (and alternatives if risky): Very little; the patch has been tested in Aurora and Nightly for many weeks.

String or IDL/UUID changes made by this patch: None

Comment 14

[[Back August 16] Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/4987ce680622