1423805 - Clear form button should not just clear all filled field states while credit card and address are categorized in the same section

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, defect, P3)

Description

[Ray Lin[:ralin]]

This is regression of Bug 1421641.

STR:
1. have at least one credit card and one address profile
2. visit https://luke-chang.github.io/autofill-demo/autocomplete-all.html
3. autofill address and autofill credit card
4. use clear button to clear address

Expected Result:
Credit card fields remain auto-filled with highlighted background


Actual Result:
All the fields in the same section are restore to normal state.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1423805 - Handle auto-filled input correctness upon target set instead of section-level.

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

Comment 2

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8935275 [details]
Bug 1423805 - Handle auto-filled input correctness upon target set instead of section-level.

https://reviewboard.mozilla.org/r/206178/#review212170

Are you going to add tests in another commit?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:766
(Diff revision 1)
> +          if (element instanceof Ci.nsIDOMHTMLInputElement) {
> +            isAutofilled |= fieldDetail.state == FIELD_STATES.AUTO_FILLED;
> +          } else {
> +            // Dim fields are those we don't attempt to revert their value
> +            // when clear the target set, such as <select>.
> +            dimFieldDetails.push(fieldDetail);
> +          }

We'll support <textarea> in the foreseeable future. In case we neglect changing this and introduce bugs at that time, I prefer to list the type of dim fields directly.

```js
if (ChromeUtils.getClassName(element) === "HTMLSelectElement") {
  // dimFieldDetails ...
} else {
  // isAutofilled...
}
```

::: browser/extensions/formautofill/FormAutofillHandler.jsm:775
(Diff revision 1)
> +          for (const fieldDetail of dimFieldDetails) {
> +            this.changeFieldState(fieldDetail, FIELD_STATES.NORMAL);
> +          }

We might need a comment here to remind us of what it is for.

Comment 3

[Ray Lin[:ralin]]

Comment on attachment 8935275 [details]
Bug 1423805 - Handle auto-filled input correctness upon target set instead of section-level.

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

Comment 4

[Ray Lin[:ralin]]

Comment on attachment 8935275 [details]
Bug 1423805 - Handle auto-filled input correctness upon target set instead of section-level.

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

Comment 5

[Ray Lin[:ralin]]

(In reply to Luke Chang [:lchang] from comment #2)
> Comment on attachment 8935275 [details]
> Bug 1423805 - Handle auto-filled input correctness upon target set instead
> of section-level.
> 
> https://reviewboard.mozilla.org/r/206178/#review212170
> 
> Are you going to add tests in another commit?
Thanks for reviewing! A test is added in the same commit.
> 
> ::: browser/extensions/formautofill/FormAutofillHandler.jsm:766
> (Diff revision 1)
> > +          if (element instanceof Ci.nsIDOMHTMLInputElement) {
> > +            isAutofilled |= fieldDetail.state == FIELD_STATES.AUTO_FILLED;
> > +          } else {
> > +            // Dim fields are those we don't attempt to revert their value
> > +            // when clear the target set, such as <select>.
> > +            dimFieldDetails.push(fieldDetail);
> > +          }
> 
> We'll support <textarea> in the foreseeable future. In case we neglect
> changing this and introduce bugs at that time, I prefer to list the type of
> dim fields directly.
> 
> ```js
> if (ChromeUtils.getClassName(element) === "HTMLSelectElement") {
>   // dimFieldDetails ...
> } else {
>   // isAutofilled...
> }
> ```
> 

fixed

> ::: browser/extensions/formautofill/FormAutofillHandler.jsm:775
> (Diff revision 1)
> > +          for (const fieldDetail of dimFieldDetails) {
> > +            this.changeFieldState(fieldDetail, FIELD_STATES.NORMAL);
> > +          }
> 
> We might need a comment here to remind us of what it is for.

fixed

---
As Bug 1417803 will be landing soon, and seems to be conflicted with this patch, I'll rebase on top of it once it landed. Thanks.

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

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8935275 [details]
Bug 1423805 - Handle auto-filled input correctness upon target set instead of section-level.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206178/diff/3-4/

Comment 7

[Ray Lin[:ralin]]

TH looks good after rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=243d4e30a1095f387d8044335d2250ef4b673171

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86a936b31146
Handle auto-filled input correctness upon target set instead of section-level. r=lchang

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/86a936b31146