1470887 - Unable to type capital L as the first character in the Awesome Bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[rkrueger11]

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.

Comment 1

[Marco Bonardo [:mak]]

hm, this is strange, we should be respecting the casing and I cannot reproduce on Windows.

Comment 2

[Drew Willcoxon :adw]

I can reproduce it (on a new profile).  Probably a regression from my autofill work.  I'll try to verify that.

Comment 3

[Drew Willcoxon :adw]

I'm on a Mac too btw, like comment 0 indicates.  It would be weird if this reproduced on Mac but not Windows, right?

Comment 4

[Drew Willcoxon :adw]

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.

Comment 5

[rkrueger11]

Of course, happy to help.  Thank you all for doing what you do.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

I just gave this a try in nightly 63 and it still reproduces.

Comment 7

[Drew Willcoxon :adw]

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.

Comment 8

[Drew Willcoxon :adw]

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.

Comment 9

[Drew Willcoxon :adw]

Oh no the problem is the losslessDecodeURI call here: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/base/content/urlbarBindings.xml#1382

Comment 10

[Drew Willcoxon :adw]

Attachment: Bug 1470887 - Preserve exactly autofilled values in the urlbar, and don't call losslessDecodeURI on them. r?mak

Comment 11

[Drew Willcoxon :adw]

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 12

[Drew Willcoxon :adw]

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

Comment 13

[Phabricator Automation]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22005ac8d9b2
Preserve exactly autofilled values in the urlbar, and don't call losslessDecodeURI on them. r=mak

Comment 15

[Drew Willcoxon :adw]

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

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/22005ac8d9b2

Comment 17

[Drew Willcoxon :adw]

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.

Comment 18

[Drew Willcoxon :adw]

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.

Comment 19

[Drew Willcoxon :adw]

macOS build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e52c7a915c7eec0f320c440ee9a350e95ecb93bd

Comment 20

[Drew Willcoxon :adw]

Attachment: Beta/62 patch

Comment 21

[Camelia Badau [:cbadau], Desktop Test Engineering]

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 22

[Marco Bonardo [:mak]]

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

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a33a2a19522

Comment 25

[Emil Ghitta, QA [:emilghitta]]

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.