1655568 - Make the Compose dialogs themeable

Status: VERIFIED FIXED

(Thunderbird :: Theme, task)

Description

[Richard Marti (:Paenglab)]

In composer we have a lot of dialogs that aren't themeable.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1655568-compose-dialogs-themeable.patch

This should be all composer dialogs.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9166394 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

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

Great work, thanks for taking care of all these dialogs.

A couple of things I noticed:
- The buttons don't have the usual border radius, is that on purpose?
- All the dialogs open at the top left corner of the screen, is there anything we can do about it?

Also, the color picker dialog is very strange in the current state and I think we should update it slightly.
The preview box is misleading as it seems like a clickable element. Do we need that or can we consistently rely on the color button itself to show a color preview?
The color button has a different style from the one you made for the Folder Properties.
We should better align all those elements and make this dialog more digestible.
I'll attach a quick mock-up for it and you let me know if it's doable.

::: mail/components/compose/content/dialogs/EdInsertTable.xhtml
@@ +74,5 @@
>        </html:tr>
>      </html:table>
>      <spacer class="spacer"/>
>    </html:fieldset>
>    <spacer class="spacer"/>

Remove this spacer and also the one above since are not really necessary.

@@ +86,2 @@
>                  oninput="forceInteger(this.id)" />
>      <label value="&pixels.label;"/>

It would be nice if also the border input could be part of the table above.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: color dialog.png

What do you think about this?
The smaller info text would use the .tip-caption class we implemented in the account manager.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: 1655568-compose-dialogs-themeable.patch

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

Comment on attachment 9166394 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

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

Great work, thanks for taking care of all these dialogs.

A couple of things I noticed:

• The buttons don't have the usual border radius, is that on purpose?

Fixed the button border radius. This was because this dialogs don't load messenger.css and didn't get the variable. Added now the variable into themeableDialog.css.

Also, the color picker dialog is very strange in the current state and I
think we should update it slightly.
The preview box is misleading as it seems like a clickable element. Do we
need that or can we consistently rely on the color button itself to show a
color preview?

I updated the layout of the colour picker dialog. I commented the calls to ColorSwatch out to not get errors. Can you now adapt them to apply the colour from the input field to the ColorPicker?

::: mail/components/compose/content/dialogs/EdInsertTable.xhtml
@@ +74,5 @@

   </html:tr>
 </html:table>
 <spacer class="spacer"/>

</html:fieldset>
<spacer class="spacer"/>

Remove this spacer and also the one above since are not really necessary.

Removed

@@ +86,2 @@

             oninput="forceInteger(this.id)" />
 <label value="&pixels.label;"/>

It would be nice if also the border input could be part of the table above.

Moved to the table. I removed the fieldset and the title as this isn't needed here.

I also made changes to the EdTableProps.xhtml because some aligning was off and the previous/next buttons used additional images and labels.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9166787 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

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

Looks good.
Adding the `SetColorSwatch` method.

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1655568-compose-dialogs-themeable.patch

Patch updated.
Can you give it a try?

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 9166875 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

This works for the hash colours but when I enter blue like the example text shows, the ColorPicker doesn't change to blue. When closing the dialog, the blue is chosen.

Comment 8

[Alessandro Castellani [:aleca]]

Ah, I forgot about that, you're right, sorry.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1655568-compose-dialogs-themeable.patch

Putting back the ColorPickerSwatch since we don't want to handle a massive array to match color strings to hex values.
I updated the style of the swatch to better distinguish it from the color button.
What do you think?

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9166918 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

Yes, the best we can do.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: 1655568-compose-dialogs-themeable.patch

Patch based on Alessandro's last patch with rounded LastPickedColor bubble.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9167112 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

[Approval Request Comment]
User impact if declined: lessintegrated composer dialogs in dark mode
Testing completed (on c-c, etc.): almost on c-c
Risk to taking this patch (and alternatives if risky): low

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/be017833e747
Make the Compose dialogs themeable. r=aleca

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9167112 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

[Triage Comment]
Approved for beta

Comment 15

[Wayne Mery (:wsmwk)]

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

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 80.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/31389f5baedd

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9167112 [details] [diff] [review]
1655568-compose-dialogs-themeable.patch

[Triage Comment]
Approved for esr78

Comment 18

[Geoff Lankow (:darktrojan)]

Thunderbird 78.2.0:
https://hg.mozilla.org/releases/comm-esr78/rev/5882989e6ad2

Comment 19

[WaltS48 [:walts48]]

Looks good to me using 80.0b4 on Ubuntu 18.04.5 LTS.

Was the Message Security dialog intentionally skipped or just missed?

Comment 20

[Richard Marti (:Paenglab)]

Missed. Could you file a bug?