Closed Bug 1423805 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Autofill, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(1 file)

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 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.
Attachment #8935275 - Flags: review?(lchang) → review+
Status: NEW → ASSIGNED
(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
Pushed by ralin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86a936b31146
Handle auto-filled input correctness upon target set instead of section-level. r=lchang
https://hg.mozilla.org/mozilla-central/rev/86a936b31146
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.