1655080 - Make the Tag- and Sanitize dialog themeable

Status: RESOLVED FIXED

(Thunderbird :: Theme, task)

Description

[Richard Marti (:Paenglab)]

The new tag and the sanitize dialogs aren't themeable.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: and added some new labels

I changed also the appearance

• tag dialog: colour picker below the textbox and added the label "Color"
• sanitize dialog: removed the expander as with only three elements it's not needed and gave them a new title "History" like FX has.

Both changes aren't compatible for 78. What do you think should we do? Only add the themeability? Or add the changes but without labels?

Comment 2

[Richard Marti (:Paenglab)]

For the tag dialog I could use https://searchfox.org/comm-central/rev/1129249f43acb470071711b9c25b8f3743ab8f97/mail/locales/en-US/messenger/preferences/preferences.ftl#392

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 1655080-tag-sanitize-themeable.patch

In tag dialog I had to change the width to min-width. Found this while trying to convert the patch for TB 78.

I also found, that the German translation uses instead of only "Label" it uses "Label and correlating Color". This makes it a bit tricky to add a label with "Color".

I also found a bug on https://searchfox.org/comm-central/rev/1129249f43acb470071711b9c25b8f3743ab8f97/mail/locales/en-US/messenger/preferences/preferences.ftl#392. It has set .label instead of .value which ends in not showing the label.

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9165965 [details] [diff] [review]
1655080-tag-sanitize-themeable.patch

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

Thanks for this, just some nits.

There's a dash after the Tag in the commit message, is that intentional?

I don't think we can uplift this due to string changes, right?

::: mail/base/content/newTagDialog.xhtml
@@ +28,5 @@
>    <linkset>
>      <html:link rel="localization" href="messenger/preferences/new-tag.ftl"/>
>    </linkset>
>  
> +  <box style="display: grid; grid-template-columns: auto 1fr;">

Add an `align-items: center;` here otherwise the Tag Name label will be top aligned.
Wouldn't be better to have this as a class instead of inline?

::: mail/base/content/sanitize.xhtml
@@ +69,5 @@
>          <spacer flex="1"/>
>        </vbox>
>  
> +    <label id="historyGroupLabel" value="&historyGroup.label;"/>
> +    <vbox id="historyGroup">

You could add a class indent for the checkboxes, or at least align them to the label above if you think the indentation is too much.

::: mail/base/content/sanitizeDialog.css
@@ +38,5 @@
> +#historyGroupLabel {
> +  margin-block: 16px 4px;
> +  margin-inline-start: 4px;
> +  font-size: 1.14em;
> +  font-weight: bold;

Let's go with a weight of 600. We're not in the preferences so the full bold it's a bit too aggressive.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: 1655080-tag-sanitize-themeable[checked-in].patch

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

Comment on attachment 9165965 [details] [diff] [review]
1655080-tag-sanitize-themeable.patch

Review of attachment 9165965 [details] [diff] [review]:

I don't think we can uplift this due to string changes, right?

No, I'll do a patch without the string changes.

::: mail/base/content/newTagDialog.xhtml

• <box style="display: grid; grid-template-columns: auto 1fr;">

Add an align-items: center; here otherwise the Tag Name label will be top
aligned.
Wouldn't be better to have this as a class instead of inline?

I added the align-items. I applied the style directly because this is a small dialog without its own stylesheet.

::: mail/base/content/sanitize.xhtml
@@ +69,5 @@

     <spacer flex="1"/>
   </vbox>

• <label id="historyGroupLabel" value="&historyGroup.label;"/>
• <vbox id="historyGroup">

You could add a class indent for the checkboxes, or at least align them to
the label above if you think the indentation is too much.

Indented 10px.

::: mail/base/content/sanitizeDialog.css
@@ +38,5 @@

+#historyGroupLabel {

• margin-block: 16px 4px;
• margin-inline-start: 4px;
• font-size: 1.14em;
• font-weight: bold;

Let's go with a weight of 600. We're not in the preferences so the full bold
it's a bit too aggressive.

Done.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/eeb76bd9e147
Make the Tag and Sanitize dialog themeable. r=aleca

Comment 7

[Richard Marti (:Paenglab)]

Under https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4abe8d4112073c53f4de199304b2fa1c8ce2ba2c you can find a beta build with a special patch without the string changes and without the reorganization of the tag dialog.

What do you think, can we go this way for beta and ESR?

Comment 8

[Alessandro Castellani [:aleca]]

Can you upload that patch here and ping me for a review?
So I can load it on my local 78 version and test it.

Comment 9

[Alessandro Castellani [:aleca]]

Reopening this as we need a 78 variation of this patch without string changes.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: 1655080-tag-sanitize-themeable-beta.patch

Patch for beta.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: 1655080-tag-sanitize-themeable-ESR.patch

Patch for ESR

Comment 12

[Richard Marti (:Paenglab)]

Attachment: German-dialogs-ESR.png

How it looks on a German ESR.

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 9166110 [details] [diff] [review]
1655080-tag-sanitize-themeable-ESR.patch

[Approval Request Comment]
User impact if declined: lesser integration of dialogs on dark systems
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 9166109 [details] [diff] [review]
1655080-tag-sanitize-themeable-beta.patch

With today's uplift is no beta version needed.

Comment 15

[Wayne Mery (:wsmwk)]

Will take this in 78.1.1 or later, after the patch has gone through beta

Comment 16

[Wayne Mery (:wsmwk)]

Per conversation with Paenglab, we'll take all further theme changes in one shot in 78.2.0

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9166110 [details] [diff] [review]
1655080-tag-sanitize-themeable-ESR.patch

[Triage Comment]
Approved for esr78

Comment 18

[Geoff Lankow (:darktrojan)]

Thunderbird 78.2.0:
https://hg.mozilla.org/releases/comm-esr78/rev/3770cff82ba9