checkbox for "Save credit card to Firefox" should be unchecked by default outside private browsing

VERIFIED FIXED in Firefox 63

Status

()

enhancement
P1
normal
VERIFIED FIXED
11 months ago
11 months ago

People

(Reporter: jgong, Assigned: sfoster)

Tracking

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

(Whiteboard: [webpayments] [user-testing])

Attachments

(1 attachment)

Reporter

Description

11 months ago
Checkbox for "Save credit card to Firefox" should be unchecked by default outside private browsing mode.
Assignee

Comment 1

11 months ago
So we want it unchecked always? We currently explicitly check it by default for non-private browsing - and have tests to verify. Its any easy change, I just want to clarify as it reverses our earlier decision when we first implemented this.
Yeah, we discussed this a few times and it was a recommendation from the research earlier this year. We can double-check with UX and UR that this is still recommended given our newer shopping research.
Flags: needinfo?(sbautista)
Flags: needinfo?(jsavory)
Whiteboard: [webpayments][triage] → [webpayments] [triage] [user-testing]
Yup, Matt is correct. The research provided a clear recommendation for the credit cards to not have them saved by default in any scenario. But we decided to keep the shipping address to be saved by default as there was less concern around it.
Flags: needinfo?(jsavory)
What Jacqueline said.
Flags: needinfo?(sbautista)
Whiteboard: [webpayments] [triage] [user-testing] → [webpayments] [user-testing]
Assignee

Comment 5

11 months ago
The plan is to create a new pref to set the default for the checked-ness of the save credit card form (defaulting to false) and to keep the test structure we have to allow testing different defaults.
Assignee: nobody → sfoster

Updated

11 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1
Flags: qe-verify+
QA Contact: hani.yacoub
Comment hidden (mozreview-request)

Comment 7

11 months ago
mozreview-review
Comment on attachment 8996488 [details]
Bug 1477106 - Use a pref to set default checkedness for "Save card to Firefox" and "Save address to Firefox" checkboxes.

https://reviewboard.mozilla.org/r/260562/#review267876

