Inline autocomplete drops https and completes without protocol/www.

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Location Bar
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Mardak, Assigned: mak)

Tracking

({sec-low})

Trunk
Firefox 15
sec-low
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13- wontfix, firefox14+ verified)

Details

(Whiteboard: [sg:low][inline-autocomplete:normal][parity-chrome][advisory-tracking-])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

6 years ago
Jesse, how do you set the level of [sg:?]?
Version: unspecified → Trunk
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
and yes, it also strips www., like normal ac

Comment 4

6 years ago
(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...
Whiteboard: sg:moderate

Comment 5

6 years ago
This was a problem before inline autocomplete as well. See bug 658707.
Whiteboard: sg:moderate → [sg:low]

Comment 6

6 years ago
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

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Updated

6 years ago
Whiteboard: [sg:low] → [sg:low][inline-autocomplete:normal]

Comment 9

6 years ago
(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?
(Assignee)

Comment 10

6 years ago
(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

5 years ago
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.
Whiteboard: [sg:low][inline-autocomplete:normal] → [sg:low][inline-autocomplete:normal][parity-chrome]

Comment 12

5 years ago
Steps to reproduce in comment 9. Also try with www.bacsport.co.uk, www.squirrelmail.ucl.ac.uk.
tracking-firefox13: --- → ?
(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.
tracking-firefox13: ? → +
Sending over to Marco, since this bug is now tracked for release and requires an assignee.
Assignee: nobody → mak77
(Assignee)

Comment 15

5 years ago
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

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

Updated

5 years ago
Blocks: 746572
(Assignee)

Comment 17

5 years ago
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.
Attachment #616940 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 18

5 years ago
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 :(
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
reminder: sqlite substr indices are 1-based, need to correct.
[Triage Comment]
Adjusting tracking for 14 based on comment 18.
status-firefox13: --- → wontfix
tracking-firefox13: + → -
tracking-firefox14: --- → +
Keywords: sec-low
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.
Attachment #616940 - Flags: feedback?(gavin.sharp) → feedback+
(Assignee)

Updated

5 years ago
Blocks: 754265
(Assignee)

Comment 23

5 years ago
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.
Attachment #616940 - Attachment is obsolete: true
Attachment #616943 - Attachment is obsolete: true
Attachment #623145 - Flags: review?(gavin.sharp)
(Assignee)

Comment 24

5 years ago
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.
Attachment #623279 - Flags: review?(dietrich)
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
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?
(Assignee)

Comment 26

5 years ago
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
Attachment #623279 - Attachment is obsolete: true
Attachment #623279 - Flags: review?(dietrich)
Attachment #627227 - Flags: review?(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!
Attachment #627227 - Flags: review?(dietrich) → review+
(Assignee)

Comment 28

5 years ago
Gavin, any possibility to review the controller part soonish? the merge is behind the corner
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.
Attachment #623145 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/mozilla-central/rev/59409e2655ca
https://hg.mozilla.org/mozilla-central/rev/960b80d99b4a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(Assignee)

Comment 31

5 years ago
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.
Attachment #627227 - Flags: approval-mozilla-aurora?
Attachment #627227 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #623145 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/cff98981add8
https://hg.mozilla.org/releases/mozilla-aurora/rev/99805da33911
status-firefox14: --- → fixed

Updated

5 years ago
Depends on: 760803
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
Status: RESOLVED → REOPENED
status-firefox14: fixed → affected
Resolution: FIXED → ---
(Assignee)

Comment 34

5 years ago
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.
(Assignee)

Comment 35

5 years ago
basically here https://hg.mozilla.org/mozilla-central/rev/59409e2655ca#l1.167
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
(Assignee)

Comment 37

5 years ago
Thank you Gavin!
(Assignee)

Comment 38

5 years ago
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.
Attachment #623145 - Attachment is obsolete: true
Attachment #629599 - Flags: review?(gavin.sharp)
(Assignee)

Comment 39

5 years ago
Created attachment 629600 [details] [diff] [review]
Part 1 updated: controller should handle typeAhead differently

Merged patch
(Assignee)

Comment 40

5 years ago
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.
Attachment #629600 - Attachment is obsolete: true
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 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?
Attachment #629599 - Flags: review?(gavin.sharp)
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.
Attachment #629603 - Flags: review+
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
Attachment #629603 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 45

5 years ago
(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.
(Assignee)

Comment 46

5 years ago
(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.
(Assignee)

Comment 47

5 years ago
(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
(Assignee)

Comment 48

5 years ago
Created attachment 629666 [details] [diff] [review]
Part 1 updated: controller should handle typeAhead differently

fixed the comment
Attachment #629599 - Attachment is obsolete: true
Attachment #629603 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 50

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1851b8c13061
status-firefox14: affected → fixed

Comment 51

5 years ago
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.
status-firefox14: fixed → verified
Blocks: 762479
Depends on: 765337

Updated

5 years ago
Depends on: 769994
(Assignee)

Updated

5 years ago
Depends on: 769348
Whiteboard: [sg:low][inline-autocomplete:normal][parity-chrome] → [sg:low][inline-autocomplete:normal][parity-chrome][advisory-tracking-]

Updated

5 years ago
Depends on: 775072

Updated

5 years ago
Depends on: 781427
No longer depends on: 781427

Updated

5 years ago
Depends on: 781617
(Assignee)

Updated

5 years ago
Depends on: 792856
You need to log in before you can comment on or make changes to this bug.