Closed Bug 1728712 Opened 3 years ago Closed 3 years ago

Fix font color for "save mail" confirmation dialogue in dark mode

Categories

(Thunderbird :: Theme, defect)

Product:
Thunderbird
Component:
Theme
Version:
Thunderbird 91
Type:
defect

Tracking

(thunderbird_esr91+ verified, thunderbird93+ fixed)

Status:
RESOLVED FIXED
Milestone:
94 Branch
Tracking Flags:
Tracking Status
thunderbird_esr91 + verified
thunderbird93 + fixed

People

(Reporter: bhakta0007, Assigned: Paenglab)

References

Details

Attachments

(4 files, 3 obsolete files)

Screen Shot 2021-09-02 at 9.41.47 AM.png
(deleted), image/png
Details
1728712-SelectedItem.patch
(deleted), patch
aleca
: review+
Details | Diff | Splinter Review
1728712-commonDialog-themeable.patch
(deleted), patch
Paenglab
: review+
Details | Diff | Splinter Review
1728712-commonDialog-themeable-beta-ESR.patch
(deleted), patch
wsmwk
: approval-comm-beta+
wsmwk
: approval-comm-esr91+
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.131 Safari/537.36

Steps to reproduce:

I am on the latest nightly: 93.0a1 (2021-09-01) (64-bit)

Create a new mail.. Type in a few words and try to close the window

Actual results:

a dialogue pops up with discard confirmation. The font color does not match the theme

Expected results:

font should be in readable color

What are your theme (OS and TB) settings?

Component: Message Compose Window → Theme

Macos : Settings->General->Appearance: Dark
Thunderbird: Tools->Add ons and Themes -> Themes -> Light.

Attached patch 1728712-commonDialog-themeable.patch (obsolete) (deleted) — — Splinter Review

How about overriding the toolkit commonDialog.xhtml with our own that supports themeability? FX uses in-content dialogs like in the prefs and doesn't suffer of this problem and so this will not be fixed.

commonDialog.xhtml is very seldom changed, and now less with the in-content approach, and it shouldn't be a problem for us to override it.

Assignee: nobody → richard.marti
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9239186 - Flags: review?(alessandro)
Blocks: tb91found
Comment on attachment 9239186 [details] [diff] [review] 1728712-commonDialog-themeable.patch Review of attachment 9239186 [details] [diff] [review]: ----------------------------------------------------------------- Indeed, I think overriding this dialog will give us more control not only from a theming aspect. A couple of nit fixes that we should land in this patch as well. - The focus-visible is not styled for when the focus moved across the dialog buttons. - The `dlgtype="accept"` should be styled like a primary class to respect the UX paradigm of highlighting the "primary" call to action.
Attachment #9239186 - Flags: review?(alessandro) → feedback+
Attached patch 1728712-commonDialog-themeable.patch (obsolete) (deleted) — — Splinter Review

Unfortunately I had to add messenger.css to the includes to get the needed variable colours. This shouldn't be bad as this is already in memory.

This can be changed when we have the variables.css implemented (but this patch is also for TB 91 where is no variables.css).

Attachment #9239186 - Attachment is obsolete: true
Attachment #9239912 - Flags: review?(alessandro)
Attached patch 1728712-SelectedItem.patch (deleted) — — Splinter Review

For the focus this changes the colour Highlight to SelectedItem which was introduced last week on M-C. Especially on Mac Highlight is now very faint.

Attachment #9239913 - Flags: review?(alessandro)
Attached patch 1728712-commonDialog-themeable.patch (obsolete) (deleted) — — Splinter Review

Updated after landing of other patches.

Attachment #9239912 - Attachment is obsolete: true
Attachment #9239912 - Flags: review?(alessandro)
Attachment #9240349 - Flags: review?(alessandro)
Attachment #9239913 - Flags: review?(alessandro) → review+
Comment on attachment 9240349 [details] [diff] [review] 1728712-commonDialog-themeable.patch Review of attachment 9240349 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. ::: mail/base/content/commonDialog.xhtml @@ +1,5 @@ > +<?xml version="1.0"?> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + nit: extra blank line.
Attachment #9240349 - Flags: review?(alessandro) → review+

Removed extra line which was copied from m-c.

Attachment #9240349 - Attachment is obsolete: true
Attachment #9240460 - Flags: review+

Please apply 1728712-commonDialog-themeable.patch first.

status-thunderbird93: --- → affected
status-thunderbird_esr91: --- → affected
tracking-thunderbird93: --- → ?
tracking-thunderbird_esr91: --- → ?
Target Milestone: --- → 94 Branch
Keywords: checkin-needed-tb

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5c688c2c2840
Override the toolkit commonDialog.xhtml with our themeable version. r=aleca
https://hg.mozilla.org/comm-central/rev/b909a9c98e3e
Use "SelectedItem" color instead of "Highlight" for focus. r=aleca

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

[Approval Request Comment]
User impact if declined: common dialogs not themeable
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Attachment #9240725 - Flags: approval-comm-esr91?
Attachment #9240725 - Flags: approval-comm-beta?

Comment on attachment 9240725 [details] [diff] [review]
1728712-commonDialog-themeable-beta-ESR.patch

[Triage Comment]
Approved for beta

Attachment #9240725 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9240725 [details] [diff] [review]
1728712-commonDialog-themeable-beta-ESR.patch

[Triage Comment]
Approved for esr91

Attachment #9240725 - Flags: approval-comm-esr91? → approval-comm-esr91+

91.1.1 on Mac looks good to me

status-thunderbird_esr91: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: