Closed Bug 1407508 Opened 7 years ago Closed 7 years ago

[Form Autofill] Create result for clear button menu if clicked on filled field in `startSearch`

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:V2])

Attachments

(5 files)

Bug 1407508 - Part 1. Expose field states constants in FormAutofillUtils.jsm.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1407508 - Part 2. Return clear form result instead of fallback to form history results for filled fields.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1407508 - Part 3. Add form autofill clear form button binding.
59 bytes, text/x-review-board-request
selee
: review+
lchang
: review+
Details
Bug 1407508 - Part 4. Implement clear populated form function for form autofill clear button.
59 bytes, text/x-review-board-request
lchang
: review+
selee
: review+
Details
Bug 1407508 - Part 5. Add mochitest for clear form button.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
We should make sure we got the correct result from the startSearch function when users click only on the filled fields. So, we'll need another path for clear button instead of directly fallback to form history if GUID presents.
status-firefox57: --- → fix-optional
Priority: -- → P3
I'll add Part 5 for mochitest soon later. Please help to review those 4 parts patches if you got some free times, thanks. Besides, I'll file a bug for the cached results problem for we should invalidate the mResults in nsAutoCompleteController once the form has been populated, otherwise we'll still get the previous cached results instead of the result for clear button when pressing key down.
Comment on attachment 8917311 [details]
Bug 1407508 - Part 1. Expose field states constants in FormAutofillUtils.jsm.

https://reviewboard.mozilla.org/r/188320/#review197130
Attachment #8917311 - Flags: review?(lchang) → review+
Depends on: 1410821
Comment on attachment 8917312 [details]
Bug 1407508 - Part 2. Return clear form result instead of fallback to form history results for filled fields.

https://reviewboard.mozilla.org/r/188322/#review197134

Looks good except only a few comments.

::: browser/extensions/formautofill/FormAutofillContent.jsm:26
(Diff revision 3)
> +XPCOMUtils.defineLazyModuleGetter(this, "ClearFormResult",
> +                                  "resource://formautofill/ProfileAutoCompleteResult.jsm");

Stale code?

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:138
(Diff revision 3)
> +    if (this.constructor.name == "CreditCardResult" && !this._isSecure && insecureWarningEnabled) {
> +      return "autofill-insecureWarning";
> +    }

Can we override this in `CreditCardResult` and leverage the remaining part by calling `super.getStyleAt()`?
Attachment #8917312 - Flags: review?(lchang)
Comment on attachment 8917312 [details]
Bug 1407508 - Part 2. Return clear form result instead of fallback to form history results for filled fields.

https://reviewboard.mozilla.org/r/188322/#review197134

> Stale code?

removed :p

> Can we override this in `CreditCardResult` and leverage the remaining part by calling `super.getStyleAt()`?

fixed and looks concise now. Thanks
Comment on attachment 8917312 [details]
Bug 1407508 - Part 2. Return clear form result instead of fallback to form history results for filled fields.

https://reviewboard.mozilla.org/r/188322/#review197458
Attachment #8917312 - Flags: review?(lchang) → review+
Comment on attachment 8920970 [details]
Bug 1407508 - Part 3. Add form autofill clear form button binding.

https://reviewboard.mozilla.org/r/191942/#review198030

