Port bug 864562 to TB [Use CSS Variables for lightweight theme styling]

RESOLVED FIXED in Thunderbird 54.0

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Bug 864562 changed LWT images usage to variables. We need to follow to still see the background images.
Posted patch LW-theme-variables.patch (obsolete) — Splinter Review
Port to TB
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8843005 - Flags: review?(mkmelin+mozilla)
Posted patch LW-theme-variables.patch (obsolete) — Splinter Review
Forgot to remove the no more needed JS code.
Attachment #8843005 - Attachment is obsolete: true
Attachment #8843005 - Flags: review?(mkmelin+mozilla)
Attachment #8843348 - Flags: review?(mkmelin+mozilla)
Posted patch LW-theme-variables.patch (obsolete) — Splinter Review
Bug 1343196 removed the toolbox background-color. I added it in this patch too for Windows. Linux is not needed.
Attachment #8843359 - Flags: review?(mkmelin+mozilla)
Posted patch LW-theme-variables.patch (obsolete) — Splinter Review
Sorry for the spam. I removed some more unneeded rules after bug 1343196.
Attachment #8843348 - Attachment is obsolete: true
Attachment #8843359 - Attachment is obsolete: true
Attachment #8843348 - Flags: review?(mkmelin+mozilla)
Attachment #8843359 - Flags: review?(mkmelin+mozilla)
Attachment #8843458 - Flags: review?(mkmelin+mozilla)
Does this work?

> +:root:-moz-lwtheme:not([customization-lwtheme]) {
> +  background-color: var(--lwt-accent-color) !important;
> +  background-image: var(--lwt-header-image) !important;
> +}

customization-lwtheme seems only to be set/managed in 
 mozilla/browser/components/customizableui/CustomizeMode.jsm
Yes it works. [customization-lwtheme] is never set also is the :not() always true and the colors are used with LW-Themes. With this the rule is also more specific to override a maybe other existing rule.

I leaved it as we never know when m-c moves this to toolkit. I'll let Magnus decide if we remove it or not. It would also work without the :not().
Yes was a little confused because as you said if its never set its always :not(). If you leave it in a short comment in the file might clear it up for others.
Duplicate of this bug: 1344505
Comment on attachment 8843458 [details] [diff] [review]
LW-theme-variables.patch

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

Looks reasonable to me, r=mkmelin
Please leave a short comment  per comment 7.
Attachment #8843458 - Flags: review?(mkmelin+mozilla) → review+
Posted patch LW-theme-variables.patch (obsolete) — Splinter Review
Added the comment.
Attachment #8843458 - Attachment is obsolete: true
Attachment #8843743 - Flags: review+
Bug 1344307 will add new changes. Patch is ready, will file a new then.
Keywords: checkin-needed
Comment on attachment 8843743 [details] [diff] [review]
LW-theme-variables.patch

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

::: mail/themes/shared/mail/tabmail.css
@@ +198,5 @@
>    background-color: transparent;
>  }
>  
> +/*
> + * LightweightThemeListener will set the current lightweight theme's header

Is this comment correct? As far as I can see you're removing that JS code, right?
Maybe we can clarify this before landing it. I can see that this comment is copied from M-C.
Keywords: checkin-needed
I'm removing that code because it is now in toolkit: toolkit/modules/LightweightThemeConsumer.jsm. Maybe it would be better with

> +/*
> + * LightweightThemeConsumer will set the current lightweight theme's header

But then we are not in sync with m-c.
Well, I looked for LightweightThemeListener and didn't find it. So the correct way to handle this is to use a correct comment for TB and attach a patch *here* for M-C, get it reviewed and I can get it landed, see for example bug 1326494 comment #18 where a comment change was pushed directly to M-C. I don't think another bug is warranted to change one word in a comment.
Fixed comment.
Attachment #8843743 - Attachment is obsolete: true
Attachment #8843888 - Flags: review+
Posted patch patch for m-cSplinter Review
Mike, now the LightweightThemeListener.js is removed and the functionality is in LightweightThemeConsumer.jsm. I changed the comment to easier find it when searching through the comment.
Attachment #8843895 - Flags: review?(mdeboer)
https://hg.mozilla.org/comm-central/rev/8be1ae77d2644e401fa6480a05ff234e70d0ccdf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Comment on attachment 8843895 [details] [diff] [review]
patch for m-c

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

Ah! Of course, this is fine.

I'm happy to see this also leading to simplification in TB! \o/
Attachment #8843895 - Flags: review?(mdeboer) → review+
Attachment #8843895 - Attachment description: 1343983commet.patch → patch for m-c
Carsten, please can you check-in the "patch for m-c"? It's only a comment change so a DONTBUILD or combined with other patches would be the best.
Flags: needinfo?(cbook)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/76ceeecd75c4
Fix the comment in tabs.inc.css. r=mikedeboer DONTBUILD a=comment-change-for-thunderbird
Keywords: checkin-needed
(In reply to Richard Marti (:Paenglab) from comment #20)
> Carsten, please can you check-in the "patch for m-c"? It's only a comment
> change so a DONTBUILD or combined with other patches would be the best.

np, landed in https://hg.mozilla.org/mozilla-central/rev/76ceeecd75c4116b24134f1ed8d7950023c07bf5
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.