448486 - urlbar autofill (inline autocomplete) and ctrl-enter don't work nicely together

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[r]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

When browser.urlbar.autofill is enabled, it clashes with ctrl-enter, and the resulting behavior is perhaps not the most useful. When part of the urlbar contents has been filled in by autofill, and ctrl-enter is pressed, one would expect ctrl-enter to override what has been filled in by autofill. But this is not what happens. See below for example.

Reproducible: Always

Steps to Reproduce:
1. Enable browser.urlbar.autofill
2. Vist news.google.com
3. Clear urlbar and enter: news
4. Press ctrl-enter
Actual Results:  
news.google.com is loaded.

Expected Results:  
www.news.com is loaded.

After news has been typed, .google.com is added to the address by autofill. If ctrl-enter is pressed, it's most likely that the user wanted www.news.com, and what autofill added should be removed.

Comment 1

[Shawn Wilsher :sdwilsh]

Bulk closing all UNCONFIRMED bugs dealing with places that haven't had any bug activity in over 120 days, have no votes, and are not enhancement requests.

If you are still experiencing this issue in Firefox 3.0 or later, please re-open the bug with steps to reproduce (if they were not part of the original comment).

Comment 2

[alex_mayorga]

I can reproduce what this bug describes on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:12.0a1) Gecko/20120131 Firefox/12.0a1 ID:20120131031150

Comment 3

[Timvde]

Maybe when holding ctrl, the autocomplete can change to <whateveristyped>.com to indicate the user is going there when pressing enter. This should revert on releasing ctrl, of course.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

unless you are a peer, you should not play with dependencies, thanks.

Comment 5

[Yann Brelière]

This is also the case for Shift+Enter (normally .net) and Ctrl+Shift+Enter (normally .org).

Test case:
* Type "google"
* Press Ctrl+Shift+Enter

Actual result:
google.com is loaded

Expected result: 
google.org is loaded

Comment 6

[george]

On Firefox 14, setting browser.urlbar.autoFill to false fixes this issue, and seems to leave the rest of the autocomplete behavior intact (i.e., I can still press down to see the suggestions, and enter to choose one).

Comment 7

[Timvde]

It doesn't fix the compatibility issue with autocomplete, as setting that preference to false disables autocomplete entirely. I actually like autocomplete, but I am forced to turn it off because of bugs like these. That shouldn't be necessary.

To let the browser not do unexpected things, this should be indicated in the autofill when holding ctrl (as I described in comment 3). This will also make the feature more apparent and easy to discover for users.

Comment 8

[rb]

I am interested in working on this bug. Can it be assigned to me?

Comment 9

[rb]

Attachment: This patch prevents default visit to the autocompleted site when "ctrl+enter" is pressed. Test it and point out the problems.

Comment 10

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

Comment on attachment 655947 [details] [diff] [review]
This patch prevents default visit to the autocompleted site when "ctrl+enter" is pressed. Test it and point out the problems.

I don't think this is a suitable solution, because it ties the base autocomplete binding's behavior (used for autocomplete across in all toolkit-based applications) to Firefox-specific behavior (Ctrl/Shift+Enter).

You may be able to apply a similar solution to the urlbarBindings.xml version of this binding, which overrides the generic autocomplete binding for the Firefox URL bar specifically.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 655947 [details] [diff] [review]
This patch prevents default visit to the autocompleted site when "ctrl+enter" is pressed. Test it and point out the problems.

what Gavin said in comment 10, we have to implement a browser-side solution

Comment 13

[rb]

Attachment: Patch to prevent default visit to autocompleted site in firefox whn ctrl/shift+enter is pressed

Comment 14

[Fester Couples]

Hello, this issue still exists in 16.0

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 657683 [details] [diff] [review]
Patch to prevent default visit to autocompleted site in firefox whn ctrl/shift+enter is pressed

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

::: browser/base/content/urlbarBindings.xml
@@ +691,5 @@
>            this.removeAttribute("actiontype");
>          }
> +        if (event.keyCode === KeyEvent.DOM_VK_CONTROL ||
> +             event.keyCode === KeyEvent.DOM_VK_SHIFT)
> +             this.value = this.value.substring(0, this.selectionStart);

So, this looks like "working", but has a couple problems.

We don't have a way to tell if the value was inline completed, so you correctly use the selection to guess. The problem lies in the fact this doesn't make any kind of checks on it, so it will also overwrite usermade selections.

