Autofill sometimes autocompletes "in the middle" (with ">>")

VERIFIED FIXED in Firefox 62

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: mak, Assigned: mak)

Tracking

({regression})

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 unaffected, firefox62 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

Assignee

Description

a year ago
Bug found by Mossop on IRC.

Typing "github/" autocompletes to "github/ >> github.com", that's incorrect, we should never return something that may be autocompleted "in the middle" (with >>).
"github/" should resolve to a local "github/" host.
Assignee

Updated

a year ago
Flags: qe-verify?

Updated

a year ago
QA Contact: gwimberly
Assignee

Comment 1

a year ago
This appears to be caused by trimTrailingSlashIfOnlySlash. basically we fixup "github/" to "github", find "github.com" and try to complete it, but the found result doesn't contain the original string, and thus legacy middle completion kicks in. We always tried to avoid it because it looks odd and confusing.
I'm not sure why we're doing this, I think it was a misread of the original code, that was running hostQuery only if the string DID NOT contain a slash, and urlQuery if a slash was at the end.
Assignee

Updated

a year ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED

Updated

a year ago
Flags: qe-verify? → qe-verify+
Assignee

Updated

a year ago
Blocks: 1464328
Assignee

Updated

a year ago
Duplicate of this bug: 1465147
Assignee

Updated

a year ago
Summary: Autofill sometimes autocompletes "in the middle" → Autofill sometimes autocompletes "in the middle" (with ">>")
There are a couple of issues with this when I test it, one not-so-serious and the other more serious.  I started in a new profile and then visited www.nytimes.com/section/us

Then I started typing nytimes.com/section/us character by character in these STR:

(1)
Type: nytimes.com
Autofilled: nytimes.com/
Heuristic result: (nytimes favicon) https://www.nytimes.com -- Visit

(2)
Type: nytimes.com/
Autofilled: (nothing)
Heuristic result: (default globe favicon) https://nytimes.com -- Visit

(3)
Type: nytimes.com/section
Autofilled: nytimes.com/section/us
Heuristic result: (nytimes favicon) https://www.nytimes.com/section/us -- Visit

(4)
Type: nytimes.com/section/
Autofilled: nytimes.com/section/us
Heuristic result: (default globe favicon) https://nytimes.com/section/ -- Visit

(5)
Type: nytimes.com/section/us
Autofilled: (nothing)
Heuristic result: (nytimes favicon) https://www.nytimes.com/section/us -- Visit

The not-so-serious thing is that whenever you type a slash, the heuristic result changes from autofill to visiturl.  Visually, the favicon changes and the "www." disappears.  When you type the next char, it goes back to an autofill result.  Without the patch, the heuristic result remains an autofill result the whole time with no jarring visual changes.

The seriouser thing is in step 4.  nytimes.com/section/us is autofilled (with trailing "us"), but the heuristic result shows https://www.nytimes.com/section/ (no "us").  If you hit enter at this point, you go to https://www.nytimes.com/section/.

Can we fix this without these two issues?  Would it be sufficient to modify trimTrailingSlashIfOnlySlash() to check for a dot like how looksLikeUrl() does?
Assignee

Comment 5

a year ago
hm, I wonder if the patch in bug 1464328 solves the latter problem, and eventually I can just merge these 2 patches.
Comment hidden (mozreview-request)
Assignee

Comment 7

a year ago
The good thing is that I could restore all the tests to the status-quo and just add new ones!
I'm still looking at the new revision, but there are similar problems with domains with at least three dots.  looksLikeUrl ends up returning true for those domains.  (That function seems just broken to me, or at least underspecified.)  I don't know whether that's the cause, but I'm guessing it's part of it because domains with only two dots work OK.

Similar STR as before, but this time I'm using www.nintendo.co.jp:

(1)
Type: www.nintendo.co
Autofilled: www.nintendo.co.jp/
Heuristic result: (Nintendo favicon) https://www.nintendo.co.jp -- Visit

(2)
Type: www.nintendo.co.
Autofilled: www.nintendo.co.jp/
Heuristic result: (default globe favicon) https://www.nintendo.co./ -- Visit

