1662536 - Wrong margin end in menulist elements inside subdialogs

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2020-09-01 11-46-39.png

Menulist elements inside subdialog have a margin-inline-end attribute set to 0, which causes inline elements to look attached.

Here's the CSS culprit: https://searchfox.org/comm-central/rev/78a165ed474d0278d4c23c947929d4bf17d6c593/mail/themes/shared/mail/preferences/dialog.css#78-82

This wasn't edited so I'm not sure what caused this regression.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1662536-font-dialog spacing.patch

The rule you pointed is used to align the buttons, menulists etc. with the dialog buttons ("OK", "Cancel" etc.).

I removed the :not([dlgtype]) because it is no more needed with the dialog buttons residing in shadow-DOM.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: 1662536-font-dialog spacing.patch

Doing a qrefresh before uploading would be better.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9173465 [details] [diff] [review]
1662536-font-dialog spacing.patch

Review of attachment 9173465 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff.
This new class should be added also to:
- The `id="totalOpenTimeEnd"` element in the Customize New Mail Alert dialog.
- The `id="expire-months-label"` in the Change Key Expiration dialog.

I only found these other places where this class is necessary, not sure if I missed something.

::: mail/themes/shared/mail/preferences/dialog.css
@@ +91,5 @@
>  textbox.tree-input {
>    font-size: unset;
>  }
>  
> +/* Give some space after the first menulist column in Fonts dialog */

This is not only for the fonts dialog, but also for the customize alert, and openpgp key prefs.
Since it's a generic class I'm sure we will use it more often.

Comment 4

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #3)

This is not only for the fonts dialog, but also for the customize alert, and
openpgp key prefs.

I don't know which OpenPGP dialog. Can you attach a screenshot of the dialog?

Comment 5

[Alessandro Castellani [:aleca]]

Account Settings > e2e encryption > "Change expiration date" on an openPGP key.
https://searchfox.org/comm-central/rev/115cf24c0971773154a93009eb41a760a1cbeec9/mail/extensions/openpgp/content/ui/changeExpiryDlg.xhtml#52

Comment 6

[Richard Marti (:Paenglab)]

Attachment: 1662536-dialog-label-spacing.patch

Using class="startSpacing" now.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9173688 [details] [diff] [review]
1662536-dialog-label-spacing.patch

Review of attachment 9173688 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
I think we should update the commit message with:
"Bug 1662536 - Implement startSpacing class to add margins to inline menulist columns. r=aleca"

::: mail/components/preferences/fonts.xhtml
@@ +94,5 @@
>              </menupopup>
>            </menulist>
>          </hbox>
>          <hbox align="center" pack="end">
> +          <label control="sizeVar" data-l10n-id="font-size-label" class="startSpacing"/>

nit: put the class on its own line to respect the 80ch limit.

@@ +157,5 @@
>          <hbox>
>            <menulist id="monospace" flex="1" style="width: 0px;" crop="right" delayprefsave="true"/>
>          </hbox>
>          <hbox align="center" pack="end">
> +          <label control="sizeMono" data-l10n-id="font-size-monospace-label" class="startSpacing"/>

nit: same here

::: mail/themes/shared/mail/preferences/dialog.css
@@ +91,5 @@
>  textbox.tree-input {
>    font-size: unset;
>  }
>  
> +/* Give some space after the first menulist column in Fonts dialog */

nit: remove the "in Fonts dialog" part since this class is used in other locations.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: 1662536-dialog-label-spacing.patch

I've changed the CSS comment to:
/* Give some space in front of elements that follows a menulist, button or an input in dialogs */

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4d06b6fa75b9
Implement startSpacing class to add margins to inline menulist columns. r=aleca

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9173702 [details] [diff] [review]
1662536-dialog-label-spacing.patch

[Approval Request Comment]
User impact if declined: labels too near to their antecedent elements in dialogs
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9173702 [details] [diff] [review]
1662536-dialog-label-spacing.patch

[Triage Comment]
Approved for beta
Approved for ers78

Comment 12

[Rob Lemley [:rjl]]

Thunderbird 81.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/0bbe3811dd79

Comment 13

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/b3b129f40271