keywords don't have anymore priority over autofill

ASSIGNED
Assigned to

Status

()

Firefox
Location Bar
P2
normal
Rank:
37
ASSIGNED
3 years ago
2 months ago

People

(Reporter: SiKing, Assigned: Julian Viereck)

Tracking

(Blocks: 2 bugs, {regression})

35 Branch
regression
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [unifiedcomplete][fxsearch])

User Story

When a search engine alias or a bookmark keyword matches the currently typed text, do not autofill even if the text matches the previously autofilled entry.

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (obsolete)
Component: Untriaged → Search
Comment hidden (obsolete)
(Reporter)

Comment 2

3 years ago
I can reproduce this on my Linux machine at home too. I changed the "Platfom" fields to All.
OS: Windows 7 → All
Hardware: x86_64 → All
(Reporter)

Comment 3

3 years ago
(In reply to :Gijs Kruitbosch from comment #1)
> Marco, related to bug 1114022, perhaps? (that bug confuses me, it says it
> landed for 36, but also says verified for 34, so now I don't understand if
> it can be implicated here or not)

I just had a look at that bug; I cannot reproduce that problem in version 35. I do not think mine here and that bug are related?
I'm honestly not sure what the bug is reporting, cause at point 3 you say you are pressing Enter... you didn't select anything from the popup right?

In such a case just typing a tag and pressing enter would either take autofill value or do a search.
What am I missing? are you using enter selects add-on?


(In reply to :Gijs Kruitbosch from comment #1)
> Marco, related to bug 1114022, perhaps? (that bug confuses me, it says it
> landed for 36, but also says verified for 34, so now I don't understand if
> it can be implicated here or not)

it was a typo, I fixed it.
Flags: needinfo?(mak77)
(Reporter)

Comment 5

2 years ago
No add-ons.

I think I might have messed up my description. Where I say "tag", I should have said "keyword". Like this: http://www-archive.mozilla.org/docs/end-user/keywords.html
Sorry about that.
So you are saying that your keyword is being autofilled to a host?
does it work if you just add a space after the keyword, before pressing enter?
(Reporter)

Comment 7

2 years ago
The keyword feature stills works. It used to be that keywords were given priority. Yes, adding a space, or some other way of deleting the suggestion offered from history, is a workaround.
so, I'm not sure this is a regression, maybe you just typed once a domain that matches and you didn't type it before the 35 timeframe.
I'm not sure what's the future of keywords used as shortcuts, so it's hard to make a decision here.
I think UnifiedComplete here would take the expected path.
Depends on: 995091
even if, actually, this shouldn't happen per
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsPlacesAutoComplete.js#1462
I guess it might be a regression due to bug 950797.
Blocks: 950797

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

2 years ago
Summary: location bar suggestions searches in wrong order → keywords don't have anymore priority over autofill

Updated

2 years ago
Blocks: 1071461

Updated

2 years ago
Keywords: regression
(Assignee)

Comment 11

2 years ago
Could someone check if the regression range is within bug 950797? Happy to dig into this in case it turns out it's a regression from bug 950797. Bug 950797 landed on mc in:

  https://hg.mozilla.org/mozilla-central/rev/a98aca20c7be
Keywords: regressionwindow-wanted

Comment 12

2 years ago
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f12535b53f52&tochange=a98aca20c7be

Regressed by: a98aca20c7be	Julian Viereck — Bug 950797 - Use result from last completion in nsAutoCompleteController to reduce autoFill UI flickering until search results come back. r=mak
Keywords: regressionwindow-wanted

Updated

2 years ago
Duplicate of this bug: 1128063
(Assignee)

Comment 14

2 years ago
Thanks Alice for determine the exact regression cause!

I am very sorry for having introduced this regression in my work on bug 950797. My plan is to look into the cause of the problem over the weekend and then let you know what I have discovered so far.
Assignee: nobody → julian.viereck
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
Duplicate of this bug: 1123265
(Assignee)

Comment 16

2 years ago
Okay, I took a look at how to write a failing unit test for this regression and then start fixing it from there (aka TDD). There are already a bunch of tests that check the behavior about the keyword matching and especially the ones in

  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unifiedcomplete/test_keywords.js

look promising but I fail at the moment at emulating adding another character, that should make the autocompletion get from completion the URL to completing keyword.

In more details: what I want to test is something along the lines of:

    add_task(function* test_less_then_equal_than_keyword() {
      do_print("Searching for less than keyworded entry should autoFill it");
      yield promiseAddVisits({ uri: NetUtil.newURI("http://mozilla.org/test/"),
                               transition: TRANSITION_TYPED });
      addBookmark({ uri: NetUtil.newURI("http://mozilla.org/test/"), keyword: "moz" });

      // The following should complete the full URI.
      yield check_autocomplete({
        search: "mo",
        autofilled: "mozilla.org/",
        completed: "mozilla.org/",
      });

      // The next insertion should complete to the keyword and not to the URI.
      yield check_autocomplete({
        search: "moz",
        autofilled: "moz",
        completed: "moz",
      });
      yield cleanup();
    });

Unfortunately, this test passes and therefore is not possible to reproduce the regression. Digging into the definition of |check_autocomplete| I think the problem is, that for each call to |check_autocomplete| the |AutoCompleteInput| is rebuild. However, for this test case, the input should stay the same, receive another text input (by typing the character "z" to get "mo" -> "moz)) and then the resulting completion must be checked. Returning the used input object from the |check_autocomplete| and reusing it for the second call to |check_autocomplete| should be doable. But I do not see how to emulate pressing the "z" character in this setup, e.g.:

    add_task(function* test_less_then_equal_than_keyword() {
      do_print("Searching for less than keyworded entry should autoFill it");
      yield promiseAddVisits({ uri: NetUtil.newURI("http://mozilla.org/test/"),
                               transition: TRANSITION_TYPED });
      addBookmark({ uri: NetUtil.newURI("http://mozilla.org/test/"), keyword: "moz" });
      let input = yield check_autocomplete({
        search: "mo",
        autofilled: "mozilla.org/",
        completed: "mozilla.org/",
      });

      synthesizeKey("z", {});

      yield check_autocomplete({
        search: "moz",
        autofilled: "moz",
        completed: "moz",
      }, input);
      yield cleanup();
    });

Where I try to emulate the pressing of the "z" character by invoking |synthesizeKey("z", {});|. It turns out this functionality is not available in this kind of unit tests and I get a "synthesizeKey is undefined" exception when running the above test code.

@Marco: What is your recommendation for writing a test case here? Do you see a way to get something similar to |synthesizeKey| working for these tests or should I write a new XUL based test like the one in bug 950797?
Flags: needinfo?(mak77)
(Reporter)

Comment 17

2 years ago
What test framework does FF use? Or to put it differently: What would I have to know if I wanted to take a crack at this?
Assume I have previous test automation experience. :)
(Assignee)

Comment 18

2 years ago
(In reply to SiKing from comment #17)
> What test framework does FF use? Or to put it differently: What would I have
> to know if I wanted to take a crack at this?
> Assume I have previous test automation experience. :)

(Please note that I am myself not an expert in this area, but here is what my mental model looks like.)

There are many different kind of tests used in the Mozilla project - not just one "test framework". You can find a list of the different kind of tests here:

  https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

I think the tests in |test_keywords.js| are so called "xpcshell" tests, which you can find more information about here:

  https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

To be able to use the |synthesizeKey("z", {});| function, I guess we need to write |Mochitest
plain| as described here:

  https://developer.mozilla.org/en-US/docs/Mochitest
(Assignee)

Comment 19

2 years ago
Created attachment 8563630 [details] [diff] [review]
fix-bug-1124238.diff

This patch provides a simple test case for this bug and includes a bugfix. The bugfix makes the failing test pass and I was also able to verify the bugfix also fixes the reported bug by running an instance of Firefox and adding a keyword "bug" to my bugzilla bookmark.

The root of the problem is based in

  nsresult
  nsAutoCompleteController::CompleteDefaultIndex(int32_t aResultIndex) {...}

If the search result should be completed, this function calls

  CompleteValue(resultValue);

With the fix from bug 950797 the input is completed before the search result comes back and nothing is done in case the search result should NOT be completed (aka. |CompleteValue(resultValue);| is not called). This is obvious wrong, as the input string is completed though the later arriving result tells to not do so. To fix this wrong behavior, the completed placeholder string should be reset again. As a simple first attempt to fix this, I reset the input field again by the search string:

  input->SetTextValue(mSearchString);

To check if this is enough I scheduled a try run of this patch, which can be found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eaa43b35d35. I let you know what the result of the try run looks like.
Flags: needinfo?(mak77)
(Assignee)

Comment 20

2 years ago
(In reply to Julian Viereck from comment #19)
> Created attachment 8563630 [details] [diff] [review]
> fix-bug-1124238.diff

The first try run was broken (haven't checked my patch after I did a |hg pull -u|) and you can find the latest try run version of the patch here:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=503c50c04d2b

There is one test failing at the moment, which I will look into:

  TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarAutoFillTrimURLs.js
(Assignee)

Comment 21

2 years ago
Created attachment 8566431 [details] [diff] [review]
fix-bug-1124238.diff

This patch is based on the old one and fixes the regression. A green try run with this patch can be found here:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a421318b934a

To fix the regression the input value is only reset to the original value if the value in the input field has actually changed.

Requesting review from @Marco on this patch.
Attachment #8563630 - Attachment is obsolete: true
Attachment #8566431 - Flags: review?(mak77)
(Assignee)

Comment 22

2 years ago
@Paolo: Given the long review queue from Marco at the moment, would you mind taking a look at this patch?
Flags: needinfo?(paolo.mozmail)

Comment 23

2 years ago
(In reply to Julian Viereck from comment #22)
> @Paolo: Given the long review queue from Marco at the moment, would you mind
> taking a look at this patch?

It's been a while since I last looked at the autocomplete code, but I might get to this review before Marco if you don't mind waiting a few more days. I don't think I'll be available before Tuesday next week though.

Marco, if you have any general level observation in the meantime, it may simplify the detailed review.
Flags: needinfo?(paolo.mozmail) → needinfo?(mak77)
I don't have very specific comments about the patch, cause every patch in AC is its own world apart.
As I said on IRC, my assumption is that sometimes the search provider returns no default complete index, and thus we should not reuse the buffered value. So on that side the patch looks proper.

But any single change in autocomplete can cause lots of minor bugs here and there, so it's required to double check which other code or feature the change might affect.
Flags: needinfo?(mak77)
Comment on attachment 8566431 [details] [diff] [review]
fix-bug-1124238.diff

let's setup a first come first serve request for now.
Attachment #8566431 - Flags: review?(paolo.mozmail)

Updated

2 years ago
Duplicate of this bug: 1136982

Updated

2 years ago
Duplicate of this bug: 1122427

Comment 28

2 years ago
Comment on attachment 8566431 [details] [diff] [review]
fix-bug-1124238.diff

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

Okay, this looks well-tested so I think we should be good to go, but I have a question first, as well as some code structure suggestions.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +1603,5 @@
> +
> +    // Reset the search string again in case it was completed with the
> +    // mPlaceholderCompletionString but the eventually recieved search result
> +    // should not be completed. Only reset the input if needed to avoid
> +    // tiggering unnecessary new searches.

Does this comment imply that, when we call SetTextValue because we have to remove the extra part from the search string, this would trigger a new search?

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ +100,5 @@
>    // At this point frecency could still be updating due to latest pages
>    // updates.
>    // This is not a problem in real life, but autocomplete tests should
>    // return reliable resultsets, thus we have to wait.
>    yield PlacesTestUtils.promiseAsyncUpdates();

When a previous input is provided, maybe we should not call promiseAsyncUpdates, so that we don't delay setting up the test listeners.

Also, I'm not totally sure that we should attach a new controller later in the function. Maybe we should just reuse the previous controller if there is a previous input, this would be more similar to what happens in practice.

Instead of a new argument, you should be able to add an optional "input" field to the options.

You can consider splitting the test function in two parts taking both one "test" argument. The first function creates the input, stores a reference to it inside the "test" object, and invokes the second function. The second function would also be called directly by your test.
Attachment #8566431 - Flags: review?(paolo.mozmail)
Attachment #8566431 - Flags: review?(mak77)
Attachment #8566431 - Flags: feedback+
(Assignee)

Comment 29

2 years ago
Thanks for the review :Paolo!

(In reply to :Paolo Amadini from comment #28)
> Comment on attachment 8566431 [details] [diff] [review]
> fix-bug-1124238.diff
> 
> ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
> @@ +1603,5 @@
> > +
> > +    // Reset the search string again in case it was completed with the
> > +    // mPlaceholderCompletionString but the eventually recieved search result
> > +    // should not be completed. Only reset the input if needed to avoid
> > +    // tiggering unnecessary new searches.
> 
> Does this comment imply that, when we call SetTextValue because we have to
> remove the extra part from the search string, this would trigger a new
> search?

Yes, but let me double check.

In your opinion, what is the correct behavior? Should search reset/call to SetTextValue trigger a new search or not?

I guess triggering a new search for the call to SetTextValue is okay here. From a user perspective, the displayed result should stay the same. Triggering a new search is not the best from a performance perspective but AFAIKT preventing a new search to be triggered would require an additional state variable in the AutoCompleteController to ignore input changes triggering a news search. As the AutoCompleteController is already pretty complex I think it is only worth adding such a suppress-input-reset-trigger-new-search variable if there is a real need for it.

Comment 30

2 years ago
(In reply to Julian Viereck from comment #29)
> I guess triggering a new search for the call to SetTextValue is okay here.
> From a user perspective, the displayed result should stay the same.
> Triggering a new search is not the best from a performance perspective but

My main concern, rather than performance, is that retriggering the search might work now but may have implications like some events being fired twice or code paths followed multiple times. As the code is already subject to race conditions, this might create new ones in non-obvious ways.

> AFAIKT preventing a new search to be triggered would require an additional
> state variable in the AutoCompleteController to ignore input changes
> triggering a news search. As the AutoCompleteController is already pretty
> complex I think it is only worth adding such a
> suppress-input-reset-trigger-new-search variable if there is a real need for
> it.

It seems that there is no really optimal solution here, but I'd prefer a sync flag used to suppress the event rather than retriggering the search.

Updated

2 years ago
Blocks: 1192218

Updated

2 years ago
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [unifiedcomplete][fxsearch]

Comment 31

2 years ago
Marco, is the patch still relevant with changes to search presentation order with suggestions?
Rank: 37
Flags: needinfo?(mak77)
yes, this bug is still relevant.
Flags: needinfo?(mak77)
Priority: P3 → P2
(Assignee)

Comment 33

a year ago
Created attachment 8712180 [details] [diff] [review]
fix-bug-1124238.diff

Rebased and updated patch partially addressing the comments from :Paolo from comment #28.

(As :Paolo and :mak are involved on this bug, I set both for feedback)

(In reply to :Paolo Amadini from comment #28)
>
> ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
> @@ +100,5 @@
> >    // At this point frecency could still be updating due to latest pages
> >    // updates.
> >    // This is not a problem in real life, but autocomplete tests should
> >    // return reliable resultsets, thus we have to wait.
> >    yield PlacesTestUtils.promiseAsyncUpdates();
> 
> When a previous input is provided, maybe we should not call
> promiseAsyncUpdates, so that we don't delay setting up the test listeners.
> 
> Also, I'm not totally sure that we should attach a new controller later in
> the function. Maybe we should just reuse the previous controller if there is
> a previous input, this would be more similar to what happens in practice.
> 
> Instead of a new argument, you should be able to add an optional "input"
> field to the options.
> 
> You can consider splitting the test function in two parts taking both one
> "test" argument. The first function creates the input, stores a reference to
> it inside the "test" object, and invokes the second function. The second
> function would also be called directly by your test.

The test was changed according to these comments. However, I kept the 'check_autocomplete' together (in contrast to splitting it) as I think the code is easier to read this way.


(In reply to :Paolo Amadini from comment #28)
> Comment on attachment 8566431 [details] [diff] [review] [diff] [review]
> fix-bug-1124238.diff
> 
> ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
> @@ +1603,5 @@
> > +
> > +    // Reset the search string again in case it was completed with the
> > +    // mPlaceholderCompletionString but the eventually recieved search result
> > +    // should not be completed. Only reset the input if needed to avoid
> > +    // tiggering unnecessary new searches.
> 
> Does this comment imply that, when we call SetTextValue because we have to
> remove the extra part from the search string, this would trigger a new
> search?

(In reply to :Paolo Amadini from comment #30)
> (In reply to Julian Viereck from comment #29)
> > I guess triggering a new search for the call to SetTextValue is okay here.
> > From a user perspective, the displayed result should stay the same.
> > Triggering a new search is not the best from a performance perspective but
> 
> My main concern, rather than performance, is that retriggering the search
> might work now but may have implications like some events being fired twice
> or code paths followed multiple times. As the code is already subject to
> race conditions, this might create new ones in non-obvious ways.

I tried to change the code to avoid the retriggering of events once the text content gets reset. However, I cannot see how this can be done. The problem is that resetting the text requires calling the |nsIAutoCompleteInput.textValue = ...| setter, which might trigger any kind of events. These events cannot be suppressed by adding a sync flag to the nsAutoCompleteController.


Therefore, the only thing possible is to prevent the controller from accepting change events while the input is reset.


Q: Is stopping the handling in the controller when resetting the text good enough or does anyone see a better solution here?
Attachment #8566431 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mak77)
Attachment #8712180 - Flags: feedback?(paolo.mozmail)
Attachment #8712180 - Flags: feedback?(mak77)

Comment 34

a year ago
Comment on attachment 8712180 [details] [diff] [review]
fix-bug-1124238.diff

I'll leave this for Marco to review, for this iteration.
Flags: needinfo?(paolo.mozmail)
Attachment #8712180 - Flags: feedback?(paolo.mozmail)
sorry for the delay, I'm a little bit overloaded with requests and regressions atm. I should be able to come to this along this week.

Regarding the question, it's hard to tell off-hand. I'd say if you don't notice strange behaviors or flickering in the urlbar with the current patch we could probably take it. We are doing something similar when we detect that the user removed autofilled part, we retrigger a new query.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#267

The only mishandling I see in the patch, is that we may have multiple results, the default index could also be provided by the second result. With this code we'd clear the placeholder and retrigger, even if the second result has a default index (and maybe it was the same). So in that case (default index provided by the second result) we'd reintroduce flicker plus continuous retriggers.
But I don't see an alternative and probably the most common case is that the first result provides a defaultIndex.

If you have something in mind to avoid the retriggers it may be worth taking a look at it.
Flags: needinfo?(mak77)
(Assignee)

Comment 36

a year ago
(In reply to Marco Bonardo [::mak] from comment #35)
> sorry for the delay, I'm a little bit overloaded with requests and
> regressions atm. I should be able to come to this along this week.

No worries Macro - it took me way to long to get back to this as well...

 
> Regarding the question, it's hard to tell off-hand. I'd say if you don't
> notice strange behaviors or flickering in the urlbar with the current patch
> we could probably take it. 


As it turned out giving this patch a "real spin" in the browser turned out it does not work. However, I am not sure if something else is "broken" here. Here is what I tried:

- create a tag "other" for the entry "https://www.bugzilla.org/"

- type "other" in the URL bar

You can find the visual output here: http://postimg.org/image/rjeyvd2wt/full/

The completion menu show as first entry (which is also the selected one) "other - search with Google" and then as second entry the Bugzilla URL (which matches the "other" tag). 

@Marco: Is this the expected behavior? From reading comment #1 of this bug it seems the second entry should be selected as it matches the tag?

---

A second test (which also failed):

- visit "facebook.com" to add it to the history

- create a tag "face" for "plus.google.com"

- enter "face" in the URL bar

The visual output is here: http://postimg.org/image/40kn8fqyb/full/

Again, according to the comment #1 of this bug, I think the correct behavior would be to show the "plus.google.com" entry first, but that's not the case.

I've also investigated what the result of calling |GetDefaultCompleteValue(...)| from |nsAutoCompleteController::CompleteDefaultIndex| at [1] is, and this is the result

  resultValue=facebook.com/ 
  aResultIndex=0

Which fits the visual result.

@Marco: Again, this result seems in contradiction to the first comment on this bug. But I am not sure if the result is expected or needs fixing.

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1726

 
> The only mishandling I see in the patch, is that we may have multiple
> results, the default index could also be provided by the second result. With
> this code we'd clear the placeholder and retrigger, even if the second
> result has a default index (and maybe it was the same). So in that case
> (default index provided by the second result) we'd reintroduce flicker plus
> continuous retriggers.

If the second entry has the default index, shouldn't |CompleteValue(...)| get called because the call to |GetDefaultCompleteValue(...)| succeeded?


  if (NS_SUCCEEDED(GetDefaultCompleteValue(aResultIndex, true, resultValue))) {
     CompleteValue(resultValue);
  } else {
Flags: needinfo?(mak77)
I think there is a misunderstatement here, the bug is not about tags, it's about keywords. Comment 0 used the wrong word.
The bug is about search engine aliases and bookmark keywords, when an alias or a keyword match the currently typed text, we should not autofill to an host.
Flags: needinfo?(mak77)
User Story: (updated)
I put the right bug description in the user story field now.
In bug 1122427 I posted steps to reproduce this issue, which I want to repeat here to make it clear what is meant:

1. Create a new Firefox profile
2. Save the shown first run page as bookmark
3. Set 'te' as keyword for the bookmark (via the Library, searching for the bookmark, selecting it, clicking 'More' at the bottom and typing it into the 'Keyword' field)
4. Type 'test.net' into the awesome bar and hit Enter
5. Open a new tab
6. Type 'te'

=> It is auto-completed to 'test.net/', though it should *not* be autocompleted in that case to allow hitting Enter to go to the keyworded bookmark.

Sebastian
(Reporter)

Comment 40

a year ago
(In reply to Marco Bonardo [::mak] from comment #37)
> I think there is a misunderstatement here, the bug is not about tags, it's
> about keywords. Comment 0 used the wrong word.
> The bug is about search engine aliases and bookmark keywords, when an alias
> or a keyword match the currently typed text, we should not autofill to an
> host.

Yes Marco. You are absolutely correct. I think I mentioned same in my Comment #5 above; I should have corrected the description too.
(Assignee)

Comment 41

a year ago
Thanks a lot for all people involved to fix my confusion :)

(In reply to Sebastian Zartner [:sebo] from comment #39)
> In bug 1122427 I posted steps to reproduce this issue, which I want to
> repeat here to make it clear what is meant:
> 
> 1. Create a new Firefox profile
> 2. Save the shown first run page as bookmark
> 3. Set 'te' as keyword for the bookmark (via the Library, searching for the
> bookmark, selecting it, clicking 'More' at the bottom and typing it into the
> 'Keyword' field)
> 4. Type 'test.net' into the awesome bar and hit Enter
> 5. Open a new tab
> 6. Type 'te'
> 
> => It is auto-completed to 'test.net/', though it should *not* be
> autocompleted in that case to allow hitting Enter to go to the keyworded
> bookmark.
> 
> Sebastian

TLDR: The patch fixes Sebastian's steps-to-reproduce.


I tested that the keyword is not complete: when typing "te" the completion is not applied, while typing "t" or "tes" performs the completion to "test.net" as expected.
(Assignee)

Comment 42

a year ago
@Marco: *ping* - easter holidays are coming and I will have a bit of spare time to work on feedback for the current patch in case you have time to provide it till then.
comment 35 is still what makes me think... in case of multiple async searches, where it's not the first result to provide a default index, but the second one, the patch will cause flickering.
And we can't tell if the next incoming result will provide a default index or not, cause it didn't arrive yet. Basically the patch is correct only if mSearches.Length() == 1 and a subset of the other cases.
And I don't have ideas on how to fix that, cause we can't tell what will come with the next result.
So either we leave this unfixed for multiple searches, or we flicker for multiple searches.
I'd probably vote for the former, so change the else into an else if (mSearches.Length() == 1), add a comment with a TODO (bug NNN) explaining the problem, and file a bug for the multiple searches case.
Obviously the behavior changed some time ago.
mozregression gives me this range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66e07ef46853&tochange=f36a4a4ff73f

Regarding the steps from comment 39 the behavior is now that you still get 'test.net/' as autocompletion when you type 'te', though when you hit Enter, you get to the first run page (https://www.mozilla.org/en-US/firefox/nightly/firstrun/).

While that works for my use case (I get to the right page when hitting Enter), the autocomplection for 'test.net' is confusing, because another page gets loaded.

Sebastian

Comment 45

7 months ago
Great. Works how it should in FF 49. ONCE MORE BROKEN in 50...

There are missing tests verificating, that is working, or what is wrong. So much regression of this.

Comment 46

7 months ago
I confirm comment #45.
Well, this is not fixed, so I'm not surprised it doesn't work reliably.
Fwiw it seems to be working on 52 for me (maybe even 51?) but it is basically wallpapered by another fix, and not properly addressed as here.

Updated

7 months ago
Duplicate of this bug: 1321553

Comment 49

5 months ago
(In reply to Sebastian Zartner [:sebo] from comment #44)

> While that works for my use case (I get to the right page when hitting
> Enter), the autocomplection for 'test.net' is confusing, because another
> page gets loaded.

‘Confusing’ is an understatement: the Firefox 52 behaviour is a regression in that a URL as displayed in the Location Bar is no longer necessarily where you'll go on pressing Enter.

Arguably it's dangerous:

1 Set up a phishing website — say, for the real site wellknownbank.com, set up wellknown.bank.

2 On a victim's Firefox, create a bookmark to your phishing website with the keyword ‘wel’ (that is, the first few letters of the real site's domain). Maybe you run a net café and have some bookmarks pre-installed in profiles. Or maybe somebody's left their computer unlocked and you have time to sneak a bookmark on to it before they return.

3 The victim types ‘wel’ in the Location Bar. This autocompletes to the URL of the genuine site that they've visited before, displaying ‘wel[lknownbank.com]’ (with the part in brackets being selected). Because the URL is correct, they don't bother glancing down at the entries in the drop-down list to see that this looks different from normal. They press Enter.

4 Firefox goes to the URL in the bookmark, not the one that was on the victim's screen. They unsuspectingly enter passwords or personal details into the phishing site.

I've managed to do step 3 above to myself: typing in enough of a domain name until I see the URL of the page I want complete in the Location Bar, then not stopping to check if the characters I've typed so far happen to be any of the bookmark keywords that I've set up at some point in the past, nor noticing that first entry in the list that's dropped down is different, and ending up at a different site to that expected.

If the Location Bar didn't autocomplete when a keyword had been typed, then I clearly wouldn't be expecting that to go to my target site; I'd keep typing more characters from its URL until the completion appeared.
Comment hidden (obsolete)

Updated

3 months ago
Priority: P2 → P3
it's still confusing
Priority: P3 → P2

Updated

2 months ago
Duplicate of this bug: 1363197
You need to log in before you can comment on or make changes to this bug.