(3)
Type: www.nintendo.co.j
Autofilled: www.nintendo.co.jp/
Heuristic result: (default globe favicon) https://www.nintendo.co.j/ -- Visit

(4)
Type: www.nintendo.co.jp
Autofilled: www.nintendo.co.jp/
Heuristic result: (Nintendo favicon) https://www.nintendo.co.jp -- Visit

In steps 2 and 3, the result flips back to a visiturl and the autofilled text doesn't match the result's URL.
When I replace that looksLikeUrl call with a trimTrailingSlashIfOnlySlash call, those problems go away.  I think looking for a slash, at least in that case, is the right thing to do since that's what distinguishes "origins" from URLs and so whether we should use the origin query or the URL query.

Oh and now that I think about it, looksLikeUrl returns true for origins that contain ports (":") or userinfos ("@"), right?  We want to use the origin query in those cases.
(In reply to Drew Willcoxon :adw from comment #9)
> Oh and now that I think about it, looksLikeUrl returns true for origins that
> contain ports (":") or userinfos ("@"), right?

for *strings* that contain ":" or "@", I mean
Assignee

Comment 11

a year ago
Yeah, but also trimTrailingSlashIfOnlySlash is not correct, since it causes this bug. Anyway, I'll have to restore trimTrailingSlashIfOnlySlash and start from it, I was hoping to have a single helper.
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8981469 [details]
Bug 1463580 - Autofill sometimes autocompletes in-the-middle (with >>) and doesn't autofill to the next slash.

https://reviewboard.mozilla.org/r/247568/#review254258

Thanks!

::: toolkit/components/places/UnifiedComplete.js:1461
(Diff revision 3)
>      let gotResult = false;
>  
> -    // If search string has a slash in it, then treat it as a possible URL and
> +    // If search string looks like a url, then treat it as a possible URL and
>      // try to autofill against URLs.  Otherwise treat it as a possible origin
>      // and try to autofill against origins.  One exception:  When the string has
> -    // only one slash and it's at the end, treat it as a possible origin, not a
> +    // only one slash and it's at the end, don't search.

"don't search" isn't accurate anymore, right?  And with the new looksLikeOrigin, this comment should be inverted ("If the search string looks like an origin, ...  Otherwise assume it might be a URL...")

::: toolkit/components/places/UnifiedComplete.js:1469
(Diff revision 3)
>        [query, params] = this._originQuery;
>      } else {
>        [query, params] = this._urlQuery;
>      }
>  
> +    // urlQuery doesn't always return a query.

Nit: _urlQuery (with underscore)

::: toolkit/components/places/UnifiedComplete.js:1547
(Diff revision 3)
> -    // PlacesSearchAutocompleteProvider only matches against engine domains.  If
> -    // the search string (without the prefix) contains multiple slashes, or a
> -    // single slash that's not at the end, don't try to match.
> -    let searchStr = trimTrailingSlashIfOnlySlash(this._searchString);
> -    if (!searchStr) {
> +    // PlacesSearchAutocompleteProvider only matches against engine domains.
> +    // Remove an eventual trailing slash from the search string (without the
> +    // prefix) and check if the resulting string is worth matching.
> +    // Later, we'll verify that the found result matches the original
> +    // searchString and eventually discard it.
> +    let searchString = this._searchString;

Nit: Keep this var named `searchStr` to preserve slightly more blame? :-)
Attachment #8981469 - Flags: review?(adw) → review+
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/646b7e8eb69b
Autofill sometimes autocompletes in-the-middle (with >>) and doesn't autofill to the next slash. r=adw

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/646b7e8eb69b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 17

a year ago
Going to take a look at this bug tomorrow.
Flags: needinfo?(gwimberly)

Comment 18

a year ago
Verified on the newest Nightly (62.0a1) 06-07-2018 with the following OS: Windows 10 x64, Mac OSX 10.11, and Ubuntu 18.10 x64
Version: 62.0a1
Build ID: 20180607100059
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(gwimberly)
You need to log in before you can comment on or make changes to this bug.