Closed
Bug 1343983
Opened 7 years ago
Closed 7 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)
18.59 KB,
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
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•7 years ago
|
||
Port to TB
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8843005 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 2•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
Added the comment.
Attachment #8843458 -
Attachment is obsolete: true
Attachment #8843743 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
Bug 1344307 will add new changes. Patch is ready, will file a new then.
Keywords: checkin-needed
Comment 12•7 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•7 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•7 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•7 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•7 years ago
|
||
Fixed comment.
Attachment #8843743 -
Attachment is obsolete: true
Attachment #8843888 -
Flags: review+
Assignee | ||
Comment 17•7 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•7 years ago
|
||
https://hg.mozilla.org/comm-central/rev/8be1ae77d2644e401fa6480a05ff234e70d0ccdf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Comment 19•7 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•7 years ago
|
Attachment #8843895 -
Attachment description: 1343983commet.patch → patch for m-c
Assignee | ||
Comment 20•7 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•7 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•7 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
•