Closed Bug 862069 Opened 8 years ago Closed 8 years ago

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

Categories

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

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23
Tracking Status
firefox22 + fixed
firefox23 --- verified

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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.
Attached patch updateKeyboardInputType.patch (obsolete) — Splinter Review
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.
Attachment #738914 - Flags: review?(nchen)
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.
Attachment #738914 - Flags: review?(nchen) → feedback+
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?
(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.
Patch v2 changes:
 * Revert setInputType() to setRawInputType()
 * Call updateKeyboardInputType() before updateGoButton() which will decide whether to restartInput() based on isSearchQuery()
Attachment #738914 - Attachment is obsolete: true
Attachment #739773 - Flags: review?(nchen)
Comment on attachment 739773 [details] [diff] [review]
updateKeyboardInputType-v2.patch

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

Thanks!
Attachment #739773 - Flags: review?(nchen) → review+
https://hg.mozilla.org/mozilla-central/rev/6953e922b5c7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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
Attached patch Patch for beta uplift (obsolete) — Splinter Review
[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
Attachment #758056 - Flags: review?(cpeterson)
Attachment #758056 - Flags: approval-mozilla-beta?
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 ?:. :)
Attachment #758056 - Flags: review?(cpeterson) → review+
Please update the patch so that we can approve for uplift for b5 to prevent this new regression.
[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
Attachment #758056 - Attachment is obsolete: true
Attachment #758056 - Flags: approval-mozilla-beta?
Attachment #758845 - Flags: review+
Attachment #758845 - Flags: approval-mozilla-beta?
Attachment #758845 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.