Mirror desktop URL autocompletion and force capitalisation from history for the path part

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
29 days ago

People

(Reporter: JanH, Assigned: mehdisolamannejad, Mentored)

Tracking

Trunk
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Bug 1349797 made our URL bar autocompletion case-insensitive.
For domains this is fine, since those are supposed to be case-insensitive anyway, however paths most of the time aren't.

I've noticed that Desktop handles this by allowing arbitrary capitalisation within the domain part, but forcing the capitalisation as suggested by the autocompletion once the user starts typing within the path portion.

Meaning e.g. if I'm typing buGziLla.mozilla.org/sho (which autocompletes to show_bug.cgi), then no matter whether I'm typing a "W" or a "w", a "w" will be entered into the URL bar.
(Assignee)

Comment 1

2 years ago
Jan, I would like to work on this bug. Would please also mentor me on this one?

Also, I noticed that Desktop doesn't show any suggestions for 'buGziLla.mozilla.org/S'. Don't know if it also is a bug or not, but just wanted to know if we are supposed to imitate the Desktop behavior 100% or not.
(Assignee)

Comment 2

2 years ago
(In reply to mehdisolamannejad from comment #1)
> Also, I noticed that Desktop doesn't show any suggestions for
> 'buGziLla.mozilla.org/S'. Don't know if it also is a bug or not, but just
> wanted to know if we are supposed to imitate the Desktop behavior 100% or
> not.

Nevermind. if something with a capital S is not in history autocompletion should halt.
(Reporter)

Comment 3

2 years ago
(In reply to mehdisolamannejad from comment #2)
> (In reply to mehdisolamannejad from comment #1)
> > Also, I noticed that Desktop doesn't show any suggestions for
> > 'buGziLla.mozilla.org/S'. Don't know if it also is a bug or not, but just
> > wanted to know if we are supposed to imitate the Desktop behavior 100% or
> > not.
> 
> Nevermind. if something with a capital S is not in history autocompletion
> should halt.

Yup, Desktop does this only if some autocompleted text is already present in the address bar. If you've typed "buGziLla.mozilla.org/", it won't yet autocomplete a path part, so for the first letter you type it cannot force any capitalisation.

That aside, sure, feel free to work on this. I haven't really looked at this very deeply, but at a first glance our afterTextChanged listener in ToolbarEditText would probably be a good place to start experimenting.
Assignee: nobody → mehdisolamannejad
Mentor: jh+bugzilla
(Reporter)

Updated

2 years ago
Depends on: 1349797
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 6

2 years ago
Sorry for the slight delay - I had started looking at your patch yesterday and was going to write up my comments today, but something else urgent intervened. In any case the basic idea of yours is fine, but there are some implementation details that need tweaking.
(Assignee)

Comment 7

2 years ago
(In reply to Jan Henning [:JanH] from comment #6)
> Sorry for the slight delay 

It's OK.

(In reply to Jan Henning [:JanH] from comment #6)
> but something else urgent intervened

I hope you're doing fine.

(In reply to Jan Henning [:JanH] from comment #6)
> but there are some implementation details 
> that need tweaking.

Sure. Please point them out and I'll work on them.
(Reporter)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8895001 [details]
Bug 1388112 - Force capitalisation from autocorrection result.

https://reviewboard.mozilla.org/r/166130/#review173086

So my comment about `afterTextChanged` being a good starting point turned out to be a red herring (I did say I hadn't looked very closely :-) ), but I see you've noticed that yourself.
The basic logic (use the autocomplete result's capitalisation if we've already got some autocomplete text showing, else partially revert the previous patch and do a case sensitive check for the path segment after all if we haven't got any matching autocomplete yet) is fine.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:71
(Diff revision 2)
>      // Spans used for marking the autocomplete text
>      private Object[] mAutoCompleteSpans;
>      // Do not process autocomplete result
>      private boolean mDiscardAutoCompleteResult;
> +    // if path segment of URL has to be corrected (since they are case-sensitive)
> +    private boolean mPathSegmentCorrection = false;

Is this actually necessary? When the user hits backspace, the autocomplete text is removed, so the next call of onAutocomplete will go through the "else" code path (which doesn't do any case manipulation) anyway.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:326
(Diff revision 2)
>              }
>  
>              beginSettingAutocomplete();
>  
> +            // Check if user has started to type the path segment of the URL
> +            final boolean pathStarted = userText.contains("/");

This doesn't work if the user is also typing the protocol.

Since you'll need it anyway for the other comments, you can add a new helper method in `StringUtils` that looks for the presence of "://" and skips it if necessary before looking for a "/" and returning either the start of the path segment or -1 if the string doesn't have any.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:330
(Diff revision 2)
> +            // Check if user has started to type the path segment of the URL
> +            final boolean pathStarted = userText.contains("/");
> +
> +            // Should we force capitalisation from result
> +            if (pathStarted && mPathSegmentCorrection) {
> +                text.replace(autoCompleteStart - 1, autoCompleteStart, result, autoCompleteStart - 1, autoCompleteStart);

Don't forget that you're dealing with an IME here, so there's no guarantee here that only one character has changed [1]. You always need to replace the whole user-typed path segment with the autocompletion result.

[1] Some keyboards (e.g. Gboard) don't offer input suggestions for URL input, so maybe try a different keyboard app to see this for yourself.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:349
(Diff revision 2)
>          } else {
>              // No autocomplete text yet; we should add autocomplete text
>  
> +            // If the user is typing the path segment, we should match
> +            // the path segment of the result, case-sensitively.
> +            final String userText = text.toString();

Not necessary, just use the equivalent `TextUtils` methods.

(But irrelevant should you follow my comment about the early return below)

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:350
(Diff revision 2)
>              // No autocomplete text yet; we should add autocomplete text
>  
> +            // If the user is typing the path segment, we should match
> +            // the path segment of the result, case-sensitively.
> +            final String userText = text.toString();
> +            final int pathStart = userText.indexOf('/'), resultPathStart = result.indexOf('/');

You already calculate `userHasPath` respectively `pathStarted` in both code paths and the same will be true for `pathStart` if you address the comment re `text.replace()` above, so just move this to the common part above this if else block.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:363
(Diff revision 2)
> +
>              // If the result prefix doesn't match the current text,
>              // the result is stale and we should wait for the another result to come in.
>              if (resultLength <= textLength ||
> -                    !StringUtils.caseInsensitiveStartsWith(result, text.toString())) {
> +                    !StringUtils.caseInsensitiveStartsWith(resultDomain, userDomain) ||
> +                    !resultPath.startsWith(userPath)) {

If you leave this early return alone and do an extra check afterwards, the domain part might indeed end up being checked twice (once case insensitively, once case sensitively), but assuming that path autocompletion is less common than domain autocompletion this shouldn't hurt too much and allow the code to be simplified quite a bit:

After the exisiting check, you already know that the two strings are the same except possibly their capitalisation, so you don't need to calculate `resultPathStart`/`resultHasPath` separately - they'll be the same value as for the user-entered text, since slashed aren't case sensitive. You no longer need to compare the domain parts because you already checked them case-insensitively. And instead of manually getting the substrings for the paths and comparing them, you can just use TextUtils.regionMatches (remember it from your previous patch, where you had to do the reverse and convert it to `startsWith` for the case-insensitive comparison?) with an appropiate offset and length so you're just comparing the path segments.
Attachment #8895001 - Flags: review?(jh+bugzilla)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

2 years ago
(In reply to Jan Henning [:JanH] from comment #8)
> If you leave this early return alone and do an extra check afterwards, the
> domain part might indeed end up being checked twice (once case
> insensitively, once case sensitively)

If this caused any confusion, sorry - I meant to say that the path part might end up being checked twice.
(Reporter)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8895001 [details]
Bug 1388112 - Force capitalisation from autocorrection result.

https://reviewboard.mozilla.org/r/166130/#review174120

With some more tweaks it's fine - but please test your patch to make sure it actually works. As it stands, it can cause a crash pretty soon after typing in the URL bar.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:309
(Diff revision 3)
>  
>          final Editable text = getText();
>          final int textLength = text.length();
>          final int resultLength = result.length();
>          final int autoCompleteStart = text.getSpanStart(AUTOCOMPLETE_SPAN);
> +        final int pathStart = StringUtils.pathStartIndex(text.toString());

Use `getNonAutocompleteText(text)` for getting the text here, otherwise you'll detect the slash included in the autocomplete result as a user typed path start and start crashing.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:326
(Diff revision 3)
>  
>              beginSettingAutocomplete();
>  
> +            // Should we force capitalisation from result
> +            if (pathStart != -1) {
> +                text.replace(pathStart + 1, autoCompleteStart, result, pathStart + 1, autoCompleteStart);

You're using pathStart + 1 here, but just pathStart below in the other case, i.e. you're excluding the slash in one case and including it in the other case. Since there's no reason for this difference, just decide on one behaviour and use it in both places, so as to avoid confusion later on.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:343
(Diff revision 3)
>              endSettingAutocomplete();
>  
>          } else {
>              // No autocomplete text yet; we should add autocomplete text
>  
> +            final String userText = text.toString();

The string conversion isn't necessary - you can just leave the call to caseInsensitiveStartsWith as it is and `TextUtils.regionMatches` handles Editables just fine.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:353
(Diff revision 3)
> -                    !StringUtils.caseInsensitiveStartsWith(result, text.toString())) {
> +                    !StringUtils.caseInsensitiveStartsWith(result, userText)) {
> +                return;
> +            }
> +
> +            // Now that we know the result and the usertext are the same case-insensitively,
> +            // we should check whether their path parts match

... whether their path parts match _case-sensitively_, to make the comment more explicit.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarEditText.java:355
(Diff revision 3)
> +            }
> +
> +            // Now that we know the result and the usertext are the same case-insensitively,
> +            // we should check whether their path parts match
> +            if (pathStart != -1 &&
> +                    !TextUtils.regionMatches(result, pathStart, userText, pathStart, userText.length() - pathStart)) {

You can use `textLength` from further up instead of userText.length().

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/StringUtils.java:293
(Diff revision 3)
>       * Case-insensitive version of {@link String#startsWith(String, int)}.
>       */
>      public static boolean caseInsensitiveStartsWith(String text, String prefix, int start) {
>          return text.regionMatches(true, start, prefix, 0, prefix.length());
>      }
> +

There are some other URL-related functions further up in this file - group this function together with them so it doesn't remain lonely :-)

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/StringUtils.java:295
(Diff revision 3)
>      public static boolean caseInsensitiveStartsWith(String text, String prefix, int start) {
>          return text.regionMatches(true, start, prefix, 0, prefix.length());
>      }
> +
> +    /**
> +     * Returns the index of the path segment of URLs

Small reminder: There's special javadoc syntax for return values, so you could write this as `@return The index of the path segment of URLs.`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/StringUtils.java:297
(Diff revision 3)
>      }
> +
> +    /**
> +     * Returns the index of the path segment of URLs
> +     */
> +    public static int pathStartIndex(String text) {

Can you also add a few tests for this in [https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestStringUtils.java](https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestStringUtils.java)?
Attachment #8895001 - Flags: review?(jh+bugzilla)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
I'm sorry about the slight delay and the crash. All of the autocompletions I got in my own testings didn't include the slash. Thank you for pointing it out.
Comment hidden (mozreview-request)
(Reporter)

Comment 15

2 years ago
(In reply to mehdisolamannejad from comment #13)
> I'm sorry about the slight delay and the crash. All of the autocompletions I
> got in my own testings didn't include the slash. Thank you for pointing it
> out.

Fair enough, in that case it's indeed not immediately obvious to spot.
(Reporter)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8895001 [details]
Bug 1388112 - Force capitalisation from autocorrection result.

https://reviewboard.mozilla.org/r/166130/#review175636

If the [test run](https://treeherder.mozilla.org/#/jobs?repo=try&revision=210c6737e1f5) doesn't show any problems, this looks fine.
Attachment #8895001 - Flags: review?(jh+bugzilla) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/49a9e5edc60d
Force capitalisation from autocorrection result. r=JanH
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49a9e5edc60d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Updated

29 days ago
Depends on: 1528715
You need to log in before you can comment on or make changes to this bug.