1422404 - Form autofill currently violates <input maxlength=1> restriction

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, defect, P3)

Description

[Daniel Holbert [:dholbert]]

My STR:
 1. Have a saved form autofill entry with e.g. FirstName, MiddleName, LastName.

 2. Go to a page that expects FirstName, MiddleInitial, LastName (with <input maxlength=1> to represent the middle initial).

 3. Notice & accept Firefox's formfill suggestion.

ACTUAL RESULTS:
The MiddleInitial <input> element ends up with your full middle name (more than 1 character, despite its maxlength=1 requirement).

EXPECTED RESULTS:
We should not have violated the maxlength=1 criteria on that <input> element.  For now, we should probably just avoid filling in that field (maybe even with a targeted heuristic to watch for "maxlength=1" and avoid autofilling such fields unless our autofill value is exactly 1 character)

(Eventually, we might want to be smarter and just truncate the autofilled value in certain recognized cases, as suggested in bug 1020839. But that's probably a bit harder to reason about & get reliably correct -- so for now, I think we should just block autofilling in this case.)

Comment 1

[Daniel Holbert [:dholbert]]

BTW, I ran into this bug when updating my contact information at USPS.com, specifically this URL:
  https://reg.usps.com/entreg/secure/EditProfilePersonalAction_input
(you need a free user-account to get to that page)

The affected input (whose maxlength Firefox violated when I accepted a Firefox autofill-suggestion for my complete information) has the following markup:
> <label id="for-tmI" for="tmI">M.I.</label>
> <input name="tmI" size="1" maxlength="1" value="" tabindex="10"
>        id="tmI" class="form-control" data-original-title=""
>        title="" type="text">

I am using Firefox Nightly 59.0a1 (2017-11-30) (64-bit) (though I'm pretty sure this is a longstanding issue).

Comment 2

[Robert Longson [:longsonr]]

It was fixed a long time ago in bug 444728 so this is a regression from that somehow. That bug included a test, I haven't checked what happened to the test.

Comment 3

[Daniel Holbert [:dholbert]]

Interesting! thanks.

Though I think this is a bit different.  That was about Autocomplete (which is a per-input-field thing), whereas I'm talking about the "formfill" feature here (which is where e.g. you start typing in a value for one field, and Firefox offers to fill in that + the next N fields all at once, with its saved copy of your name/address/etc)

Comment 4

[Daniel Holbert [:dholbert]]

(...and formfill is a relatively new feature, I think -- much newer than bug 444728.  I don't know whether or not traditional single-field autocomplete is also affected by this bug -- I'm not sure how to directly test that.)

Comment 5

[Luke Chang [:lchang] (inactive)]

Thanks for reporting. It's a bug indeed. Will discuss with UX how to deal with this case.


(In reply to Robert Longson [:longsonr] from comment #2)
> It was fixed a long time ago in bug 444728 so this is a regression from that
> somehow. That bug included a test, I haven't checked what happened to the
> test.

Form Autofill uses different modules from form-history's to fill in data, so I believe it doesn't affect bug 444728.

Comment 6

[Luke Chang [:lchang] (inactive)]

As discussed with UX, we decided to fill in the truncated data instead of nothing so that users won't think the profile is somehow broken.

Comment 7

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

Review commit: https://reviewboard.mozilla.org/r/205846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205846/

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211524

::: browser/extensions/formautofill/FormAutofillHandler.jsm:334
(Diff revision 1)
> +      let element = detail.elementWeakRef.get();
> +      if (element && element.tagName != "INPUT") {
> +        continue;
> +      }

@maxlength also exists on <textarea>.

Why not remove this check and go right to the element.maxLength handling?

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:476
(Diff revision 1)
> +               <input autocomplete="address-line1" maxlength="1">
> +               <input autocomplete="address-line2" maxlength="0">
> +               <input autocomplete="address-line3" maxlength="3">

I don't think truncating makes sense for address-line fields.

I still think it's worth considering a move closer to a Chromium model of a class for each type of field and have the logic for filling, reading, and formatting values in each class (possibly using shared helpers).

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211524

> @maxlength also exists on <textarea>.
> 
> Why not remove this check and go right to the element.maxLength handling?

I was thinking to avoid checking maxlength on <select> but just forgot <textarea>. Will fix it. Thanks for the reminder.

> I don't think truncating makes sense for address-line fields.
> 
> I still think it's worth considering a move closer to a Chromium model of a class for each type of field and have the logic for filling, reading, and formatting values in each class (possibly using shared helpers).

I don't think "address-line" fields make sense, either. I chose them just because there aren't many fields in the test profile. (I was thinking to use name-related fields for testing but that has to modify all the other tests.)

Truncating data is definately not enough. I intend to come up with a better way for each fields in bugs like bug 1020819 or what I've done for tel-related fields. And I second that we should leverage Chromium's model while implementing those follow-ups. In the meantime, however, I think we should at least not violate the "maxlength" restriction while filling the data. That's why I made this quick patch.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211524

> I don't think "address-line" fields make sense, either. I chose them just because there aren't many fields in the test profile. (I was thinking to use name-related fields for testing but that has to modify all the other tests.)
> 
> Truncating data is definately not enough. I intend to come up with a better way for each fields in bugs like bug 1020819 or what I've done for tel-related fields. And I second that we should leverage Chromium's model while implementing those follow-ups. In the meantime, however, I think we should at least not violate the "maxlength" restriction while filling the data. That's why I made this quick patch.

The test case should at least change to test something that is an actual expected output, not just one that is expected in the short term. Otherwise, the person who fixes the code to not truncated address-line will have to fix this "incorrect" test case.

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211524

> The test case should at least change to test something that is an actual expected output, not just one that is expected in the short term. Otherwise, the person who fixes the code to not truncated address-line will have to fix this "incorrect" test case.

Regarding the actual expected output, "address-line" might be relatively better because Chrome simply does truncating rules for "address-line" as well according to my test. For others (name, tel, email, address-level2 (city), address-level1 (state), country), there is a bigger chance that they'll have additional rules to adapt the "maxlength" restriction in the future. What do you think?

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211524

> Regarding the actual expected output, "address-line" might be relatively better because Chrome simply does truncating rules for "address-line" as well according to my test. For others (name, tel, email, address-level2 (city), address-level1 (state), country), there is a bigger chance that they'll have additional rules to adapt the "maxlength" restriction in the future. What do you think?

Well I think there are some good test cases that we can use that will be more stable:
```html
<input autocomplete="given-name" maxlength="1">
<input autocomplete="additional-name" maxlength="1">
<input autocomplete="family-name" maxlength="1">
<input autocomplete="postal-code" maxlength="5"> <!-- for the U.S. without the sorting code -->


```

Do you have to change all other tests if you use a different `profileData` property for this test?

Comment 13

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211524

> Well I think there are some good test cases that we can use that will be more stable:
> ```html
> <input autocomplete="given-name" maxlength="1">
> <input autocomplete="additional-name" maxlength="1">
> <input autocomplete="family-name" maxlength="1">
> <input autocomplete="postal-code" maxlength="5"> <!-- for the U.S. without the sorting code -->
> 
> 
> ```
> 
> Do you have to change all other tests if you use a different `profileData` property for this test?

Good idea. Will do.

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211766

It looks good. Just wait for the updated test case and read the patch again.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:340
(Diff revision 1)
> +      if (element && element.tagName != "INPUT") {
> +        continue;
> +      }
> +
> +      let maxLength = element.maxLength;
> +      if (maxLength === undefined || maxLength < 0 || profile[key].length <= maxLength) {

Quote the description for `maxlength` from MDN:

Specifying a negative number results in the default behavior (i.e. the user can enter an unlimited number of characters). The constraint is evaluated only when the value of the attribute has been changed.

Any invaild value of `maxlength` seems to be converted to -1 for indicating an unlimited number.
Is there any possible it would be `undefined`?

How about `maxLength == -1 || profile[key].length <= maxLength`?

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:476
(Diff revision 1)
> +               <input autocomplete="address-line1" maxlength="1">
> +               <input autocomplete="address-line2" maxlength="0">
> +               <input autocomplete="address-line3" maxlength="3">

How about add the test case of -1? e.g.
<input autocomplete="postal-code" maxlength="-1">

Comment 15

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205846/diff/1-2/

Comment 16

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review211766

> Quote the description for `maxlength` from MDN:
> 
> Specifying a negative number results in the default behavior (i.e. the user can enter an unlimited number of characters). The constraint is evaluated only when the value of the attribute has been changed.
> 
> Any invaild value of `maxlength` seems to be converted to -1 for indicating an unlimited number.
> Is there any possible it would be `undefined`?
> 
> How about `maxLength == -1 || profile[key].length <= maxLength`?

The `maxLength` will be `undefined` if it isn't an eligible element for setting `maxlength`. Although those elements should be excluded during identifying process already, I think it's still worth checking it here so we don't need to modify this code after supporting more types of elements. BTW, for now, it's used for <select> as I removed the check of <input> elements in the previous block.

> How about add the test case of -1? e.g.
> <input autocomplete="postal-code" maxlength="-1">

Good point. Added.

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8934918 [details]
Bug 1422404 - [Form Autofill] Adapt profiles to the maxlength restriction of fields.

https://reviewboard.mozilla.org/r/205846/#review213282

LGTM!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb00bf981b4e
[Form Autofill] Adapt profiles to the maxlength restriction of fields. r=seanlee

Comment 19

[Dorel Luca [:dluca]]

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