Last Comment Bug 720081 - Inline autocomplete drops https and completes without protocol/www.
: Inline autocomplete drops https and completes without protocol/www.
Status: RESOLVED FIXED
[sg:low][inline-autocomplete:normal][...
: sec-low
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 15
Assigned To: Marco Bonardo [::mak]
:
:
Mentors:
Depends on: 760803 765337 769348 769994 775072 781617 792856
Blocks: 566489 746572 754265 762479
  Show dependency treegraph
 
Reported: 2012-01-20 22:52 PST by Ed Lee :Mardak
Modified: 2013-04-30 11:25 PDT (History)
30 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
verified


Attachments
Part 1: controller should complete typeAhead differently (5.56 KB, patch)
2012-04-20 05:35 PDT, Marco Bonardo [::mak]
gavin.sharp: feedback+
Details | Diff | Splinter Review
Part 2: Places prefix data (22.28 KB, patch)
2012-04-20 05:46 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 1: controller should complete typeAhead differently (9.83 KB, patch)
2012-05-11 08:19 PDT, Marco Bonardo [::mak]
gavin.sharp: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 2: Add and use prefix data to Places moz_hosts (25.78 KB, patch)
2012-05-11 13:39 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 2: Add and use prefix data to Places moz_hosts (25.96 KB, patch)
2012-05-25 07:40 PDT, Marco Bonardo [::mak]
dietrich: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 1 update interdiff (10.60 KB, patch)
2012-06-03 06:32 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 1 updated: controller should handle typeAhead differently (22.49 KB, patch)
2012-06-03 06:33 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 1 updated: controller should handle typeAhead differently (22.54 KB, patch)
2012-06-03 06:58 PDT, Marco Bonardo [::mak]
gavin.sharp: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 1 updated: controller should handle typeAhead differently (20.68 KB, patch)
2012-06-03 15:44 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Ed Lee :Mardak 2012-01-20 22:52:53 PST
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 2012-01-20 22:55:53 PST
Jesse, how do you set the level of [sg:?]?
Comment 2 Marco Bonardo [::mak] 2012-01-21 01:20:06 PST
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] 2012-01-21 01:22:52 PST
and yes, it also strips www., like normal ac
Comment 4 Michael Lefevre 2012-01-21 04:07:20 PST
(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 2012-01-21 06:10:41 PST
This was a problem before inline autocomplete as well. See bug 658707.
Comment 6 Daniel Cater 2012-01-21 18:10:56 PST
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 Cater 2012-01-21 18:16:31 PST
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] 2012-01-23 03:31:01 PST
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 Cater 2012-01-26 09:53:22 PST
(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] 2012-01-26 10:00:49 PST
(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 Cater 2012-04-10 09:08:15 PDT
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 Cater 2012-04-10 09:11:17 PDT
Steps to reproduce in comment 9. Also try with www.bacsport.co.uk, www.squirrelmail.ucl.ac.uk.
Comment 13 Alex Keybl [:akeybl] 2012-04-11 16:46:28 PDT
(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] 2012-04-11 17:02:21 PDT
Sending over to Marco, since this bug is now tracked for release and requires an assignee.
Comment 15 Marco Bonardo [::mak] 2012-04-12 03:32:24 PDT
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] 2012-04-15 20:36:07 PDT
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] 2012-04-20 05:35:05 PDT
Created attachment 616940 [details] [diff] [review]
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] 2012-04-20 05:39:38 PDT
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] 2012-04-20 05:46:09 PDT
Created attachment 616943 [details] [diff] [review]
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] 2012-04-20 07:53:38 PDT
reminder: sqlite substr indices are 1-based, need to correct.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-02 12:14:45 PDT
[Triage Comment]
Adjusting tracking for 14 based on comment 18.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-10 15:14:20 PDT
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] 2012-05-11 08:19:09 PDT
Created attachment 623145 [details] [diff] [review]
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] 2012-05-11 13:39:32 PDT
Created attachment 623279 [details] [diff] [review]
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) 2012-05-24 18:03:33 PDT
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] 2012-05-25 07:40:19 PDT
Created attachment 627227 [details] [diff] [review]
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) 2012-05-25 13:57:44 PDT
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] 2012-05-29 02:02:24 PDT
Gavin, any possibility to review the controller part soonish? the merge is behind the corner
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-31 16:02:19 PDT
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 31 Marco Bonardo [::mak] 2012-06-01 07:57:12 PDT
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 33 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-02 13:39:13 PDT
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] 2012-06-02 15:26:13 PDT
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] 2012-06-02 15:26:53 PDT
basically here https://hg.mozilla.org/mozilla-central/rev/59409e2655ca#l1.167
Comment 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-02 16:40:36 PDT
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] 2012-06-03 01:51:14 PDT
Thank you Gavin!
Comment 38 Marco Bonardo [::mak] 2012-06-03 06:32:10 PDT
Created attachment 629599 [details] [diff] [review]
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] 2012-06-03 06:33:52 PDT
Created attachment 629600 [details] [diff] [review]
Part 1 updated: controller should handle typeAhead differently

Merged patch
Comment 40 Marco Bonardo [::mak] 2012-06-03 06:58:29 PDT
Created attachment 629603 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-06-03 10:00:28 PDT
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: gavin@gavinsharp.com] 2012-06-03 10:15:51 PDT
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: gavin@gavinsharp.com] 2012-06-03 10:16:17 PDT
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: gavin@gavinsharp.com] 2012-06-03 10:17:07 PDT
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] 2012-06-03 15:27:14 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] 2012-06-03 15:30:31 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] 2012-06-03 15:36:07 PDT
(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] 2012-06-03 15:44:46 PDT
Created attachment 629666 [details] [diff] [review]
Part 1 updated: controller should handle typeAhead differently

fixed the comment
Comment 49 Marco Bonardo [::mak] 2012-06-03 15:50:27 PDT
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 51 Vlad [QA] 2012-06-05 01:45:50 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.