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

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P3)

Description

[Ton [:Tonnes]]

Attachment: 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.

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

Increased the dialog width and made minor adjustments so that the buttons can fit longer strings.

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

Comment 2

[Scott Wu [:scottwu]]

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 3

[Luke Chang [:lchang] (inactive)]

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

Comment 4

[Francesco Lodolo [:flod]]

Is it possible to make these widths proper CSS rules, and make them localizable instead of hard coding them?

Comment 5

[Luke Chang [:lchang] (inactive)]

Form Autofill is only enabled on Fx58 en-US build so set firefox58:unaffected.

Comment 6

[Scott Wu [:scottwu]]

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.

Comment 7

[Francesco Lodolo [:flod]]

(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 8

[Scott Wu [:scottwu]]

Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

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

Comment 9

[Francesco Lodolo [:flod]]

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

Comment 10

[Scott Wu [:scottwu]]

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 11

[Scott Wu [:scottwu]]

Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

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

Comment 12

[Francesco Lodolo [:flod]]

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.

Comment 13

[Scott Wu [:scottwu]]

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 14

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 15

[Scott Wu [:scottwu]]

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.

Comment 16

[Scott Wu [:scottwu]]

Comment on attachment 8928057 [details]
Bug 1413162 - Set width to manageAddresses and manageCreditCards dialogs and move button styles to common stylesheets.

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

Comment 17

[Scott Wu [:scottwu]]

Thanks :lchang, :flod, and :jaws. Rebased to central and checking it in.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/224bb68f1fc9

Comment 20

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.