720081 - Inline autocomplete drops https and completes without protocol/www.

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Ed Lee :Mardak]

With bug 566489, my first result for "f" is https://www.facebook.com but inline autocomplete ends up loading "facebook.com/"

Comment 1

[Ed Lee :Mardak]

Jesse, how do you set the level of [sg:?]?

Comment 2

[Marco Bonardo [:mak]]

the result of inline is not directly related to the first entry in the ac popup, and unfortunately it ignores the protocol (like the normal ac does)

Comment 3

[Marco Bonardo [:mak]]

and yes, it also strips www., like normal ac

Comment 4

[Michael Lefevre]

(In reply to Edward Lee :Mardak from comment #1)
> Jesse, how do you set the level of [sg:?]?

You don't any more - you take your best guess and let it get changed later if necessary - https://groups.google.com/d/msg/mozilla.dev.planning/yeqEfA1Tk-4/mrqYRIOxctIJ . I've taken a guess, feel free to change...

Comment 5

[Jesse Ruderman]

This was a problem before inline autocomplete as well. See bug 658707.

Comment 6

[Daniel C]

I thought fixing bug 566489 meant that instead of doing:

Type a letter.
Press the down key.
Press enter.

(Something I do for with quite a few letters for my most common sites).

I could instead do:

Type a letter.
Press enter.

For the letter b, this doesn't work. Usually, I type b, press down, and press enter. This goes to www.b........com

Now, if I type b, the first result in the dropdown is the same (my intended destination) but the location bar is prefilled with b........com

Pressing enter goes to b........com (omitting the www) and for this particular site, that doesn't work (leading to an error page).

So it doesn't save me any keystrokes and just leaves me confused as to how that URL was suggested (looking in places.sqlite shows that I've visited the www version 3000+ times and the non-www version 7 times (always by mistake).

Comment 7

[Daniel C]

To add to that, the moz_inputhistory table backs that up strongly.

After a brief test of Chromium, it doesn't appear to suffer from the same problem.

Comment 8

[Marco Bonardo [:mak]]

Daniel, how is that related to this bug at all?

Btw, we may fix the https issue collecting proper data, I think there's a similar issue for other protocols like ftp (a ftp://ftp.moz.com would be autocompleted to http). We can add a scheme column to the inline ac table and have a sort of priority in schemas, like ftp,https,http.

Comment 9

[Daniel C]

(In reply to Marco Bonardo [:mak] from comment #8)
> Daniel, how is that related to this bug at all?

Because you can't just drop the www and expect things to work.

Here are some concrete STR. Start with a clean profile in each case.

1. Type www.adhgems.co.uk into the location bar and press Enter.
2. Open a new tab.
3. Close the first tab.
4. Type 'a' into the location bar (autofills to adhgems.co.uk).
5. Press enter.

After step 5 in Fx I see an error page. In Chromium the correct page is displayed.

Do you get the same results?

Comment 10

[Marco Bonardo [:mak]]

(In reply to Daniel Cater from comment #9)
> (In reply to Marco Bonardo [:mak] from comment #8)
> > Daniel, how is that related to this bug at all?
> 
> Because you can't just drop the www and expect things to work.

Right, sorry I didn't read correctly your first comment.

Comment 11

[Daniel C]

I think that the inline autocomplete feature will cause quite a few complaints without this fix.

It essentially means that inline autocomplete is suggesting a site that:

a) You've never been to
b) Won't load (will show the error page) if the site isn't configured to redirect to the www version.

I have hit at least 3 such sites, which means I have to refocus the location bar and explicitly type the www. I suspect there are hundreds of thousands of such sites.

Comment 12

[Daniel C]

Steps to reproduce in comment 9. Also try with www.bacsport.co.uk, www.squirrelmail.ucl.ac.uk.

Comment 13

[Alex Keybl [:akeybl]]

(In reply to Daniel Cater from comment #11)
> I think that the inline autocomplete feature will cause quite a few
> complaints without this fix.
> 
> It essentially means that inline autocomplete is suggesting a site that:
> 
> a) You've never been to
> b) Won't load (will show the error page) if the site isn't configured to
> redirect to the www version.

Given the above, we can expect this to be a jarring user experience. Tracking for FF13.

Comment 14

[Alex Keybl [:akeybl]]

Sending over to Marco, since this bug is now tracked for release and requires an assignee.

Comment 15

[Marco Bonardo [:mak]]

Fixing this requires some large changes, probably including API changes, don't think it can make Firefox 13, maybe not even 14. Btw, will look into it.
If this is such a big deal, we should flip the pref off again.

Comment 16

[Luke Wagner [:luke]]

I love inline autocomplete, but after several weeks I still keep getting bit by this (in particular, for https://mail.mozilla.com).

Comment 17

[Marco Bonardo [:mak]]

Attachment: Part 1: controller should complete typeAhead differently

So there are 2 things we need to fix this:
1. The controller should be able to finally complete to a different value than the defaultComplete one (cause while the user types it should not replace text, but on confirmation it has to)
2. Places should have prefix data to feed the result

This patch is the 1. implementation I used to implement and test 2, it works, but doesn't look like the cleanest approach.  I'm basically handling typeAhead results so that the comment can be used as the final value, if provided, otherwise the common path is executed.

Before cleaning it up, adding comments and so on I'd like to gather some feedback and even alternative ideas to obtain a similar result.  I mostly went this way cause we may still be able to backport this bug to Aurora and keep the feature enabled, but that requires not breaking interfaces.

Comment 18

[Marco Bonardo [:mak]]

hm, after a second look at what I did in Places, looks like this won't ever be backportable to FF13, cause I have a schema migration in the middle that happened in FF14 and is not easily backportable (well we may hack up something like repeating v20 in v22 but it's really ugly).  So nevermind the backport thoughts, this may at a maximum make 14 :(

Comment 19

[Marco Bonardo [:mak]]

Attachment: Part 2: Places prefix data

this may be considered ready for review, provided it may change based on how we handle the controller requirement.

Comment 20

[Marco Bonardo [:mak]]

reminder: sqlite substr indices are 1-based, need to correct.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Adjusting tracking for 14 based on comment 18.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 616940 [details] [diff] [review]
Part 1: controller should complete typeAhead differently

This is a bit of a gross abuse of the existing nsIAutocompleteResult API, but we have a bunch of those already :/

It seems like you could probably factor out some common code (finding/validating the right result object) from GetFinalDefaultCompleteValue/GetDefaultCompleteValue.

The magic in GetFinalDefaultCompleteValue certainly needs more comments (why are you checking the "comment" field, etc.).

I could live with this as a short-termed backport-friendly hack to avoid changing nsIAutocompleteResult.

Comment 23

[Marco Bonardo [:mak]]

Attachment: Part 1: controller should complete typeAhead differently

This is the updated controller patch, addressing comments.

I didn't make a specific test for this cause the approach is known to change, thus I don't want to give false code examples which add-ons may rely on, though it will be tested through Places inline tests.  Places inline ac is the only expected user for now (I added some scary comment to ask to not rely on this behavior for now) until we fix bug 754265 and make a proper API, that will need an automated test.

I'm still looking at the Places part 2, cause I just found a bug for which I have to add some test and a more efficient query for which I have to figure out its trigger shape.  Btw I'll likely get review from dietrich on that part, so we can balance reviews load.

Comment 24

[Marco Bonardo [:mak]]

Attachment: Part 2: Add and use prefix data to Places moz_hosts

warning: black magic inside :)
Well, not exactly, but there's some fancy SQL to have a prefixes priority, so that secure connections are always preferred.  I added a test case for each edge-case I could think of.  After all the patch is smaller than I thought.
Note for the review: this is intended to be backported to Aurora, along with Part 1.

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 623279 [details] [diff] [review]
Part 2: Add and use prefix data to Places moz_hosts

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

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +1410,5 @@
>      // Domains have no "/" in them.
>      let lastSlashIndex = this._currentSearchString.lastIndexOf("/");
>      if (lastSlashIndex == -1) {
>        var hasDomainResult = false;
> +      var domain, untrimmed;

untrimmed what? should name the thing as well.

::: toolkit/components/places/nsPlacesTriggers.h
@@ +78,5 @@
>      "WHERE id = OLD.place_id;" \
>    "END" \
>  )
>  
> +#define HOSTS_PREFIX_PRIORITY_FRAGMENT \

can you add a nice big comment about what's happening in this fragment?

@@ +82,5 @@
> +#define HOSTS_PREFIX_PRIORITY_FRAGMENT \
> +  "SELECT CASE " \
> +    "WHEN EXISTS( " \
> +      "SELECT 1 FROM moz_places WHERE url BETWEEN 'https://www.' || host || '/' " \
> +                                             "AND 'https://www.' || host || '/' || X'FFFF' " \

can you explain what's going on here?

Comment 26

[Marco Bonardo [:mak]]

Attachment: Part 2: Add and use prefix data to Places moz_hosts

Added the comment and clarified the var.
To properly read the query, remember that || is the equivalent of + when concatenating strings, while || X'FFFF' just means "append the largest char"
so we search any page between https://www.mozilla.org/ and https://www.mozilla.org/MAXCHAR

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 627227 [details] [diff] [review]
Part 2: Add and use prefix data to Places moz_hosts

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

thanks!

Comment 28

[Marco Bonardo [:mak]]

Gavin, any possibility to review the controller part soonish? the merge is behind the corner

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 623145 [details] [diff] [review]
Part 1: controller should complete typeAhead differently

sorry for the review delay! The GetFinalDefaultCompleteValue comment should probably mention that it only returns a result if the value is case-insensitively the same as the input text. I'm guessing this is to cover situations like e.g. pressing backspace before pressing enter when inline autocompletion is enabled?

We could probably inline GetDefaultCompleteValue() into CompleteDefaultIndex() now, since it has only one caller, but no need to do that here.

Comment 30

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/59409e2655ca
https://hg.mozilla.org/mozilla-central/rev/960b80d99b4a

Comment 31

[Marco Bonardo [:mak]]

Comment on attachment 627227 [details] [diff] [review]
Part 2: Add and use prefix data to Places moz_hosts

[Approval Request Comment]
Bug caused by (feature/regressing bug #): No regression, this is the last blocker to keep inline autocomplete feature enabled in a release.
User impact if declined: We have to disable again the feature in beta, so no inline autocomplete in Firefox 14.
Testing completed (on m-c, etc.): inline is active in Nightly and Aurora from version 12 and got QA testing. This patch has automated tests and manual testing.
Risk to taking this patch (and alternatives if risky): part 1 is safe and really specific to inline, so shouldn't have any negative impact. part 2 has downside that can't be backed out, cause it bumps the Places database version. The feature can still be disabled through the usual pref.
String or UUID changes made by this patch: none

PS: the approval is actually intended for both patches in this bug, not flagging both though since would be useless bugspam.

Comment 32

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cff98981add8
https://hg.mozilla.org/releases/mozilla-aurora/rev/99805da33911

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Backed out the autocomplete controller changes because of bug 760803.
https://hg.mozilla.org/mozilla-central/rev/9274e6b53af4
https://hg.mozilla.org/releases/mozilla-aurora/rev/6db7fbab6f0c

Comment 34

[Marco Bonardo [:mak]]

hm unfortunately the tests are now unhappy (permaorange) cause some of them were relying on the proper inline behavior.
I think the problem in the patch was just that the typeAhead if condition doesn't check anymore nsCaseInsensitiveStringComparator and always applies its result, just inverting the 2 conditions should fix any problem. Will verify that.

Comment 35

[Marco Bonardo [:mak]]

basically here https://hg.mozilla.org/mozilla-central/rev/59409e2655ca#l1.167

Comment 36

[:Gavin Sharp [email: [e-mail]]]

OK, I also backed out part of m-c revision 960b80d99b4a (pieces of "Part 2"/attachment 627227 [details] [diff] [review]), which depends on the autocomplete controller changes:

https://hg.mozilla.org/mozilla-central/rev/82d895d433e9
https://hg.mozilla.org/releases/mozilla-aurora/rev/32787d343859

Comment 37

[Marco Bonardo [:mak]]

Thank you Gavin!

Comment 38

[Marco Bonardo [:mak]]

Attachment: Part 1 update interdiff

This is the interdiff of the changes I made to part 1:
1. added 2 new ac tests to ensure we don't regress as we did and the typeAhead special behavior
2. reverted the change to handleNavigation, we should not use the "final" value there, cause the user is still editing the value we should not replace it
3. fixed a bug I introduced in 2009 in EnterMatch, if there is a popup selection we should never try to defaultComplete (bogus if/elseif condition)
4. Inverted the checks in getFinalDefaultCompleteValue, so we first sanity check and then replace if needed

Will also attach the merged patch.

Comment 39

[Marco Bonardo [:mak]]

Attachment: Part 1 updated: controller should handle typeAhead differently

Merged patch

Comment 40

[Marco Bonardo [:mak]]

Attachment: Part 1 updated: controller should handle typeAhead differently

A Places test found another small glitch, if the comment was empty GetFinalDefaultCompleteValue was overwriting the _retval with an empty string, now using a temp autostring.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Just want to make sure I understand all the changes. Looking at the interdiff, there are three major changes:

1) If there is a valid selectedIndex, we should never complete from the default index (changes to nsAutoCompleteController::EnterMatch)
2) If we're completing from the default index, only do it if the input value matches the default index value (case insensitively). Previous patch failed to do this if there was an "override" final default value. (changes to GetFinalDefaultCompleteValue) 
3) Don't use the final default value in HandleKeyNavigation

