Closed
Bug 1343983
Opened 8 years ago
Closed 8 years ago
Port bug 864562 to TB [Use CSS Variables for lightweight theme styling]
Categories
(Thunderbird :: Theme, enhancement)
Thunderbird
Theme
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 54.0
People
(Reporter: Paenglab, Assigned: Paenglab)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
Bug 864562 changed LWT images usage to variables. We need to follow to still see the background images.
Assignee | ||
Comment 1•8 years ago
|
||
Port to TB
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8843005 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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().
Comment 7•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
Added the comment.
Attachment #8843458 -
Attachment is obsolete: true
Attachment #8843743 -
Flags: review+
Assignee | ||
Comment 11•8 years ago
|
||
Bug 1344307 will add new changes. Patch is ready, will file a new then.
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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?
Comment 13•8 years ago
|
||
Maybe we can clarify this before landing it. I can see that this comment is copied from M-C.
Keywords: checkin-needed
Assignee | ||
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
Fixed comment.
Attachment #8843743 -
Attachment is obsolete: true
Attachment #8843888 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Comment 19•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8843895 -
Attachment description: 1343983commet.patch → patch for m-c
Assignee | ||
Comment 20•8 years ago
|
||
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
Comment 21•8 years ago
|
||
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
Comment 22•8 years ago
|
||
(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.
Description
•