::: browser/components/payments/res/containers/address-form.js:149
(Diff revision 1)
>      } else {
> -      // Adding a new record: default persistence to checked when in a not-private session
> +      let defaults = PaymentDialogUtils.getDefaultPreferences();
> +      // Adding a new record: default persistence to the pref value when in a not-private session
>        this.persistCheckbox.hidden = false;
> -      this.persistCheckbox.checked = !state.isPrivate;
> +      this.persistCheckbox.checked = state.isPrivate ? false :
> +                                                     defaults.saveAddressDefaultChecked;

nit, indent the `defaults` to line up with the `false`.

::: browser/components/payments/res/containers/basic-card-form.js:159
(Diff revision 1)
>        }
> -      // Adding a new record: default persistence to checked when in a not-private session
> +
> +      let defaults = PaymentDialogUtils.getDefaultPreferences();
> +      // Adding a new record: default persistence to pref value when in a not-private session
>        this.persistCheckbox.hidden = false;
> -      this.persistCheckbox.checked = !state.isPrivate;
> +      if ("persistCheckboxValue" in basicCardPage) {

Can you check `hasOwnProperty` like you did elsewhere in the patch?

::: browser/components/payments/res/containers/basic-card-form.js:164
(Diff revision 1)
> -      this.persistCheckbox.checked = !state.isPrivate;
> +      if ("persistCheckboxValue" in basicCardPage) {
> +        // returning to this page, use previous checked state
> +        this.persistCheckbox.checked = basicCardPage.persistCheckboxValue;
> +      } else {
> +        this.persistCheckbox.checked = state.isPrivate ? false :
> +                                                       defaults.saveCreditCardDefaultChecked;

ditto regarding the indent

::: browser/components/payments/test/browser/browser_address_edit.js:52
(Diff revision 1)
>        is(Object.keys(savedAddresses).length, 1, "1 saved address at the start of test");
>      });
>  
>      let testOptions = [
> -      { isTemporary: false, expectPersist: true },
> -      { isTemporary: true, expectPersist: false },
> +      { setPersistCheckedValue: true, isTemporary: false, expectPersist: true },
> +      { setPersistCheckedValue: false, isTemporary: true, expectPersist: false },

I don't see where `setPersistCheckedValue` or `isTemporary` are used in this test.

::: browser/components/payments/test/browser/browser_card_edit.js:166
(Diff revision 1)
> -    await fillInCardForm(frame, PTU.BasicCards.JaneMasterCard, {
> +    cardOptions = Object.assign({}, {
> -      isTemporary: aOptions.isPrivate,
>        checkboxSelector: "basic-card-form .persist-checkbox",
> +      expectPersist: aOptions.expectCardPersist,
>      });
> +    // if (aOptions.hasOwnProperty("setCardPersistCheckedValue")) {

Did you intend to delete this?
Attachment #8996488 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 9

11 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d69e19bad6c1
Use a pref to set default checkedness for "Save card to Firefox" and "Save address to Firefox" checkboxes. r=jaws
Backe dout for eslint failure at builds/worker/checkouts/gecko/browser/components/payments/content/paymentDialogFrameScript.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d69e19bad6c19ed1c33dafc72f7b13ad40c079e3

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191475676&repo=autoland&lineNumber=309

Backout: https://hg.mozilla.org/integration/autoland/rev/53d5379ec145914f875662cbbeb77ddf5c05769f
Flags: needinfo?(sfoster)
Assignee

Comment 11

11 months ago
I can see that 'Services' is global here, and its not in the eslint-env mozilla/frame-script globals list, or defined in our  .eslintrc.js. But oddly `mach eslint browser/components/payments/content/paymentDialogFrameScript.js` doesn't flag any errors or warnings. I can add it, but I'd like to understand the difference between the automation and my local environment as I trust in mach eslint to keep me honest.
Flags: needinfo?(sfoster)
Comment hidden (mozreview-request)
Assignee

Comment 13

11 months ago
(In reply to Sam Foster [:sfoster] from comment #11)
> browser/components/payments/content/paymentDialogFrameScript.js` doesn't
> I'd like to understand the
> difference between the automation and my local environment as I trust in
> mach eslint to keep me honest.

With another rebase I did get the eslint error. No mysteries here.
Comment hidden (mozreview-request)

Comment 16

11 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62864390f824
Use a pref to set default checkedness for "Save card to Firefox" and "Save address to Firefox" checkboxes. r=jaws

Comment 17

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/62864390f824
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

Comment 18

11 months ago
Hi Sam,

I tried to verify the fix on the latest Nightly (Build ID: 20180802220056) by checking the default state of the checkboxes and also changing them via the newly added preferences.

The result is that both checkboxes will be unchecked regardless of the changes made in about:preferences. They are unchecked by default (when opening the "Add CC/Address" screen) and after switching the preference to true or false. I also restarted the browser to be sure that the changes are properly saved.
Flags: needinfo?(sfoster)
Assignee

Comment 19

11 months ago
(In reply to Timea Babos from comment #18)
> Hi Sam,
> 
> I tried to verify the fix on the latest Nightly (Build ID: 20180802220056)
> by checking the default state of the checkboxes and also changing them via
> the newly added preferences.
> 
> The result is that both checkboxes will be unchecked regardless of the
> changes made in about:preferences. They are unchecked by default (when
> opening the "Add CC/Address" screen) and after switching the preference to
> true or false. I also restarted the browser to be sure that the changes are
> properly saved.

Thanks for catching this. I can confirm this with a build from central today. But oddly the automated tests are passing and I had tried to take care to test different pref values and checking/not-checking this checkbox to verify exactly this. I'm investigating...
Flags: needinfo?(sfoster)
Assignee

Updated

11 months ago
Depends on: 1480880
Assignee

Comment 20

11 months ago
I've filed bug 1480880 for this. Lets try verifying this once I have a patch landed for that bug.

Comment 21

11 months ago
Verified - Fixed on the latest Firefox Nightly 63.0a1 (2018-08-09) on Windows 10 x64, Windows 7 x64, Ubuntu 16.04 and Mac 10.13.

Since the fix for Bug 1480880 is verified, this issue is also verified - fixed.
The checkbox for "Save credit card" is "false" by default in both normal and private browsing as well.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.