It was a combination of 1) and 2) that caused bug 760803, right? Fixing only 2) would have been sufficient, but we might as well also fix 1) to avoid even attempting to complete the default index when it doesn't make sense.

3) looks like an unrelated fix, but seems reasonable I guess. If you're trying to edit a completed value it would be weird for it to suddenly change when you press e.g. "Home". On the other hand, if you press right arrow at the end of a completion, we might want to show the actual value that will be used if you were to press "Enter"? Maybe this is a case where we should handle different navigation keys differently? Not a big deal either way, I don't think we need to over-rotate on this here.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 629599 [details] [diff] [review]
Part 1 update interdiff

>diff --git a/toolkit/components/autocomplete/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/nsAutoCompleteController.cpp

> nsAutoCompleteController::GetFinalDefaultCompleteValue(nsAString &_retval)

>   if (NS_SUCCEEDED(result->GetTypeAheadResult(&isTypeAheadResult)) &&
>       isTypeAheadResult &&
>       NS_SUCCEEDED(result->GetCommentAt(defaultIndex, _retval)) &&
>       !_retval.IsEmpty()) {
>     return NS_OK;
>   }
> 
>   return NS_OK;

I was going to comment that this didn't look right, but it looks like you fixed this in the actual patch (I guess this interdiff is against an old version? ah yeah, just saw comment 40)

