1541929 - On undo/Ctrl+Z, autofill completes the URL without a selection and moves the caret to the end

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Marco Bonardo [:mak]]

While reproducing another bug, I hit this case.
I time a few random chars in the urlbar, starting from a, like "asdfwqekj", I press ctrl+Z, it changes to "asrock.com/"... At first I was surprised, then I noticed in my profile "as" autofills to asrock.com.
The big problem is that there's no selection, the site is just autofilled in place. I think we should not autofill in this case, but the old urlbar does autofill, with removable selection, that is already a better solution than what we have now.

Comment 1

[Marco Bonardo [:mak]]

Didn't really mean to transform adw in a bug!

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [::mak] from comment #1)

Didn't really mean to transform adw in a bug!

That's deep, man.

Comment 3

[Drew Willcoxon :adw]

nsAutoCompleteController doesn't complete the default index when there's a text selection. iow selectionStart must == selectionEnd for autofill to happen due to processing the first result. UrlbarInput doesn't have that restriction, which is why this happens in quantumbar but not awesomebar.

Specifically, nsAutoCompleteController returns here: https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1475

And UrlbarInput does the autofill in this bug here: https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/browser/components/urlbar/UrlbarInput.jsm#488

I'd like to fix this bug by bring quantumbar to parity with awesomebar as I just described, and then file another bug for considering not autofilling at all when undoing. They're two separate tasks that we should consider separately imo. (We might be able to use editor's nsITransactionListener to listen for undo and prohibit autofill: https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/editor/txmgr/nsITransactionListener.idl#55 )

Comment 4

[Drew Willcoxon :adw]

Attachment: Bug 1541929 - Don't autofill the first result in some cases.

We need to handle autofilling the first result separately from autofilling results in general (which happens in UrlbarInput.setValueFromResult), so add a new UrlbarInput.autofillFirstResult method. The controller calls it instead of setValueFromResult. I ported the logic from nsAutoCompleteController, as described in the bug.

Other changes are related to the new test for this.

As part of this work, I was interested in learning how awesomebar handles browser_autoFill_typed.js, so I added it to the legacy tests, with a small tweak in the test for awesomebar.

Comment 5

[Drew Willcoxon :adw]

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

Comment 6

[Drew Willcoxon :adw]

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

Comment 7

[Drew Willcoxon :adw]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea02b41a2b1d
Don't autofill the first result in some cases. r=mak

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ea02b41a2b1d

Comment 10

[Drew Willcoxon :adw]

I filed bug 1545559 for specifically not autofilling on undo. I'll update this bug's summary so it's clear what we ended up fixing.