::: toolkit/content/widgets/autocomplete.xml:1321
(Diff revision 4)
> +                "autofill-footer",
> +                "autofill-clear-button",
> +                "autofill-insecureWarning",
> +              ];
>                reusable = originalType === style ||
> -                         (style !== "autofill-profile" && originalType !== "autofill-profile" &&
> +                !(formAutofillStyles.includes(style) || formAutofillStyles.includes(originalType));

This condition is for the case that two different style are still possible to be reused except the FormAutofill related styles.

When a style is different with the previous one, it can be reused only in the case that they are with the similiar structure.

Any different structure of <content> won't be able to be reused, so `formAutofillStyles` is more like a list for excluding the different structure items.

Can you refine the comment to make the comment more general?
In addition, `formAutofillStyles` can be changed to `UNREUSEABLE_STYLES`.
Attachment #8920970 - Flags: review?(selee) → review+
Comment on attachment 8920971 [details]
Bug 1407508 - Part 4. Implement clear populated form function for form autofill clear button.

https://reviewboard.mozilla.org/r/191944/#review198006

LGTM! Thanks.

::: browser/extensions/formautofill/FormAutofillContent.jsm:531
(Diff revision 4)
> +    if (!focusedInput) {
> +      return;
> +    }
> +
> +    let formHandler = this.getFormHandler(focusedInput);
> +    if (!formHandler) {

I think the check can be removed since an inexistent `focusedInput` won't be able to go to `clearForm`.

If you have any reason to check this, please consider `_clearProfilePreview`[1] as well.

[1] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/browser/extensions/formautofill/FormAutofillContent.jsm#313

::: browser/extensions/formautofill/FormAutofillContent.jsm:599
(Diff revision 4)
> -    if (selectedRowStyle == "autofill-footer") {
> -      focusedInput.addEventListener("DOMAutoComplete", () => {
> +    focusedInput.addEventListener("DOMAutoComplete", () => {
> +      if (selectedRowStyle == "autofill-footer") {
>          Services.cpmm.sendAsyncMessage("FormAutofill:OpenPreferences");
> -      }, {once: true});
> -    }
> +      }

nit: `else if (selectedRowStyle == "autofill-clear-button")` here?
Attachment #8920971 - Flags: review?(selee) → review+
Comment on attachment 8920970 [details]
Bug 1407508 - Part 3. Add form autofill clear form button binding.

https://reviewboard.mozilla.org/r/191942/#review198054

::: browser/extensions/formautofill/content/formautofill.xml:329
(Diff revision 4)
> +        this._itemBox = document.getAnonymousElementByAttribute(
> +          this, "anonid", "autofill-item-box"
> +        );

It seems not to be used in this patch.
Attachment #8920970 - Flags: review?(lchang) → review+
Comment on attachment 8920971 [details]
Bug 1407508 - Part 4. Implement clear populated form function for form autofill clear button.

https://reviewboard.mozilla.org/r/191944/#review198068

::: browser/extensions/formautofill/FormAutofillContent.jsm:531
(Diff revision 4)
> +    if (!focusedInput) {
> +      return;
> +    }
> +
> +    let formHandler = this.getFormHandler(focusedInput);
> +    if (!formHandler) {

Although I can't think of any chance that it would happen, I feal it's ok to check and "log.warn". Not a strong opinion though.
Attachment #8920971 - Flags: review?(lchang) → review+
Move out some shared functions from test files and add a new test case for clear button in part 5. The TH result seems good as yet: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8a840e67517ee219e509ec442b9e800f3be11e2
Comment on attachment 8921385 [details]
Bug 1407508 - Part 5. Add mochitest for clear form button.

https://reviewboard.mozilla.org/r/192414/#review201726

Looks good. I'm only concerned about a few naming.

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:105
(Diff revision 2)
> +    elem = document.querySelector(elem);
> +  }
> +  is(elem.value, String(expectedValue), "Checking " + elem.id + " field");
> +}
> +
> +function checkFormAutofilled(profile) {

I was confused that the naming (including the old name, `checkFormFilled`) doesn't imply it'll help to press Enter key. How about `pressEnterAndCheckFormAutofilled` or `triggerAutofillAndCheckProfile`?

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:240
(Diff revision 2)
>  
>  function initPopupListener() {
>    registerPopupShownListener(popupShownListener);
>  }
>  
> +async function select(fieldSelector, selectIndex) {

The `select` sounds like "choose" or "click" but what it really does is "hover". Do you think if something like `triggerPopupAndHoverItem` would be better?

::: browser/extensions/formautofill/test/mochitest/test_clear_form.html:34
(Diff revision 2)
> +initPopupListener();
> +
> +
> +add_task(async function setup_storage() {

nit: Did you add these two blank lines on purpose?

::: browser/extensions/formautofill/test/mochitest/test_formautofill_preview_highlight.html:59
(Diff revision 2)
>  add_task(async function check_preview() {
>    const focusedInput = await setInput("#organization", "");
>  
>    doKey("down");
>    await expectPopup();
> -  checkFormPreviewFields();
> +  checkFormFieldsStyle();

nit: Is the suggested usage of the first argument `null` or omitted?
Attachment #8921385 - Flags: review?(lchang)
Comment on attachment 8921385 [details]
Bug 1407508 - Part 5. Add mochitest for clear form button.

https://reviewboard.mozilla.org/r/192414/#review201726

fixed all the issues, thanks for your prompt review :D
Comment on attachment 8921385 [details]
Bug 1407508 - Part 5. Add mochitest for clear form button.

https://reviewboard.mozilla.org/r/192414/#review201744

Thanks.
Attachment #8921385 - Flags: review?(lchang) → review+
Thanks. I'll mark checkin-needed once the dep(bug 1410821) is resolved, so maybe not hurry for 58.
Component: Form Manager → Form Autofill
NI myself, will rebase on top of bug 1339731 right after it landed.
Flags: needinfo?(ralin)
Looks good on try after rebase, thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83fd4efa31afe32efcd449367417c043bb07538f
Flags: needinfo?(ralin)
Keywords: checkin-needed
Blocks: 1392902
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61c0df291447
Part 1. Expose field states constants in FormAutofillUtils.jsm. r=lchang
https://hg.mozilla.org/integration/autoland/rev/f33cd39a7854
Part 2. Return clear form result instead of fallback to form history results for filled fields. r=lchang
https://hg.mozilla.org/integration/autoland/rev/bad080156dd0
Part 3. Add form autofill clear form button binding. r=lchang,seanlee
https://hg.mozilla.org/integration/autoland/rev/ae98f62c79a6
Part 4. Implement clear populated form function for form autofill clear button. r=lchang,seanlee
https://hg.mozilla.org/integration/autoland/rev/5a19f0e93ac2
Part 5. Add mochitest for clear form button. r=lchang
Keywords: checkin-needed
Reminder: Please do not add new XBL bindings without extraordinary need.
Flags: needinfo?(timdream)
Flags: needinfo?(kchang)
Hi Joe,

I talked to Luke, and I think it's best if we could share some context here:

* One of the design goals for form autofill was to make every UI piece self-contained in the system add-on.
* To do that, UI was implemented as XUL binding, with a base and inheritances. See bug 1326138, bug 1300995, bug 1371149.
* This bug is one of these children that inherit the base binding.

To minimize # of XBL bindings, we could remove the children and overload the base to do all the things. But it would be hard to remove the base while still keeping the design goal. We concluded that we should keep these bindings and move them to the technology of our choice at once.

Let me know what you think. Thanks!
Flags: needinfo?(timdream)
Flags: needinfo?(kchang)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #54)
> Hi Joe,
> 
> I talked to Luke, and I think it's best if we could share some context here:
> 
> * One of the design goals for form autofill was to make every UI piece
> self-contained in the system add-on.
> * To do that, UI was implemented as XUL binding, with a base and
> inheritances. See bug 1326138, bug 1300995, bug 1371149.
> * This bug is one of these children that inherit the base binding.
> 
> To minimize # of XBL bindings, we could remove the children and overload the
> base to do all the things. But it would be hard to remove the base while
> still keeping the design goal. We concluded that we should keep these
> bindings and move them to the technology of our choice at once.
> 
> Let me know what you think. Thanks!

Since this is fitting into the pattern already established with autocomplete-profile-listitem-base and children (https://bgrins.github.io/xbl-analysis/tree#autocomplete-profile-listitem-base), I think this new one won't add much extra cost to migrating the listitem subtree. Are you expecting to add any other new bindings for autofill?
Flags: needinfo?(timdream)
Hi Brian,

(In reply to Brian Grinstead [:bgrins] from comment #55)
> I think this new one won't add much extra cost to migrating the listitem subtree.

Thanks.

> Are you expecting to add any other new bindings for autofill?

No, there's no remaining feature related to the listitem or other bindings. It's the last one in our foreseeable scope.
Flags: needinfo?(timdream)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: