Bug 1470887 - Preserve exactly autofilled values in the urlbar, and don't call losslessDecodeURI on them. r?mak
46 bytes, text/x-phabricator-request
|Details | Review|
8.18 KB, patch
|Details | Diff | Splinter Review|
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:62.0) Gecko/20100101 Firefox/62.0 Build ID: 20180621152331 Steps to reproduce: Typed a capital L as the first keystroke in the awesome bar Actual results: A lowercase "l" appeared with the autocomplete results for "localhost:8888" Expected results: A capital "L" should appear along with the same autocomplete results (which are not case sensitive). This would be consistent with the behavior when I type other capital letters as the first keystroke.
hm, this is strange, we should be respecting the casing and I cannot reproduce on Windows.
I can reproduce it (on a new profile). Probably a regression from my autofill work. I'll try to verify that.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm on a Mac too btw, like comment 0 indicates. It would be weird if this reproduced on Mac but not Windows, right?
Yeah, this doesn't reproduce on the 5/14/2018 Nightly, which is the one before the autofill bug landed. We/I should probably try to fix this soon. I probably removed a case check or splice in UnifiedComplete.js that I shouldn't have, so hopefully this isn't too hard to fix. Thanks for filing this bug.
Priority: -- → P1
Of course, happy to help. Thank you all for doing what you do.
I just gave this a try in nightly 63 and it still reproduces.
It's taken me a while to track this down. I'm still not sure where the problem is happening, but it seems to happen only for origins with ports. I can't reproduce it with example.com, or when localhost is bound to the standard port 80 and I therefore don't need to include a port. Still investigating.
My hunch is that the problem is in nsAutoCompleteController. We only started autofilling ports at all with the new autofill (see bug 764062). Previously, in this bug's case, you would only get "Localhost" autofilled, no port. It's plausible nsAutoCompleteController just isn't handling this case correctly now that it's been exposed. So in that sense this isn't a regression at all, but it's probably better to think of it as one since from the user's POV, you get the behavior described in comment 0, and it didn't use to happen.
Oh no the problem is the losslessDecodeURI call here: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/base/content/urlbarBindings.xml#1382
The problem is in the urlbar's onBeforeTextValueSet. When autofill fills an origin without a port (or a scheme), the makeURI call in onBeforeTextValueSet returns null. The string passed to makeURI in that case is something like "example.com/". Therefore onBeforeTextValueSet doesn't call losslessDecodeURI on the string, so the string is used as-is for the input value, preserving the case in whatever the user typed. But when makeURI does return a URI, we call losslessDecodeURI on it, which effectively downcases the string. makeURI returns a URI for strings like "example.com:8888/". It also returns a URI if you include the scheme, so this bug can also be reproduced if you visited localhost without a port and type "http://L", which gets autofilled to "http://localhost/". I modified the base autocomplete binding so that it doesn't call onBeforeTextValueSet if the value is being set due to autofill. We already had a _disableTrim property to handle a similar case: when _disableTrim is true, we don't call trimValue when `value` is set. I renamed _disableTrim to _textValueBeingSetDueToCompleteDefault. It seems OK to not call onBeforeTextValueSet in this case for all autocomplete implementations, not only the urlbar. IMO it makes sense because a value is being autofilled. But if you disagree, then we could move the `if (this._textValueBeingSetDueToCompleteDefault)` check directly to urlbar's onBeforeTextValueSet implementation. The patch also adds test tasks to test_autofill_origins.js. That test doesn't actually hit this bug since it's an xpcshell test so it's not using the browser's urlbar binding, but it seems like a good idea anyway.
Comment on attachment 8993541 [details] Bug 1470887 - Preserve exactly autofilled values in the urlbar, and don't call losslessDecodeURI on them. r?mak Marco Bonardo [::mak] has approved the revision. https://phabricator.services.mozilla.com/D2256
Attachment #8993541 - Flags: review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/22005ac8d9b2 Preserve exactly autofilled values in the urlbar, and don't call losslessDecodeURI on them. r=mak
STR for QA: 1. Start with a new profile 2. Start typing "mozilla" and verify it's autofilled to "mozilla.org/" 3. Start typing "MOZILLA" (all uppercase) and verify it's autofilled to "mozilla.org/" except it doesn't change the uppercase letters you've typed. For example, when you type "M", it should be autofilled to "Mozilla.org/". When you type "MOZ", it should be autofilled to "MOZilla.org/". 4. Start typing "https://mozilla.org" and verify it's autofilled to "http://mozilla.org/" 5. Start typing "https://MOZILLA" and verify it's autofilled to "https://mozilla.org/" except it doesn't change the uppercase letters you've typed, same as in step 3
Whiteboard: [fxsearch] → [fxsearch][STR for QA in comment 15]
Camelia, could you please verify this bug on Nightly and this Beta try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5701164694dc6a9368a29b4f54384daa6fbf7ff4 STR are in comment 15. Thank you! I'll request uplift after verification.
Looks like there are unrelated build failures on macOS right now, bug 1477408. I'll have to trigger new builds on macOS whenever that's fixed.
I've tested on Windows 7 x64, Ubuntu 16.04 x64 and macOS 10.13 using latest Nightly 63.0a1 (2018-07-23) and Beta try build from comment 17 (comment 19 - for mac) and everything looks fine according to STR from comment 15.
Comment on attachment 8993887 [details] [diff] [review] Beta/62 patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1239708 [User impact if declined]: When the user is typing in the address bar, we may not respect the typed casing, preventing the user to type the expected string [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: already provided [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: it's skipping one case where we were not expected to transform the input text [String changes made/needed]: none
Attachment #8993887 - Flags: approval-mozilla-beta?
Comment on attachment 8993887 [details] [diff] [review] Beta/62 patch Address bar fix for a recent regression in 62, verified in nightly, let's uplift for beta 13.
Attachment #8993887 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have managed to reproduce this issue by following the steps provided in comment 15 using Firefox 63.0a1 (BuildId:20180625220119). This issue is verified fixed using Firefox 62.0b12 (BuildId:20180726080725) available on treeherder, on Windows 10 64bit, macOS 10.13.4 and Ubuntu 16.04 64bit.
You need to log in before you can comment on or make changes to this bug.