inline autocomplete on the other side happens on really specific conditions:
- selectionStart > 0
- selectionEnd == value.length - 1
- selectionStart < selectionEnd (though I'm not sure if here we properly support the select direction, to be verified)
- cursor is at the end of the string

If we enforce these conditions, then the only case we'd not handle is if the user starts selecting from the middle of the string, selects up to the end, and then ctrl/shift+enter, that is a quite more limited use-case we may ignore (the most common selections in this widget likely start from the beginning or the end, since those are easier targets).

Plus, this special code should do anything only if this.completeDefaultIndex is true, since that's the issue.

The other issue is that this basically cuts away the value when you down CTRL or SHIFT, but doesn't restore it if you change your mind and up the key, so basically this adds a new behavior to the CTRL/SHIFT keys that would be unique among all existing text fields: cut away selection. This is non native and possibly a bit weird.

You need a solution that intercepts ENTER, checks all of the above conditions and acts accordingly.

Comment 18

[alex_mayorga]

This bug just bite me on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130304030933 CSet: 86c98c4d36da so marking as trunk.

Comment 20

[Thomas Wisniewski]

Attachment: 448486-ctrl_shift+enter_considers_only_user_input_not_autofilled_text.diff

:mak, this patch seems to be what you had in mind in comment 15, but it uses a different approach based on other code I see in urlbarBindings.xml. What do you think? It seems to work fine with both RTL and LTR mode.

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f65233c67245ed5d3116cc050e39da23feaf8a3

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8880235 [details] [diff] [review]
448486-ctrl_shift+enter_considers_only_user_input_not_autofilled_text.diff

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

thank you, it's great that you are looking into this and also wrote a test!

::: browser/base/content/test/urlbar/browser.ini
@@ +101,5 @@
>  [browser_urlbarTrimURLs.js]
>  subsuite = clipboard
>  [browser_urlbarUpdateForDomainCompletion.js]
>  [browser_urlbar_autoFill_backspaced.js]
> +[browser_urlbar_autoFill_bypassed.js]

I'd maybe name this browser_urlbar_canonize_on_autofill.js

::: browser/base/content/test/urlbar/browser_urlbar_autoFill_bypassed.js
@@ +5,5 @@
> +async function test_autocomplete(data) {
> +  let {desc, typed, autofilled, modified, waitForUrl, keys, action} = data;
> +  info(desc);
> +
> +  let promiseLoad = waitForDocLoadAndStopIt(waitForUrl);

I'd probably move this down just before you start synthesizing keys

@@ +28,5 @@
> +  Services.prefs.setBoolPref("browser.urlbar.autoFill", true);
> +
> +  // Add a typed visit, so it will be autofilled.
> +  await PlacesTestUtils.addVisits({
> +    uri: NetUtil.newURI("http://example.com/"),

the NetUtils.newURI here should not be needed, a string will just work.

@@ +59,5 @@
> +                            keys: [["VK_RETURN", { shiftKey: true }]],
> +                            action: "searchengine"
> +                          });
> +
> +  await PlacesTestUtils.clearHistory();

not needed, since you put this in the cleanup function already.

::: browser/base/content/urlbarBindings.xml
@@ +1427,5 @@
> +            event.keyCode === KeyEvent.DOM_VK_RETURN &&
> +            (event.shiftKey || event.ctrlKey)) {
> +          this.textValue = this.handleEnterSearchString ||
> +                           this.mController.searchString;
> +        }

the urlbar code changed a lot in the last 5 years, I think we may have better ways to do this now.

See maybeCanonizeURL. We pass .value to it, but we may want to pass the searchstring instead. Is there a case where we should canonize something different than what the user typed? I can't think of any, so far.
What do you think?

Comment 22

[Thomas Wisniewski]

Comment on attachment 8880235 [details] [diff] [review]
448486-ctrl_shift+enter_considers_only_user_input_not_autofilled_text.diff

>Is there a case where we should canonize something different than what the user typed? I can't think of any, so far. What do you think?

I can't think of anything specific, but then I'm not very familiar with the UI code yet, so I'd have to leave that to your better judgement. It looks like maybeCanonizeURL (or something very similar) is called after this code is run anyway, to adjust the URL and add .com/.net/etc. I don't think there's much point in calling it again here, from the looks of it.

Regardless, there were a couple of failures caught by the try run which I've addressed in a new patch (I did need to add the selectionStart/End conditions, as well as MacOSX-specific handling for the ctrl-key case). I'm sending that patch through another try-run, and if it fares well then I'll adjust it to address your other notes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d87012e9c3c1699c57bf8e9008be3ed3b83093a

Let me know if you have any other concerns, and I'll take another look!

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Thomas Wisniewski from comment #22)
> It looks
> like maybeCanonizeURL (or something very similar) is called after this code
> is run anyway, to adjust the URL and add .com/.net/etc. I don't think
> there's much point in calling it again here, from the looks of it.

No, I meant that we should not have to guess whether we autofilled or not by checking selection, and we should not change textValue anymore. We can have better hooks now.
We can indeed check if selectedIndex == 0 && controller.getStyleAt(0).includes("autofill"). At that point we could set a flag and check that flag in handleEnter, if it's set we could pass the searchstring instead of .value to maybeCanonizeURL.

The only thing I don't recall off-hand, and should be verified, is whether in handleEnter we can still query the selectedIndex and getStyleAt, or if we should do that earlier. I would try to put the check just before the current call to maybeCanonizeURL and see if it works.

Another case that came to my mind is, for example, typing "porcu" in the locationbar, I get a Search suggestion for "porcupine" and I want to canonize that to "www.porcupine.com" so I CTRL+ENTER on it.

(PS: Another alternative could involve somehow exposing the reason for the last textValue set, through setTextValueWithReason in autocomplete.xml, we basically know when textValue is autofilled).

Comment 24

[Thomas Wisniewski]

Attachment: 448486-ctrl_shift+enter_on_urlbar_canonizes_search_suggestions_and_ignores_autofill_results.diff

Ah, I see. Then this patch seems to do the trick for both canonizing search suggestions and ignoring the autofill as reported.

Thankfully it did not require anything fancy, just checking in-situ in handleEnter to see whether the input value is a search suggestion, or if it has autofilled text.

I've also updated another test to cover the search suggestion case as well.

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8880527 [details] [diff] [review]
448486-ctrl_shift+enter_on_urlbar_canonizes_search_suggestions_and_ignores_autofill_results.diff

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

Once you have an updated patch, please set a flag on me so I can push it to the Try server to check for eventual intermittent or unexpected failures (unless you already have commit access to the Try Server).

::: browser/base/content/test/urlbar/browser_urlbarSearchSuggestions.js
@@ +54,5 @@
> +  }
> +
> +  let promiseLoad = waitForDocLoadAndStopIt(expectedUrl);
> +
> +  for (let i=0; i<idx; ++i) {

please add whitespaces around "=" and "<" per coding style

@@ +77,5 @@
> +
> +add_task(async function shiftEnterOnSuggestion() {
> +  return testPressEnterOnSuggestion("http://www.foofoo.net/",
> +                                    { shiftKey: true });
> +});

I think it will be enough to run the ctrl test, we are already testing the shift code path in the other test.
The risk I see here is that the test becomes too complex and takes too much time to run, going too close to the tests timeout. Since it's not critical to test everything here, we can go with the 2 first tests.

::: browser/base/content/test/urlbar/browser_urlbar_canonize_on_autofill.js
@@ +1,3 @@
> +/* This test ensures that pressing ctrl/shift+enter bypasses the autoFilled
> + * value, and only considers what the user typed (but not just enter).
> + */

please add a license boilerplate at the top:

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

::: browser/base/content/urlbarBindings.xml
@@ +1316,5 @@
>            this.handleEnterSearchString = this.mController.searchString;
>  
>            if (!this._deferredKeyEventQueue.length &&
>                (this.popup.selectedIndex != 0 || this.gotResultForCurrentQuery)) {
> +            let url = this.value;

it may not be an url, indeed it's likely not an url.
let's name this "canonizeValue" for now.

@@ +1322,5 @@
> +                                   event.metaKey :
> +                                   event.ctrlKey)) {
> +              let action = this._parseActionUrl(url);
> +              if (action && action.type == "searchengine") {
> +                url = action.params.searchSuggestion;

I'm not sure we need this at all, since when a search suggestion is selected, its value is already put in the input field. I think in this case this.value == action.params.searchSuggestion. Please check.

Comment 26

[Thomas Wisniewski]

>please set a flag on me so I can push it to the Try server to check for eventual intermittent or unexpected failures (unless you already have commit access to the Try Server)

I do have access to push to try, and was going to do a final try-run after your next batch of comments (since the last run passed and all the changes since then were quite trivial).

If there's a specific flag I should be setting then just let me know. I've just been using: ./mach try -b do -p linux64,macosx64,win64 -u 'all[x64,10.10,Windows 8]' -t 'none[x64,10.10,Windows 8]' --no-retry

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

"-u firefox-ui-functional,mochitests" may be enough tests.

once the tests are complete, you can look at the raw log to find where your test was run (in which chunk of mochitest-browser) and retrigger that chunk 5-10 times to check for intermittents. But eventually I could do that, I don't know if try access also allows to retrigger tests. In that case just set any flag on me and I'll do (likely on Tuesday since on Monday I'm traveling for the Mozilla All Hands).

Comment 28

[Thomas Wisniewski]

Sure, I think I can retrigger the test myself. And who knows? Maybe we'll bump into each other at the All Hands, too.

Comment 29

[Thomas Wisniewski]

Attachment: 448486-ctrl_shift+enter_on_urlbar_canonizes_search_suggestions_and_ignores_autofill_results.diff

>I'm not sure we need this at all, since when a search suggestion is selected, its value is already put in the input field.

At that stage the actual value isn't the suggestion itself, but rather a string like this:

    'moz-action:searchengine,{"engineName":"Google","input":"porcupine","searchQuery":"porcu","searchSuggestion":"porcupine"}'

That's why I have to parse it out.

I've addressed your other comments though, and am doing a try-run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04addea49956ef21748766c7d9b5021f312a2173

I'll re-run the bc? tasks myself, though I only see my tests running in the e10s mochitests for OSX/Linux. I'll request another review once that's done.

Also note that the eslint failure on that try-run is fixed in the patch I'm attaching here; for some reason "mach eslist" was somehow suddenly broken in my local setup and telling me that there were no errors, which is how I missed these bugs.

Comment 30

[Thomas Wisniewski]

Comment on attachment 8880899 [details] [diff] [review]
448486-ctrl_shift+enter_on_urlbar_canonizes_search_suggestions_and_ignores_autofill_results.diff

Alright, I've given the patch 10 runs through the tests and only unrelated tests failed, so I think it's ready for the next round of reviewing.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8880899 [details] [diff] [review]
448486-ctrl_shift+enter_on_urlbar_canonizes_search_suggestions_and_ignores_autofill_results.diff

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

once the patch is updated, you can add the checkin-needed keyword to land.

::: browser/base/content/urlbarBindings.xml
@@ +1322,5 @@
> +                                   event.metaKey :
> +                                   event.ctrlKey)) {
> +              let action = this._parseActionUrl(canonizeValue);
> +              if (action && action.type == "searchengine") {
> +                canonizeValue = action.params.searchSuggestion;

The last problem I oversee here, is that not all "searchengine" entries have a searchSuggestion property. Or better, currentl that's the case, but in future we may have other searchEngine entries without it.
So here you could probably just replace action.type == "searchengine" with "searchSuggestion" in action.params.

Comment 32

[Thomas Wisniewski]

Attachment: 448486-ctrl_shift+enter_on_urlbar_canonizes_search_suggestions_and_ignores_autofill_results.diff

>So here you could probably just replace action.type == "searchengine" with "searchSuggestion" in action.params.

Yes, that did indeed work.

Here's the updated patch with r+ ; requesting check-in.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84f8a3442301
Have ctrl/shift+enter on urlbar canonize search suggestions, and ignore autofill results. r=mak

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84f8a3442301

Comment 35

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly firefox-3.1a2pre.en-US.win32 (2008-7-29) on Windows 7, 64 Bit!

This bug's fix is verified with latest Beta!

Build  ID  : 20170810180547 
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 36

[Bogdan Maris, Desktop QA]

I verified this bug using Beta 56.0b9 on Windows 10 x64, Windows 7 x32 and Mac OS x 10.11 and it looks that it's fixed.

On the other hand on Ubuntu (14.04 x32) it's still reproducing. When you click on Ctrl+Enter it still opens the news.google.com page. Any thoughts?

Comment 37

[Thomas Wisniewski]

Odd, it's working fine for me on today's nightly in both Gentoo and on a stock Ubuntu VM. Is it possible you somehow have browser.urlbar.autofill disabled on that device?

Comment 38

[Bogdan Maris, Desktop QA]

(In reply to Thomas Wisniewski from comment #37)
> Odd, it's working fine for me on today's nightly in both Gentoo and on a
> stock Ubuntu VM. Is it possible you somehow have browser.urlbar.autofill
> disabled on that device?

Thank you for the suggestion, but that wasn't the problem, "browser.urlbar.autofill" pref is enabled. I retested on Ubuntu 16.04 x64, on a different machine and the bug was still reproducing on Beta 56.0b9.
I verified it on latest Nightly 57.0a1 too, and you are right. The bug is fixed there.

Comment 39

[Thomas Wisniewski]

That's very odd. It's working fine for my Gentoo Linux box on 56b9 as well. Have you had a chance to try a fresh profile or a fresh beta reinstall? I'm not sure what it could be at this point except a weird installation issue/respin, or something profile-specific.

Comment 40

[Bogdan Maris, Desktop QA]

I retested everything on the machines I tested earlier using beta 56.0b9 and I couldn't reproduce the bug. Maybe it's one of those bugs that occasionally occur. But the issue does seem to be fixed at this point. In case I can manage to reproduce this issue again constantly I will log a new bug.

Comment 41

[eyal gruss]

i reopened https://bugzilla.mozilla.org/show_bug.cgi?id=594217 - to add a keyboard shortcut

Comment 42

[eyal gruss]

(In reply to eyal gruss (eyaler) from comment #41)
> i reopened https://bugzilla.mozilla.org/show_bug.cgi?id=594217 - to add a
> keyboard shortcut

sorry wrong place