Looking at a diff of 59409e2655ca and m-c+attachment 629603 [details] [diff] [review], one other thing stands out (aside from the things mentioned in comment 41):

-   * Gets the defaultComplete value to be used when the user navigates or
-   * confirms the current match.
-   * The value is returned only if it case-insensitively matches the current
-   * input text, otherwise the method returns NS_ERROR_FAILURE.
-   * This happens because we don't want to override user casing in case of a
-   * navigation key (unless the text is the same), or to replace text if the
-   * user backspaces just before Enter.
+   * Gets the defaultComplete value to be used when the user confirms the
+   * current match.

The old comment seems more complete and still accurate, why did you decide to change this?

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 629603 [details] [diff] [review]
Part 1 updated: controller should handle typeAhead differently

r=me with the comment change from comment 42 explained.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 629603 [details] [diff] [review]
Part 1 updated: controller should handle typeAhead differently

[Triage Comment]
Don't forget to also re-land the tests backed out in comment 36

Comment 45

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #41)
> It was a combination of 1) and 2) that caused bug 760803, right?

Right

> Fixing only
> 2) would have been sufficient, but we might as well also fix 1) to avoid
> even attempting to complete the default index when it doesn't make sense.

Yes, my test catched that wrong path

> 3) looks like an unrelated fix, but seems reasonable I guess. If you're
> trying to edit a completed value it would be weird for it to suddenly change
> when you press e.g. "Home". On the other hand, if you press right arrow at
> the end of a completion, we might want to show the actual value that will be
> used if you were to press "Enter"? Maybe this is a case where we should
> handle different navigation keys differently? Not a big deal either way, I
> don't think we need to over-rotate on this here.

Well, I tried that, the problem is that the test jumps right and looks quite jumpy.

Comment 46

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #42)
> The old comment seems more complete and still accurate, why did you decide
> to change this?

not expected, I originally started changing this method, then figured out was the wrong way to fix this... so I may have touched the comment. Will verify the interdiff with the previous landing.

Comment 47

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [:mak] from comment #45)
> Well, I tried that, the problem is that the test jumps right and looks quite
> jumpy.

I meant the TEXT (in the input field) looks jumpy

Comment 48

[Marco Bonardo [:mak]]

Attachment: Part 1 updated: controller should handle typeAhead differently

fixed the comment

Comment 49

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/dd6ec482a85d
will push to Aurora once I get a green on m-c and a completely new clobber to test again. Thank you for everything, from backout to the review, very much appreciated.

Comment 50

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1851b8c13061

Comment 51

[Vlad [QA]]

I've verified this using:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120604 Firefox/14.0a2

and after using the steps from comment9, after I type "a" in the url bar, it gets filled with "adhgems.co.uk/".
Pressing the enter key, the site is loaded with "www".

I've also tested this using Aurora from 20120603 and it didn't work, so the bug is fixed.