Closed
Bug 1436100
Opened 7 years ago
Closed 7 years ago
Bug 1435122 breaks the WE themes on Thunderbird (JavaScript error: resource://gre/modules/LightweightThemeConsumer.jsm, line 204: TypeError: elem is null)
Categories
(WebExtensions :: Themes, enhancement, P2)
WebExtensions
Themes
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: Paenglab, Assigned: Paenglab)
References
Details
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
The in bug 1435122 added variable (https://hg.mozilla.org/mozilla-central/rev/f7334fd418e40a7cb6a76e39451b011fca1333aa#l7.18 and https://hg.mozilla.org/mozilla-central/rev/f7334fd418e40a7cb6a76e39451b011fca1333aa#l7.27) breaks the WE themes on TB. The toolbar background color, the icon color, the line colors etc. are no more set.
Removing the third variable fixes the themes on TB.
Assignee | ||
Comment 1•7 years ago
|
||
Gijs and Brian, what is needed to apply the third variable only on Firefox?
I tried it with #ifdefs but didn't found a way to enable the preprocessor. Would there be a way with AppConstants, and if yes, how?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bgrinstead)
Comment 2•7 years ago
|
||
Maybe _setProperties needs to guard against a null value of elem before calling _setProperty?
Flags: needinfo?(bgrinstead)
Comment 3•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Maybe _setProperties needs to guard against a null value of elem before
> calling _setProperty?
Probably. Though more generally I'm a bit confused... are these values used in comm-central?
It looks like tabs-border-color is used, but tab-loading-fill isn't.
I expect that really, we should decouple the exact variables here from LWTConsumer, or move LWTConsumer to browser/ and have suite/tb fork it. It won't make sense to keep adding stuff that suite/tb don't need, and in effect they are tightly coupled to browser style. The fact that mail style uses the same variable name here is pure coincidence, and if we move variables around like we did in bug 1435122 then TB will just break.
As a stopgap, you can nullcheck the element and fall back to root, but that isn't really the 'right' fix - it just happens to work at the moment.
More generally, I don't really understand how WE themes are supposed to work in TB. Is there a plan for this? Is it working even expected?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(richard.marti)
Assignee | ||
Comment 4•7 years ago
|
||
tab-loading-fill is the only variable TB doesn't use. All other are used or I'm on a patch to use them (toolbarbutton-background, toolbarbutton-hover-background and toolbarbutton-active-background).
Flags: needinfo?(richard.marti)
Comment 5•7 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #4)
> tab-loading-fill is the only variable TB doesn't use. All other are used or
> I'm on a patch to use them (toolbarbutton-background,
> toolbarbutton-hover-background and toolbarbutton-active-background).
What's the name of the tabstrip in TB / SeaMonkey?
The best fix here would be to have application-specific element IDs for these variables. TB probably wants to take a similar patch to bug 1435122 (and bug 1435993) to move variable definitions to the elements they apply to, instead of having them all blindly on :root .
Assignee | ||
Comment 6•7 years ago
|
||
TB uses the LWT colors not only on the main window. The toolbars of the Addressbook- and the Composer window use them too.
An additional problem is, that the menu bar and the tab bar are in a other toolbox than the main toolbar. A complicated construct, I know.
Comment 7•7 years ago
|
||
Richard, can you attach the non-working WIP patch you mentioned on IRC.
Summary: Bug 1435122 breaks the WE themes on Thunderbird → Bug 1435122 breaks the WE themes on Thunderbird (JavaScript error: resource://gre/modules/LightweightThemeConsumer.jsm, line 204: TypeError: elem is null)
Assignee | ||
Comment 8•7 years ago
|
||
Unfortunately it was on the PC there the power supply blew up. :(
it was something like this (handmade from memory):
function _setProperties(root, active, vars) {
for (let [cssVarName, varsKey, optionalElementID] of kCSSVarsMap) {
+ if (root.ownerDocument.getElementById(optionalElementID)) {
let elem = optionalElementID ? root.ownerDocument.getElementById(optionalElementID)
: root;
+ } else {
+ let elem = root;
+ }
_setProperty(elem, active, cssVarName, vars[varsKey]);
}
}
It should check if optionalElementID is found in the tree. If not use root.
Comment 9•7 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #8)
> Unfortunately it was on the PC there the power supply blew up. :(
>
> it was something like this (handmade from memory):
>
> function _setProperties(root, active, vars) {
> for (let [cssVarName, varsKey, optionalElementID] of kCSSVarsMap) {
> + if (root.ownerDocument.getElementById(optionalElementID)) {
> let elem = optionalElementID ?
> root.ownerDocument.getElementById(optionalElementID)
> : root;
> + } else {
> + let elem = root;
> + }
> _setProperty(elem, active, cssVarName, vars[varsKey]);
> }
> }
>
> It should check if optionalElementID is found in the tree. If not use root.
This will unbreak things, but ideally TB should similarly move CSS variables off root and will then need to start maintaining its own set of elements to set these variables on.
This is why I suggested forking files or decoupling the general file from the specific variables that get set some other way.
Comment 10•7 years ago
|
||
Richard, can we do what Gijs suggests in comment #5 and comment #9. Since the null check isn't the "right" fix, I assume M-C wouldn't take the patch even if we could get it to work.
Assignee | ||
Comment 11•7 years ago
|
||
Gijs, is this what you thought in comment #5 and comment #9? If yes, it doesn't work yet. My knowledge is too limited, what do I need to make it work?
When it works I'm planning to move the LWThemeMap.js to browser (and on c-c to mail) and package it then in modules.
Attachment #8950000 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 12•7 years ago
|
||
Comment on attachment 8950000 [details] [diff] [review]
Bug1436100-m-c.patch
Review of attachment 8950000 [details] [diff] [review]:
-----------------------------------------------------------------
This would be one way of doing it, yes.
The reason it won't work like this is because you need an EXPORTED_SYMBOLS variable that defines which bits in the module you want to export into places where you import the module. As a review nit, it'd be good to have that name patch the name of the module, probably by renaming the `kCSSVars` thing which in any case is only used in 1 or 2 places in LWTConsumer. So you'd have:
```
var EXPORTED_SYMBOLS = ["ThemeVariableMap"];
const ThemeVariableMap = {
...
};
```
in the module (named `ThemeVariableMap.jsm`). We can start omitting "LW" because we no longer support 'full' themes anyway, and these themes aren't the "original" lightweight themes, either.
Attachment #8950000 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Comment 13•7 years ago
|
||
(In reply to :Gijs from comment #12)
> As a review nit, it'd be good to have
> that name patch the name of the module, probably by renaming the `kCSSVars`
> thing which in any case is only used in 1 or 2 places in LWTConsumer.
*match* the name of the module, argh mornings.
Assignee | ||
Comment 14•7 years ago
|
||
Thank you Gijs for the explanation.
This works now.
Assignee: nobody → richard.marti
Attachment #8950000 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8950357 -
Flags: review?(gijskruitbosch+bugs)
Comment 16•7 years ago
|
||
Comment on attachment 8950357 [details] [diff] [review]
Patch for M-C
Review of attachment 8950357 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, r=me
Thinking about it more though, there's still some tight coupling in the way that the LWTConsumer depends on the structure of the array in ThemeVariableMap. If for Firefox's sake we need to add more information to that, TB will break (again). I don't see how to best avoid that without introducing much more complexity, though - maybe pushing out more functionality into browser / mail instead of toolkit. But we can cross that bridge when we get there.
::: browser/modules/moz.build
@@ +88,5 @@
> with Files("SitePermissions.jsm"):
> BUG_COMPONENT = ("Firefox", "Site Identity and Permission Panels")
>
> +with Files('ThemeVariableMap.jsm'):
> + BUG_COMPONENT = ('Firefox', 'Toolbars and Customization')
Firefox :: Theme please.
::: toolkit/modules/LightweightThemeConsumer.jsm
@@ +6,5 @@
>
> ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> ChromeUtils.import("resource://gre/modules/Services.jsm");
> ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
> +ChromeUtils.import("resource:///modules/ThemeVariableMap.jsm");
Can you add a comment here explaining what's going on? E.g.:
// Get the theme variables from the app resource directory.
// This allows per-app variables.
Attachment #8950357 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to :Gijs from comment #16)
> Comment on attachment 8950357 [details] [diff] [review]
> Patch for M-C
>
> Review of attachment 8950357 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thanks, r=me
>
> Thinking about it more though, there's still some tight coupling in the way
> that the LWTConsumer depends on the structure of the array in
> ThemeVariableMap. If for Firefox's sake we need to add more information to
> that, TB will break (again). I don't see how to best avoid that without
> introducing much more complexity, though - maybe pushing out more
> functionality into browser / mail instead of toolkit. But we can cross that
> bridge when we get there.
I'm watching this files and can react at the least when they land in m-c.
> ::: browser/modules/moz.build
> @@ +88,5 @@
> > with Files("SitePermissions.jsm"):
> > BUG_COMPONENT = ("Firefox", "Site Identity and Permission Panels")
> >
> > +with Files('ThemeVariableMap.jsm'):
> > + BUG_COMPONENT = ('Firefox', 'Toolbars and Customization')
>
> Firefox :: Theme please.
Done
> ::: toolkit/modules/LightweightThemeConsumer.jsm
> @@ +6,5 @@
> >
> > ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> > ChromeUtils.import("resource://gre/modules/Services.jsm");
> > ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
> > +ChromeUtils.import("resource:///modules/ThemeVariableMap.jsm");
>
> Can you add a comment here explaining what's going on? E.g.:
>
> // Get the theme variables from the app resource directory.
> // This allows per-app variables.
Done
Attachment #8950357 -
Attachment is obsolete: true
Attachment #8950500 -
Flags: review+
Assignee | ||
Comment 18•7 years ago
|
||
Sheriff, please check-in only the M-C patch. We do the C-C patch our self.
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Comment on attachment 8950358 [details] [diff] [review]
Patch for C-C
Thank you for pursuing this!
Attachment #8950358 -
Flags: review?(jorgk) → review+
Comment 20•7 years ago
|
||
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf20a59521ad
Let the apps use their own ThemeVariableMap.jsm constant. r=gijs
Keywords: checkin-needed
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Pulsebot from comment #20)
> Pushed by shindli@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bf20a59521ad
> Let the apps use their own ThemeVariableMap.jsm constant. r=gijs
This will conflict with bug 1417880 which landed in autoland shortly before this push.
Stefan, do you want to back-out the patch and then land this one after bug 1417880 landed?
Flags: needinfo?(shindli)
Attachment #8950538 -
Flags: review+
Assignee | ||
Comment 22•7 years ago
|
||
Updated C-C patch with the bug 1417880 changes.
Attachment #8950358 -
Attachment is obsolete: true
Attachment #8950539 -
Flags: review+
Comment 23•7 years ago
|
||
Backed out changeset bf20a59521ad (bug 1436100) per developers request on a CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/96115b59fa4db7d4d235982f6993c396aaeb7453
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=96115b59fa4db7d4d235982f6993c396aaeb7453&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Flags: needinfo?(shindli)
Assignee | ||
Updated•7 years ago
|
Attachment #8950538 -
Attachment is obsolete: true
Assignee | ||
Comment 24•7 years ago
|
||
Stefan, sorry to bother you again. Bug 1417880 was backed-out. Could you land this one again? We need this landed because it breaks the themes on TB.
Flags: needinfo?(shindli)
Keywords: checkin-needed
Comment 25•7 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #22)
> Updated C-C patch with the bug 1417880 changes.
We go with the updated patch for C-C despite the backout of bug 1417880? Or revert to the previous one?
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #25)
> (In reply to Richard Marti (:Paenglab) from comment #22)
> > Updated C-C patch with the bug 1417880 changes.
> We go with the updated patch for C-C despite the backout of bug 1417880? Or
> revert to the previous one?
I saw no issue with too much entries in this file (I tested it with fantasy values).
Comment 27•7 years ago
|
||
Comment on attachment 8950500 [details] [diff] [review]
Patch for M-C
Review of attachment 8950500 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/moz.build
@@ +88,5 @@
> with Files("SitePermissions.jsm"):
> BUG_COMPONENT = ("Firefox", "Site Identity and Permission Panels")
>
> +with Files('ThemeVariableMap.jsm'):
> + BUG_COMPONENT = ('Firefox', 'Theme')
I think the newly created Toolkit - WebExtensions: Themes component might be more appropriate
Updated•7 years ago
|
Priority: -- → P2
Comment 28•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #27)
> I think the newly created Toolkit - WebExtensions: Themes component might be
> more appropriate
Oh, right, that would make even more sense.
Assignee | ||
Comment 29•7 years ago
|
||
Changed to BUG_COMPONENT = ('Toolkit', 'WebExtensions: Themes')
Attachment #8950500 -
Attachment is obsolete: true
Flags: needinfo?(shindli)
Attachment #8950918 -
Flags: review+
Comment 30•7 years ago
|
||
Comment on attachment 8950918 [details] [diff] [review]
Patch for M-C
Review of attachment 8950918 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/moz.build
@@ +88,5 @@
> with Files("SitePermissions.jsm"):
> BUG_COMPONENT = ("Firefox", "Site Identity and Permission Panels")
>
> +with Files('ThemeVariableMap.jsm'):
> + BUG_COMPONENT = ('Toolkit', 'WebExtensions: Themes')
nit: this should also use double quotes for consistency with the rest of the file
Assignee | ||
Comment 31•7 years ago
|
||
Used double quotes now.
Attachment #8950918 -
Attachment is obsolete: true
Attachment #8950940 -
Flags: review+
Comment 32•7 years ago
|
||
Please provide an updated patch, there are conflicts when one tries to apply it.
Flags: needinfo?(richard.marti)
Keywords: checkin-needed
Comment 33•7 years ago
|
||
Nevermind, mistake in the console.
Flags: needinfo?(richard.marti)
Keywords: checkin-needed
Comment 34•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45dae5243bef
Let the apps use their own ThemeVariableMap.jsm constant. r=gijs
Keywords: checkin-needed
Comment 35•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 36•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b74299720d0a
Thunderbird part: Provide ThemeVariableMap.jsm. r=jorgk
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•