994472 - Support URI autocomplete on Swype (and other composition focused) keyboards

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Our current URI autocomplete bails if you're in the middle of a composition. That means that for many popular keyboards it never helps at all. We should see if we can fix it.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

I spent a long time the other day trying to make this work. If we change the real text in the textbox, we often seem to risk breaking things. Races can easily occurr because the user is changing the text at the same time we are. Even if you deal with those problems changes you introduce in the textbox break the current composition.

I played for awhile with deliberately setting a new composition. That semi works, but you can't actually position the cursor inside a composition and so you wind up with weird situations where its hard to guess what will happen with the next letter you press. i.e. the composition shown in the textbox doesn't match the composition the IME has. That seems confusing, but essentially the textbox will say "facebook.com" when really the IME only has "face". Typing "b" will cause no visible change at all. Typing "g" would show "faceg".

This takes an alternative approach of stacking two EditText's on top of one another and using the bottom one for the autocomplete text (right now its using the Hint text in there, but I just did that for styling). That means we don't have to play with the REAL text in the textbox at all. If the user hits enter/go, we pull the autocomplete text and use that as the current url instead (the Go button probably doesn't work in this WIP).

As much as it feels like a hack, I think it would fix a few bugs we have because it never changes the real text typed by the user. It just uses it to change this autocomplete textbox. The slightly prettier version might be to overwrite the textbox'es onDraw(), but I'm pretty sure that's a deep rabbit hole.

Before I dig much further, any opinions or ideas lucas/ian?

Screenshot (this changes our styling here, but we can adjust that:
http://dl.dropboxusercontent.com/u/72157/autocomplete.png

Build:
http://people.mozilla.com/~wjohnston/autocomplete.apk

Comment 2

[Lucas Rocha (:lucasr)]

Before I have a look at the patch, what kind of bugs would this approach fix exactly?

Comment 3

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

Does feel like a hack, but I really like the effect. One thing is that it's not full autocomplete -- I can't tap to the end of the autocompleted text and starting typing at the end. There's a bug now if I add spaces to the middle of the text, it'll become misaligned with the text underneath. Also, stacking text like this throws off font anti-aliasing, but I see the difference is really small so it might not matter.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8404431 [details] [diff] [review]
WIP Patch

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

"I can't tap to the end of the autocompleted text and starting typing at the end. There's a bug now if I add spaces to the middle of the text, it'll become misaligned with the text underneath."

These issues seem like blockers regarding this approach. For instance, I can't think of a good way to handle the spaces in the middle of the text while using two overlapping TextViews.

Wes, have you considered creating a TextView subclass instead? Or even custom copy of the original source code so that we can implement the proper split between the displayed text and IME text?

Comment 5

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #2)
> Before I have a look at the patch, what kind of bugs would this approach fix
> exactly?

Mostly I was trying to make this work with keyboard like Swipe. That's probably the biggest win. I've seen issues with the textbox appending characters at the wrong place as well though, presumably because there are races between what you're really typing

For the spaces thing, we should be removing the suggestion if the text looks like a search. Fixing the tapping to the end sounds a little harder. I also noticed some issues if I scroll the textbox (its hard to get an autocomplete that long :) )

I'll look into the TextView code...

Comment 6

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

This might fix bug 990314 too. I'm also experimenting with changing the EditText string buffer directly, so we can bypass some of the IME interactions that take place in EditText. Hopefully that'll let us change the buffer while preserving compositions, etc.

Comment 7

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

Attachment: WIP - Use text spans to implement autocompletion

This patch uses spans to mark autocomplete text and also keeps selection/composing spans at their original locations when appending autocomplete text. It needs more testing but seems to work pretty well with Swype/Swiftkey/etc.

Comment 8

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

Here's the effect using Swype with the above patch, http://people.mozilla.org/~nchen/builds/bug994472-fennec-31.0a1.png

Here's the build (Fennec nchen), http://people.mozilla.org/~nchen/builds/bug994472-fennec-31.0a1.apk

Comment 9

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

Ian, do you have a preference for the color of the autocomplete text?

We can do hint color, http://dl.dropboxusercontent.com/u/72157/autocomplete.png
Or selection color, http://people.mozilla.org/~nchen/builds/bug994472-fennec-31.0a1.png
Or any color really.

On one hand the hint color is what Google's autosuggestion does. On the other hand, the selection color is closer to what we have right now, and it provides a greater contrast.

Comment 10

[Ian Barlow (:ibarlow)]

(In reply to Jim Chen [:jchen :nchen] from comment #9)
> Ian, do you have a preference for the color of the autocomplete text?
> 
> We can do hint color,
> http://dl.dropboxusercontent.com/u/72157/autocomplete.png
> Or selection color,
> http://people.mozilla.org/~nchen/builds/bug994472-fennec-31.0a1.png
> Or any color really.
> 
> On one hand the hint color is what Google's autosuggestion does. On the
> other hand, the selection color is closer to what we have right now, and it
> provides a greater contrast.

Thanks for showing options, Jim. I prefer the grey -- it's simple, clear and tasteful. 

The orange is a bit overkill, and vibrates against the already orange URL bar border.

Comment 11

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

Attachment: Use text spans to implement autocompletion (v2)

Changed to use the text color with 0.5 alpha for the autocomplete text (I thought about using the hint color, but on my Nexus 4 at least, the hint color appears too close to the text color). Using a 0.5 alpha value also ensures the autocomplete text looks good for private mode.

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 8410448 [details] [diff] [review]
Use text spans to implement autocompletion (v2)

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

I really like this and I think it helps makes this easier to read too. Thanks (and sorry for the review delay, this just makes my head spin a bit so I had to think about it a bit)!

I do think it could use some empty lines to break it up, and a little more explicit commenting, but I'm mostly worried about what happens when you hit backspace. I think we have to get that interaction right.

::: mobile/android/base/toolbar/ToolbarEditText.java
@@ +160,5 @@
> +    private static String getNonAutocompleteText(final Editable text) {
> +        final int start = text.getSpanStart(AUTOCOMPLETE_SPAN);
> +        if (start < 0) {
> +            return text.toString();
> +        }

insert some white space here. I'm not going to mark a bunch of these by hand, but I think it would help readability to split this code up a bit throughout.

@@ +170,5 @@
> +        if (start < 0) {
> +            return;
> +        }
> +        beginSettingAutocomplete();
> +        // Spans are removed automatically

This code hurts my brain, but some of these comments are a bit cryptic. Removed automatically by what? when? Maybe just:

// Remove the autocomplete text. Android will automatically remove the autocomplete spans as well.

@@ +211,5 @@
>          mAutoCompleteResult = result;
> +
> +        if (autoCompleteStart > -1) {
> +            // Autocomplete span already exists, replace existing
> +            if (!TextUtils.regionMatches(result, 0, text, 0, autoCompleteStart)) {

This line is hard for me to parse. In fact, I'm not quite sure what this does... Can you flesh out the comment a bit? i.e.

// If the new autocomplete result and the current textbox text don't match, this result is old. Ignore it.

@@ +217,5 @@
> +                return;
> +            }
> +            beginSettingAutocomplete();
> +            // Existing autocomplete spans will be preserved
> +            text.replace(autoCompleteStart, textLength, result, autoCompleteStart, resultLength);

// Otherwise, replace autocomplete with the new text. Android will maintain the spans for us.

@@ +222,5 @@
> +
> +        } else {
> +            // No autocomplete span yet, create one
> +            if (resultLength <= textLength ||
> +                    !TextUtils.regionMatches(result, 0, text, 0, textLength)) {

Don't bother wrapping this line. this check is repeated in both branches. Maybe you can move it out of the if-else. The resultLength <= textLength check is probably fine in both cases as well. It might even be worth noting to readers that the results come in asynchronously to avoid blocking the user's typing, so we're not guaranteed that they still apply.

@@ +252,5 @@
> +
> +            for (final Object span : mAutoCompleteSpans) {
> +                text.setSpan(span, textLength, resultLength, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
> +            }
> +            bringPointIntoView(resultLength);

I'm not sure we want to do this. This should scroll the end of the string into view, right? I think the composing part should always be in view, not the autocomplete part?

@@ +375,5 @@
>          setTextType(imeAction == EditorInfo.IME_ACTION_GO ?
>                      TextType.URL : TextType.SEARCH_QUERY);
>      }
>  
> +    private class SelectionChangeListener implements OnSelectionChangedListener {

I can't quite figure out when this happens. i.e. I haven't been able to get a state where I have a selection and autocomplete at the same time. Can you provide some info?

@@ +409,5 @@
> +            if (StringUtils.isSearchQuery(text, false)) {
> +                doAutocomplete = false;
> +            } else if (mAutoCompletePrefixLength > textLength) {
> +                // If you're hitting backspace (the string is getting smaller), don't autocomplete
> +                doAutocomplete = false;

One difference here with our old implementation. Hitting backspace used to delete the autocomplete text the first time you tapped it (since it was selected). Now it will start eating away at the content right away. i.e.

"supp" autocompletes to "(supp)ort.mozilla.org"
hitting backspace results in "sup" instead of "supp"
with Swype it just results in "";

The more I think about it, I think we need to address it though. Otherwise its hard for users to remove the autocomplete text.

Comment 14

[Aaron Train [:aaronmt]]

Requesting tracking since this fixes bug 990314 and may fix bug 1001415 (address-bar duplicates characters) for a variety of keyboards. A major nuisance for many.

Comment 15

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

Attachment: Use text spans to implement autocompletion (v3)

(In reply to Wesley Johnston (:wesj) from comment #13)
> Comment on attachment 8410448 [details] [diff] [review]
> Use text spans to implement autocompletion (v2)
> 
> I do think it could use some empty lines to break it up, and a little more
> explicit commenting, but I'm mostly worried about what happens when you hit
> backspace. I think we have to get that interaction right.

Thanks for the review! I tried to add more line breaks and comments. Let me know if you want more.

> @@ +222,5 @@
> > +
> > +        } else {
> > +            // No autocomplete span yet, create one
> > +            if (resultLength <= textLength ||
> > +                    !TextUtils.regionMatches(result, 0, text, 0, textLength)) {
> 
> Don't bother wrapping this line. this check is repeated in both branches.
> Maybe you can move it out of the if-else. The resultLength <= textLength
> check is probably fine in both cases as well. It might even be worth noting
> to readers that the results come in asynchronously to avoid blocking the
> user's typing, so we're not guaranteed that they still apply.

Why not wrap the line? The two checks are not the same. In the first case we're only matching prefixes between the current text and the result, whereas in this second case we're matching the whole text to the result.

> @@ +252,5 @@
> > +
> > +            for (final Object span : mAutoCompleteSpans) {
> > +                text.setSpan(span, textLength, resultLength, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
> > +            }
> > +            bringPointIntoView(resultLength);
> 
> I'm not sure we want to do this. This should scroll the end of the string
> into view, right? I think the composing part should always be in view, not
> the autocomplete part?

In practice the cursor is always in view. I added a comment to say that. The user should always know there's autocomplete text. Otherwise it's would be very unexpected if the user presses enter and go to a different page than what the user entered.

> @@ +375,5 @@
> >          setTextType(imeAction == EditorInfo.IME_ACTION_GO ?
> >                      TextType.URL : TextType.SEARCH_QUERY);
> >      }
> >  
> > +    private class SelectionChangeListener implements OnSelectionChangedListener {
> 
> I can't quite figure out when this happens. i.e. I haven't been able to get
> a state where I have a selection and autocomplete at the same time. Can you
> provide some info?

The selection can change when you reposition the cursor somewhere (by tapping or arrow keys).

> @@ +409,5 @@
> > +            if (StringUtils.isSearchQuery(text, false)) {
> > +                doAutocomplete = false;
> > +            } else if (mAutoCompletePrefixLength > textLength) {
> > +                // If you're hitting backspace (the string is getting smaller), don't autocomplete
> > +                doAutocomplete = false;
> 
> One difference here with our old implementation. Hitting backspace used to
> delete the autocomplete text the first time you tapped it (since it was
> selected). Now it will start eating away at the content right away. i.e.
> 
> "supp" autocompletes to "(supp)ort.mozilla.org"
> hitting backspace results in "sup" instead of "supp"
> with Swype it just results in "";
> 
> The more I think about it, I think we need to address it though. Otherwise
> its hard for users to remove the autocomplete text.

I have thought about this too, and I prefer the current behavior. In the new implementation, the autocomplete text is not actually selected, and you can see the cursor still blinking between the regular text and the autocomplete text. In this case you wouldn't expect backspace to delete the autocomplete text, you would expect it to delete what's before the cursor. To remove the autocomplete text, you can just tap space instead of backspace. Repositioning the cursor backwards also clears the autocomplete text. Finally, if you want to search exactly what you entered, you can tap on one of the search labels.

Comment 16

[Wesley Johnston (:wesj)]

Interesting(In reply to Jim Chen [:jchen :nchen] from comment #15)
> > One difference here with our old implementation. Hitting backspace used to
> > delete the autocomplete text the first time you tapped it (since it was
> > selected). Now it will start eating away at the content right away. i.e.
> > 
> > "supp" autocompletes to "(supp)ort.mozilla.org"
> > hitting backspace results in "sup" instead of "supp"
> > with Swype it just results in "";
> > 
> > The more I think about it, I think we need to address it though. Otherwise
> > its hard for users to remove the autocomplete text.
> 
> I have thought about this too, and I prefer the current behavior. In the new
> implementation, the autocomplete text is not actually selected, and you can
> see the cursor still blinking between the regular text and the autocomplete
> text. In this case you wouldn't expect backspace to delete the autocomplete
> text, you would expect it to delete what's before the cursor. To remove the
> autocomplete text, you can just tap space instead of backspace.
> Repositioning the cursor backwards also clears the autocomplete text.
> Finally, if you want to search exactly what you entered, you can tap on one
> of the search labels.

Hmm.. If you don't mind, I'd like UX to chime in on that too then.

Comment 17

[Ian Barlow (:ibarlow)]

It's kind of hard to understand what I'm being asked here... Mind posting a build?

Comment 18

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

Here's another build (Fennec nchen): http://people.mozilla.org/~nchen/builds/bug994472-2-fennec-31.0a1.apk

We're looking at a change in behavior when trying to get rid of the URL autocomplete text. It used to be you have to press backspace to get rid of the autocomplete text. With the new implementation, you have to press space or reposition the cursor.

Comment 19

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

Seems like there's no easy way to delete just the autocomplete text on backspace, when using the new approach. There are two things at odds here: to make autocomplete work with all keyboards, we need to make keyboards unaware of the autocomplete text; however, hiding the autocomplete text from the keyboard also means the keyboard doesn't know to delete it on backspace.

That essentially means we can support SwiftKey, Swype, etc. or we can have the previous behavior of deleting only autocomplete text on backspace, but not both :(  (Note that the current approach comes with its own baggage like bug 984057, bug 990314, and bug 1001415)

Comment 20

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

Attachment: Emulate previous backspace behavior (v1)

Kind of hacky, but this does restore the previous backspace behavior. Tested with Swype and Swiftkey.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

We can evaluate the risk of uplift after the patches land

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 8417680 [details] [diff] [review]
Use text spans to implement autocompletion (v3)

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

This looks great. Sorry for the delay jchen :(

::: mobile/android/base/toolbar/ToolbarEditText.java
@@ +43,5 @@
>  public class ToolbarEditText extends CustomEditText
>                               implements AutocompleteHandler {
>  
>      private static final String LOGTAG = "GeckoToolbarEditText";
> +    private static final NoCopySpan AUTOCOMPLETE_SPAN = new NoCopySpan.Concrete();

I probably wouldn't make this static.

@@ +156,5 @@
> +            // Span to mark the autocomplete text
> +            AUTOCOMPLETE_SPAN,
> +            // Span to change the autocomplete text color
> +            new ForegroundColorSpan(Color.argb(
> +                0x80, Color.red(textColor), Color.green(textColor), Color.blue(textColor)))

Ian wanted this to look selected. http://developer.android.com/reference/android/R.attr.html#textColorHighlight seems to provide something like the highlight color. Can we use it with a BackgroundColorSpan?

@@ +164,5 @@
> +        mAutoCompletePrefixLength = 0;
> +    }
> +
> +    /**
> +     * Get the portion of text that is not marked as autocomplete text

Rnweman will cry if you don't put periods at the end of sentences.

@@ +374,5 @@
> +    private class SelectionChangeListener implements OnSelectionChangedListener {
> +        @Override
> +        public void onSelectionChanged(final int selStart, final int selEnd) {
> +            // The user has repositioned the cursor somewhere. We need to adjust
> +            // the autocomplete text depedning on where the new cursor is.

s/depedning/depending/

@@ +384,5 @@
> +                // Do not commit autocomplete text if there is no autocomplete text
> +                // or if selection is still at start of autocomplete text
> +                return;
> +            }
> +            if (selStart <= start && selEnd <= start) {

Add a line before.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 8418916 [details] [diff] [review]
Emulate previous backspace behavior (v1)

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

Nice. I don't mind a hack like this.

::: mobile/android/base/toolbar/ToolbarEditText.java
@@ +24,5 @@
>  import android.text.Selection;
>  import android.text.Spanned;
>  import android.text.TextUtils;
>  import android.text.TextWatcher;
> +import android.text.style.BackgroundColorSpan;

Oh you did this here :) yay!

Comment 24

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

Addressed review comments. Thanks Wes!

https://hg.mozilla.org/integration/fx-team/rev/7c8ba49235f2
https://hg.mozilla.org/integration/fx-team/rev/80f8cb8918bf

Comment 25

[u428464]

I hope this will be uplifted asap. ;-) It's a pretty annoying on S4 devices.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c8ba49235f2
https://hg.mozilla.org/mozilla-central/rev/80f8cb8918bf

Comment 27

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

If we don't see big regressions, we will want to uplift this to fix bugs with some keyboards.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes for 32

Comment 29

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

Since we're less than a week from the next merge, I think we should just let this bug ride the train.