1371113 - [Form Autofill] Prompt a door hanger to ask users whether to save a credit card profile when users submit a form

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Luke Chang [:lchang] (inactive)]

There will be a door hanger prompted to ask users whether or not to save the credit card profile when every time they submit a form with a new credit card information.

There will also be a dropdown with an option "never save credit card details" in the door hanger to allow users to disable the credit card autofill feature entirely.

Comment 1

[Luke Chang [:lchang] (inactive)]

We need to make sure the credit card door hanger comes after the address' one when there are both address fields and credit card fields in the same page and the address saving door hanger hasn't shown before.

Comment 2

[Steve Chung [:steveck]]

Attachment: Bug 1371113 - Part 1: Add credit card enabled pref.

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

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1371113 - Add credit card doorhanger.

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

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

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

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8892785 [details]
Bug 1371113 - Part 1: Add credit card enabled pref.

https://reviewboard.mozilla.org/r/163778/#review169674

Please file another bug to address this part as it needs to be uplifted but the other one doesn't.

I think you could also deal with the telemetry entry in `TelemetryEnvironment.jsm` in the same bug.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

https://reviewboard.mozilla.org/r/163780/#review169678

Are you going to add tests in another commit?

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

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

Comment 8

[Steve Chung [:steveck]]

(In reply to Luke Chang [:lchang] from comment #6)
> Comment on attachment 8892786 [details]
> Bug 1371113 - Add credit card doorhanger.
> 
> https://reviewboard.mozilla.org/r/163780/#review169678
> 
> Are you going to add tests in another commit?

Yes the test part would be added within the patch that actually displays the doorhanger in parent(blocked by bug 1337314). Maybe we can land the existing patch first and create another follow up for showing doorhanger + mochitest.

Comment 9

[Luke Chang [:lchang] (inactive)]

Since the crypto part only affects the last step (saving into storage), I would recommend you to implement the communication part in this bug and let door hanger itself can be tested first. Thus, we can also unblock other works such as metrics.

Comment 10

[Steve Chung [:steveck]]

Hi Juwei, since we'll only have one save button for credit card record saving, is there a clear definition about when to "update" or "save a new one"? For example, we can always "update" if user apply the autofill if data is not exactly match and "create" new" if user entered manually, it should be easiest solution for us. But do we need to consider some cases like:
- User just want to save another new credit card if user apply the autofill but change the number.
- If user entered a card number manually but the last 4 digits matched the existing records(we can't know the complete card number  unless we decrypt first)

Comment 11

[Juwei Huang[:juwei]]

For MVP I would like to keep it as simple as possible, and since we don't show any hint or doorhanger for update/merge cases, I would suggest that if users change anything (no matter they delete the number and input the same number again), we just ask if users want to create a new one.

Comment 12

[Steve Chung [:steveck]]

Attachment: Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

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

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

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

Comment 14

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

https://reviewboard.mozilla.org/r/168198/#review173454

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

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

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

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

Comment 17

[Steve Chung [:steveck]]

Attachment: Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

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

Comment 18

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

https://reviewboard.mozilla.org/r/168198/#review177212

::: browser/extensions/formautofill/FormAutofillParent.jsm:373
(Diff revision 2)
> +    if (state == "disable") {
> +      Services.prefs.setBoolPref("extensions.formautofill.creditCards.enabled", false);
> +      return;
> +    }
> +
> +    await this.profileStorage.creditCards.normalizeCCNumberFields(creditCard.record);

I noticed that it might fail if the format of "cc-number" isn't valid. We should either put "try...catch" here or validate "cc-number" in "createRecords" [1]. I personally prefer the latter so the doorhanger won't show up if it's invalid. What do you think?

BTW, Scott is going to introduce a "isCCNumber" function in FormAutofillUtils in bug 1370764. We can leverage it.


[1] https://dxr.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139/browser/extensions/formautofill/FormAutofillHandler.jsm#520

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163780/diff/5-6/

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

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

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

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

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

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

Comment 23

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

https://reviewboard.mozilla.org/r/171592/#review177884

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:13
(Diff revision 3)
> +                                                       "popupshown");
> +      await ContentTask.spawn(browser, null, async function() {
> +        let form = content.document.getElementById("form");
> +        let name = form.querySelector("#cc-name");
> +        name.focus();
> +        await new Promise(resolve => setTimeout(resolve, 1000));

Why do we need to sleep before "setUserInput"?

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:68
(Diff revision 3)
> +  is(creditCards.length, 1, "Still 1 address in storage");
> +  is(creditCards[0]["cc-name"], "User 1", "Verify the name field");
> +});
> +
> +add_task(async function test_submit_creditCard_never_save() {
> +  // TODO: Verify the menu item action once we clarify the problem.

Please file a bug and mention the bug number here.

::: browser/extensions/formautofill/test/browser/head.js:18
(Diff revision 3)
>  const ENABLED_PREF = "extensions.formautofill.addresses.enabled";
> +const CREDITCARD_PREF = "extensions.formautofill.creditCards.enabled";

They are going to be changed in bug 1370766. You'll need to rebase on it.

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

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

Comment 25

[Steve Chung [:steveck]]

Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Still has unresolved issues in MozReview.

Comment 27

[Luke Chang [:lchang] (inactive)]

Confirmed it was fixed. Thanks.

Comment 28

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4083953a1ec9 -d e58415175f5d: rebasing 415888:4083953a1ec9 "Bug 1371113 - Add credit card doorhanger. r=lchang"
merging browser/extensions/formautofill/locales/en-US/formautofill.properties
rebasing 415889:73a3de36b1e3 "Bug 1371113 - Part 2:Show doorhanger and save/disable credit card. r=lchang"
rebasing 415890:421e1ab0042f "Bug 1371113 - Part 3: Add mochitest for credit card doorhanger. r=lchang" (tip)
merging browser/extensions/formautofill/test/browser/browser.ini
merging browser/extensions/formautofill/test/browser/browser_first_time_use_doorhanger.js
merging browser/extensions/formautofill/test/browser/head.js
warning: conflicts while merging browser/extensions/formautofill/test/browser/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 29

[Steve Chung [:steveck]]

Comment on attachment 8892786 [details]
Bug 1371113 - Add credit card doorhanger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163780/diff/6-7/

Comment 30

[Steve Chung [:steveck]]

Comment on attachment 8896926 [details]
Bug 1371113 - Part 2:Show doorhanger and save/disable credit card.

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

Comment 31

[Steve Chung [:steveck]]

Comment on attachment 8900223 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171592/diff/5-6/

Comment 32

[Steve Chung [:steveck]]

Conflicts resolved.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08e729b36517
Add credit card doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/397bc9636e83
Part 2:Show doorhanger and save/disable credit card. r=lchang
https://hg.mozilla.org/integration/autoland/rev/6819414d32eb
Part 3: Add mochitest for credit card doorhanger. r=lchang

Comment 34

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

Backed out for failing browser_misused_characters_in_strings.js: cancelCreditCardLabel has a misused apostrophe:

https://hg.mozilla.org/integration/autoland/rev/97b5e21ce80f4b430de03a466f452dc74cdbb626
https://hg.mozilla.org/integration/autoland/rev/05b970ff2affbeb7d35ec07b12a56bec2ebd0cc5
https://hg.mozilla.org/integration/autoland/rev/a4f59593c33d7d0a83167fa42cd3c482852215d9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6819414d32eb9e2a293f5c937d3db745237ea26a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126335978&repo=autoland
> TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_misused_characters_in_strings.js | jar:file:///Z:/task_1503894150/build/application/firefox/browser/omni.ja!/chrome/en-US/locale/en-US/formautofill.properties with key=cancelCreditCardLabel has a misused apostrophe. Strings with apostrophes should use foo’s instead of foo's. -

Comment 35

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1371113 - Part 1: Add credit card doorhanger.

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

Comment 36

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1371113 - Part 2: Show doorhanger and save/disable credit card.

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

Comment 37

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

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

Comment 38

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901728 [details]
Bug 1371113 - Part 1: Add credit card doorhanger.

https://reviewboard.mozilla.org/r/173170/#review178492

Comment 39

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901729 [details]
Bug 1371113 - Part 2: Show doorhanger and save/disable credit card.

https://reviewboard.mozilla.org/r/173172/#review178494

Comment 40

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901730 [details]
Bug 1371113 - Part 3: Add mochitest for credit card doorhanger.

https://reviewboard.mozilla.org/r/173174/#review178496

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d4c737cf76d
Part 1: Add credit card doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/a6973ffa879c
Part 2: Show doorhanger and save/disable credit card. r=lchang
https://hg.mozilla.org/integration/autoland/rev/be72dae43ad2
Part 3: Add mochitest for credit card doorhanger. r=lchang

Comment 42

[Luke Chang [:lchang] (inactive)]

I can help on this. Thanks.

Comment 43

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

Backed out for frequently failing browser-chrome's browser/base/content/test/performance/browser_startup.js:

https://hg.mozilla.org/integration/autoland/rev/954af5fafc8abe18deca43a2aa0f31ede6a6a289
https://hg.mozilla.org/integration/autoland/rev/9b07254f54e2e1b6ea2e79493f1594b213c758a4
https://hg.mozilla.org/integration/autoland/rev/0e62978a8c3c0cb07d8c0a0f7d62ca2897529c09

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=be72dae43ad278f0e05c2e60826ec0b4f2a1a86e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126385187&repo=autoland

[task 2017-08-28T09:55:23.736476Z] 09:55:23     INFO - TEST-PASS | browser/base/content/test/performance/browser_startup.js | PlacesCategoriesStarter.js is not allowed before handling user events - 
[task 2017-08-28T09:55:23.752589Z] 09:55:23     INFO - TEST-PASS | browser/base/content/test/performance/browser_startup.js | nsPlacesExpiration.js is not allowed before handling user events - 
[task 2017-08-28T09:55:23.760941Z] 09:55:23     INFO - TEST-INFO | started process screentopng
[task 2017-08-28T09:55:26.025271Z] 09:55:26     INFO - TEST-INFO | screentopng: exit 0
[task 2017-08-28T09:55:26.025717Z] 09:55:26     INFO - Buffered messages logged at 09:55:19
[task 2017-08-28T09:55:26.027471Z] 09:55:26     INFO - Entering test bound 
[task 2017-08-28T09:55:26.027744Z] 09:55:26     INFO - Buffered messages finished
[task 2017-08-28T09:55:26.029388Z] 09:55:26     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup.js | resource:///modules/RecentWindow.jsm is not allowed before handling user events - 
[task 2017-08-28T09:55:26.029601Z] 09:55:26     INFO - Stack trace:
[task 2017-08-28T09:55:26.031378Z] 09:55:26     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_startup.js:null:207

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a4e16c753a9
Part 1: Add credit card doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/2b055a4c047e
Part 2: Show doorhanger and save/disable credit card. r=lchang
https://hg.mozilla.org/integration/autoland/rev/2801212a5855
Part 3: Add mochitest for credit card doorhanger. r=lchang

Comment 45

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

Relanded the patches. The failing test got put into a different test bucket by automation which broke the analytics.

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/1d4c737cf76d
https://hg.mozilla.org/mozilla-central/rev/a6973ffa879c
https://hg.mozilla.org/mozilla-central/rev/be72dae43ad2

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backouts:
https://hg.mozilla.org/mozilla-central/rev/0e62978a8c3c
https://hg.mozilla.org/mozilla-central/rev/9b07254f54e2
https://hg.mozilla.org/mozilla-central/rev/954af5fafc8a

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm unsure exactly where that leaves the state of this bug.

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a4e16c753a9
https://hg.mozilla.org/mozilla-central/rev/2b055a4c047e
https://hg.mozilla.org/mozilla-central/rev/2801212a5855

Comment 50

[Steve Chung [:steveck]]

Hi Wes, may I know the problem of this patch right now? It seems you backed out and reland the patch again.

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment 47 was the merge of backout commits from comment 43.

Comment 49 is the merge of the re-landed commits from comment 44.

As far as I can tell, that means that these patches are officially part of mozilla-central.