[Form Autofill] Implement credit card dropdown style

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: lchang, Assigned: ralin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [form autofill:M4][ETA:8/2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
The credit card autofill dropdown is slightly different from address' one. There will be a small credit card icon on the left and the number will be shown with some digits masked.
(Assignee)

Comment 1

10 months ago
Add CC visual spec: https://www.dropbox.com/sh/oe31pxe975csniw/AADd8iMRRFMW-E1VrhVV1k6Za?dl=0
Assignee: nobody → ralin
Status: NEW → ASSIGNED
(Assignee)

Updated

9 months ago
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:8/2]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review166674

::: browser/extensions/formautofill/content/credit-card-icon.svg:2
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 21.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->

remind meself to clean up unnecessary attr and comment.

::: browser/extensions/formautofill/content/formautofill.xml:84
(Diff revision 2)
>    </binding>
>  
>    <binding id="autocomplete-profile-listitem" extends="chrome://formautofill/content/formautofill.xml#autocomplete-profile-listitem-base">
>      <xbl:content xmlns="http://www.w3.org/1999/xhtml">
> -      <div anonid="autofill-item-box" class="autofill-item-box">
> +      <div anonid="autofill-item-box" class="autofill-item-box" xbl:inherits="ac-image">
>          <div class="profile-label-col profile-item-col">

should add a separate span for **** for it has its own style
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

9 months ago
Bug 1371149 has already added some utils for testing that's also required in this bug, so let's save our time not duplicate those works before bug 1371149 landed, and I'll add Part 2 patch about the browser chrome test then. Thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

9 months ago
mozreview-review
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review171570

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:273
(Diff revision 6)
>      super(...args);
>    }
>  
> +  _fmtMaskedCreditCardLabel(maskedCCNum = "") {
> +    if (!/^(\*+)\d{4}$/.test(maskedCCNum)) {
> +      return maskedCCNum;

I am not sure if I understand this function correctly.

It looks like parsing the input string and converting to an object in {affix, label} for the normal case.

This `return` seems try to find the mismatch case, but why does it return the original string?

Is Both `affix` and `label` got `undefined` what you expected in this example?
```JS
let {affix, label} = this._fmtMaskedCreditCardLabel("123123");
```

That's great to have a comment to explain the behavior or give an example here.
Attachment #8889919 - Flags: review?(selee)
(Assignee)

Comment 12

9 months ago
mozreview-review-reply
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review171570

Thanks for the rapid review :)

> I am not sure if I understand this function correctly.
> 
> It looks like parsing the input string and converting to an object in {affix, label} for the normal case.
> 
> This `return` seems try to find the mismatch case, but why does it return the original string?
> 
> Is Both `affix` and `label` got `undefined` what you expected in this example?
> ```JS
> let {affix, label} = this._fmtMaskedCreditCardLabel("123123");
> ```
> 
> That's great to have a comment to explain the behavior or give an example here.

Good point, I didn't think through at that point, now I look back to the code and the early return seems super irrational... I think we could just remove the regx check since the result is directly grabbed from Storage, and it should be reliable enough.

Comment 13

9 months ago
mozreview-review
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review171642

::: browser/extensions/formautofill/skin/shared/autocomplete-item.css:25
(Diff revision 6)
>    --col-spacer: 7px;
>    --item-width: calc(50% - (var(--col-spacer) / 2));
> -  --item-text-color: -moz-FieldText;
> +  --label-text-color: #262626;
> +  --comment-text-color: #646464;
> +  --warning-text-color: #646464;
> +  --option-btn-text-color: -moz-FieldText;

May I know how do you determine which value should be  defined as a variable?

Is this for the different OS or the different themes (OS or FX)?

Comment 14

9 months ago
mozreview-review
Comment on attachment 8894811 [details]
Bug 1371139 - Part 2. Add browser chrome test to verify dropdown layout.

https://reviewboard.mozilla.org/r/165946/#review171644

This patch looks good! I will review it again when all parts are ready. Thank you.

::: browser/extensions/formautofill/test/browser/browser_dropdown_layout.js:38
(Diff revision 1)
> +    is(firstItem.getAttribute("ac-image"), "", "Should not show icon");
> +
> +    // The breakpoint of two-lines layout is 150px
> +    await reopenPopupWithResizedInput(browser, focusInput, 140);
> +    is(firstItem._itemBox.getAttribute("size"), "small", "Show two-lines layout");
> +    await reopenPopupWithResizedInput(browser, focusInput, 160);

Could you change the pixel value to exact 150px here? e.g. 
151px for one-line layout,
150px for two-line layout

This makes the test more robust to verify the value that you defined in the code.

::: browser/extensions/formautofill/test/browser/browser_dropdown_layout.js:56
(Diff revision 1)
> +    isnot(firstItem.getAttribute("ac-image"), "", "Should show icon");
> +
> +    // The breakpoint of two-lines layout is 185px
> +    await reopenPopupWithResizedInput(browser, focusInput, 175);
> +    is(firstItem._itemBox.getAttribute("size"), "small", "Show two-lines layout");
> +    await reopenPopupWithResizedInput(browser, focusInput, 195);

as above.
Attachment #8894811 - Flags: review?(selee)
(Assignee)

Comment 15

9 months ago
mozreview-review-reply
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review171642

> May I know how do you determine which value should be  defined as a variable?
> 
> Is this for the different OS or the different themes (OS or FX)?

I categorize those variable into three groups:
1. layout related: mainly reduce the duplicate rules for two different layouts(1-line, 2-lines)
2. font-size: the font size values are for we'll override the size in platform-specified stylesheet, and the refactorying here can help out if we gonna change the size of any labels in the future.
3. color: color variables are for high contrast mode in Windows(or maybe other OS). Similarly, this also benefits our maintainability.
(Assignee)

Comment 16

9 months ago
mozreview-review-reply
Comment on attachment 8894811 [details]
Bug 1371139 - Part 2. Add browser chrome test to verify dropdown layout.

https://reviewboard.mozilla.org/r/165946/#review171644

Thanks \o/

I'm still fixing the insecure warning now, I'll rebase and fix the issues once bug 1371149 landed.

> Could you change the pixel value to exact 150px here? e.g. 
> 151px for one-line layout,
> 150px for two-line layout
> 
> This makes the test more robust to verify the value that you defined in the code.

I was considering doing that at beginning, however, I found the border of input would affect the final computed size of our dropdown width. For instance, at present, the default margin and border of input are making input around 8px wider than the given width, that is if we set 150px to input, we got 158px for the real dropdown width output. So, instead of setting a precise number, I think 10px is a reasonable buffer for in case the default input style changed in the future. We probably want to lower the risk to be a surprise then.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

8 months ago
mozreview-review
Comment on attachment 8894811 [details]
Bug 1371139 - Part 2. Add browser chrome test to verify dropdown layout.

https://reviewboard.mozilla.org/r/165946/#review175686

LGTM! Thank you.
Attachment #8894811 - Flags: review?(selee) → review+

Comment 20

8 months ago
mozreview-review
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review175688

LGTM! Thank you.
Attachment #8889919 - Flags: review?(selee) → review+
(Reporter)

Comment 21

8 months ago
mozreview-review
Comment on attachment 8889919 [details]
Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.

https://reviewboard.mozilla.org/r/160976/#review175700

Carry over Sean's r+.
Attachment #8889919 - Flags: review?(lchang) → review+
(Reporter)

Comment 22

8 months ago
mozreview-review
Comment on attachment 8894811 [details]
Bug 1371139 - Part 2. Add browser chrome test to verify dropdown layout.

https://reviewboard.mozilla.org/r/165946/#review175702

Carry over Sean's r+.
Attachment #8894811 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

8 months ago
Thanks for the review :)


Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bd618c0879702f33b8373192a1394184922d073
Keywords: checkin-needed

Comment 27

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8209aae88ad9
Part 1. Implementation of form auto fill credit card layout. r=lchang,seanlee
https://hg.mozilla.org/integration/autoland/rev/4a092af42641
Part 2. Add browser chrome test to verify dropdown layout. r=lchang,seanlee
Keywords: checkin-needed

Comment 28

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8209aae88ad9
https://hg.mozilla.org/mozilla-central/rev/4a092af42641
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.