Closed Bug 1413162 Opened 2 years ago Closed 2 years ago

[Form Autofill] "Remove" button for Saved Credit Cards may not fit dialog

Categories

(Toolkit :: Form Autofill, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox58 --- unaffected
firefox59 --- verified

People

(Reporter: Tonnes, Assigned: scottwu)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:V2])

Attachments

(2 files)

Attached image Remove_SCC.png
Localized text for "Remove" on the left button in the Saved Credit Cards dialog may not fit, or rather: the dialog is too narrow to make the left button display fully (the text does seem to fit the button.) See the screenshot for nl.
Whiteboard: [form autofill:V2]
Component: Form Manager → Form Autofill
Assignee: nobody → scwwu
When the strings on the buttons are long, naturally they would be pushed out of frame or wrapped, both of which occurred here. The simplest fix is to increase the dialog width to avoid this from happening, and UX agrees with this approach.

This situation occurs more easily on Windows specifically because the default dialog width is smaller on Windows. In the patch I specified a width on the dialogs (just like on editAddress and editCreditCard).
Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

https://reviewboard.mozilla.org/r/199282/#review204420
Attachment #8928057 - Flags: review?(lchang) → review+
Is it possible to make these widths proper CSS rules, and make them localizable instead of hard coding them?
Form Autofill is only enabled on Fx58 en-US build so set firefox58:unaffected.
I didn't know making the width localizable is an option, but yes it's possible. I've also found that the button paddings in the dialog are bigger than they should be. If we fix this we don't even need to change the dialog width.

I guess it's still a good thing to have the width localizable, in case there's another locale that needs even more space.
(In reply to Scott Wu [:scottwu] from comment #6)
> I guess it's still a good thing to have the width localizable, in case
> there's another locale that needs even more space.

A couple of examples
https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/browser/preferences/colors.dtd
https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/browser/preferences/preferences.dtd
Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

https://reviewboard.mozilla.org/r/199282/#review204422

::: browser/extensions/formautofill/locales/en-US/formautofill.properties:95
(Diff revision 2)
>  showCreditCardsBtnLabel = Show Credit Cards
>  hideCreditCardsBtnLabel = Hide Credit Cards
>  removeBtnLabel = Remove
>  addBtnLabel = Add…
>  editBtnLabel = Edit…
> +# LOCALIZATION NOTE (manageDialogsWidth): Optionally set the width of manageAddresses and manageCreditCards in px.

Suggested comment: "This strings sets the default width for windows used to manage addresses and credit cards."

::: browser/extensions/formautofill/locales/en-US/formautofill.properties:96
(Diff revision 2)
>  hideCreditCardsBtnLabel = Hide Credit Cards
>  removeBtnLabel = Remove
>  addBtnLabel = Add…
>  editBtnLabel = Edit…
> +# LOCALIZATION NOTE (manageDialogsWidth): Optionally set the width of manageAddresses and manageCreditCards in px.
> +manageDialogsWidthInPx = 560

Is there any chance to turn this into a CSS rule (width: 560px), or value (560px)?

Tools will catch errors in strings that are supposed to be valid CSS values, we won't have much control over a numeric string like 560.

In case, the ID should be manageDialogsWidth
Attachment #8928057 - Flags: review?(francesco.lodolo)
Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

https://reviewboard.mozilla.org/r/199282/#review204422

> Is there any chance to turn this into a CSS rule (width: 560px), or value (560px)?
> 
> Tools will catch errors in strings that are supposed to be valid CSS values, we won't have much control over a numeric string like 560.
> 
> In case, the ID should be manageDialogsWidth

It's possible except it would only work with px as the unit, and not em or rem. I was worried that localizers will try to use em and break the dialog (the dialog and the frame size would not match).
Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

https://reviewboard.mozilla.org/r/199282/#review205372

Thanks, this works for me.
Attachment #8928057 - Flags: review?(francesco.lodolo) → review+
Attachment #8928057 - Flags: review?(jaws)
I moved a few HTML style rules to common css, so that they would match XUL styling. Jared, could you take a quick look? Thanks!
Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

https://reviewboard.mozilla.org/r/199282/#review206084

::: browser/extensions/formautofill/content/manageDialog.css
(Diff revision 3)
>  
>  fieldset > legend {
>    box-sizing: border-box;
>    width: 100%;
>    padding: 0.4em 0.7em;
> -  font-size: 0.9em;

These font-sizes are being removed but I don't see any mention of why in the bug or the patch. Was this intentional?
Attachment #8928057 - Flags: review?(jaws) → review+
Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

https://reviewboard.mozilla.org/r/199282/#review206084

> These font-sizes are being removed but I don't see any mention of why in the bug or the patch. Was this intentional?

Yes it was intentional. The font-sizes for xhtml documents loaded in subdialogs had different sizes than similar xul components so I had to tweak them to make them look consistent with the rest of preferences. However after Bug 1392532 landed, which fixed the font size problems, the rules became counterproductive.
Thanks :lchang, :flod, and :jaws. Rebased to central and checking it in.
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/224bb68f1fc9
Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets. r=flod,jaws,lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/224bb68f1fc9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
No longer blocks: 1419819
Depends on: 1419819
I have reproduced this issue using Firefox  58.0a1 (2017.10.30) nl-build on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 59.0b6, nl-build on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.2.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.