Closed Bug 1124238 Opened 9 years ago Closed 6 years ago

keywords don't have anymore priority over autofill

Categories

(Firefox :: Address Bar, defect, P2)

Product:
Firefox
Component:
Address Bar
Version:
35 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed

People

(Reporter: mark.lehky, Assigned: mak)

References

Details

(Keywords: regression, 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 file, 3 obsolete files)

Bug 1124238 - The autofill placeholder should not override keywords.
59 bytes, text/x-review-board-request
mak
: review+
Details
Component: Untriaged → Search
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
(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)
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?
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: UnifiedComplete
I guess it might be a regression due to bug 950797.
Blocks: 950797
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: location bar suggestions searches in wrong order → keywords don't have anymore priority over autofill
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
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
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
Status: NEW → ASSIGNED
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)
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. :)
(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
Attached patch fix-bug-1124238.diff (obsolete) — — Splinter Review 
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)
(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
Attached patch fix-bug-1124238.diff (obsolete) — — Splinter Review 
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)
@Paolo: Given the long review queue from Marco at the moment, would you mind taking a look at this patch?
Flags: needinfo?(paolo.mozmail)
(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)
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+
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.
(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.
Blocks: 1192218
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [unifiedcomplete][fxsearch]
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
Attached patch fix-bug-1124238.diff (obsolete) — — Splinter Review 
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 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)
(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
(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.
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.
@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
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.
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.
(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.
Priority: P2 → P3
it's still confusing
Priority: P3 → P2
Seeing Julian not being active on Bugzilla for a long time, I unassign him from this bug.

Sebastian
Assignee: julian.viereck → nobody
Status: ASSIGNED → NEW
Hopefully I'll have the time to complete his work in the future.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #8712180 - Flags: feedback?(mak77)
Faced this bug today as well in FF 55 and 57. It shows the autofill, but opens the bookmark -> very confusing.

Actually I would expect the autofill to be shown AND executed after pressing [Enter]. I think it would still be confusing if bookmark keywords disable the autofill for these characters.

For me it would be perfect like this: Running a search (or open bookmark) should only be done if the user entered: 
[KeywordChars] + [Space] + [SomeSearchKey]
For the bookmark the search key would be empty.
Keywords are not generic keywords, this is a specific property that can be set in Firefox bookmarks.
Ie, if my facebook bookmarks has a keyboard property of f, enter f at the location bar to get the URL, not http://fu.bar I was reading 5 minutes ago.

f + arrow down makes more sense if you want the first suggestion instead of a keyword property, as it has been for a long time.

Finally, if you want autofill, then do NOT set keywords in your bookmarks.
Thanks for the quick reply. Really nice to get such quick feedback.

I am aware that the keyword is a specific property. However, the current behavior is at least confusing, at worst dangerous as Smylers pointed out in comment 49.

Your example looks like this for me (still speaking for FF 55 and 57): I have a facebook bookmark with keyword 'f'. I type 'f' and the address bar is autofilled with http://fu.bar (which i visited 5 mins ago) + the first suggestion is http://fu.bar (also highlighted). I press [Enter] and http://facebook.com is opened. 

Furthermore, pressing the [Arrow down] will get me to the second suggestion, which might be http://foo.com. To get to the URL that FF shows me (as autofill and first suggestion), I have to press [Arrow right] so the autofill becomes the actual value in the address bar.
Well, at least the autocompletion list now tells you that you access the bookmarked page (unfortunately only the domain and not the full URL), which mitigates the phishing szenario outlined by Smylers a bit.

Nontheless, my expection would be that when the keyword for a bookmarked page is entered that it is not autocompleted and that the first item in the suggestion list reflects the bookmarked page correctly (maybe including a hint that the entered string matches a keyword).

Applied to the test case I mentioned in comment 39 this would work like follows:
Typing 't' would autocomplete to 'test.net/', then typing 'e' would remove the autocompletion and show something like '* Firefox Nightly First Run Page — https://www.mozilla.org/en-US/firefox/nightly/firstrun/ — Visit' (where the asterisk denotes the bookmark star) as first item in the suggestions list like it's done when you type a part of the bookmarked URL.

Sebastian
Attachment #8712180 - Attachment is obsolete: true
Comment on attachment 8982921 [details]
Bug 1124238 - The autofill placeholder should not override keywords.

https://reviewboard.mozilla.org/r/248748/#review254968

I verified and unbitrotted your patch. I'm sorry it took so long, but since we're in the process of changing autofill this sounded like the right time to fix this longstanding annoyance
Attachment #8982921 - Flags: review?(mak77) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/de4bc518d2d4
The autofill placeholder should not override keywords. r=mak
https://hg.mozilla.org/mozilla-central/rev/de4bc518d2d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: qe-verify-
Tested this in Nightly 62.0a1 (2018-06-04) and the entered keyword is not autocompleted anymore. So, thank you for that!

Unfortunately, the URL of the bookmark is not displayed as expected yet but only the domain, as outlined in comment 58. Is that intended? If so, what's the reason behind that? And if not, should I file a new bug for that?

Sebastian
Flags: needinfo?(mak77)
(In reply to Sebastian Zartner [:sebo] from comment #65)
> Tested this in Nightly 62.0a1 (2018-06-04) and the entered keyword is not
> autocompleted anymore. So, thank you for that!
> 
> Unfortunately, the URL of the bookmark is not displayed as expected yet but
> only the domain, as outlined in comment 58. Is that intended? If so, what's
> the reason behind that? And if not, should I file a new bug for that?

It's "by design" for now, keywords were intended to be a way to search, and in that case we show "domain: searchstring". If you don't type a searchstring after the keyword, we only show "domain". The alias functionality of keywords (typing just the keyword to go to a page) is sort-of deprecated and will probably disappear in some future version of Firefox (likely when bug 648398 will be fixed).
You are free to file a trivial bug if you wish, but it will likely end up as a P5 for us, we'll unlikely have resources to work on it.
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: