Closed Bug 1371139 Opened 6 years ago Closed 6 years ago

[Form Autofill] Implement credit card dropdown style

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: lchang, Assigned: ralin)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

Bug 1371139 - Part 1. Implementation of form auto fill credit card layout.
59 bytes, text/x-review-board-request
lchang
: review+
selee
: review+
Details
Bug 1371139 - Part 2. Add browser chrome test to verify dropdown layout.
59 bytes, text/x-review-board-request
lchang
: review+
selee
: review+
Details 
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.
Add CC visual spec: https://www.dropbox.com/sh/oe31pxe975csniw/AADd8iMRRFMW-E1VrhVV1k6Za?dl=0
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:8/2]
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
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 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)
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 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 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)
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.
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 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 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+
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+
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+
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
https://hg.mozilla.org/mozilla-central/rev/8209aae88ad9
https://hg.mozilla.org/mozilla-central/rev/4a092af42641
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.