Closed Bug 1436100 Opened 6 years ago Closed 6 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)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
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)
Maybe _setProperties needs to guard against a null value of elem before calling _setProperty?
Flags: needinfo?(bgrinstead)
(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)
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)
(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 .
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.
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)
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.
(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.
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.
Attached patch Bug1436100-m-c.patch (obsolete) — Splinter Review
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 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+
(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.
Attached patch Patch for M-C (obsolete) — Splinter Review
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)
Attached patch Patch for C-C (obsolete) — Splinter Review
This is the C-C part.
Attachment #8950358 - Flags: review?(jorgk)
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+
Attached patch Patch for M-C (obsolete) — Splinter Review
(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+
Sheriff, please check-in only the M-C patch. We do the C-C patch our self.
Keywords: checkin-needed
Comment on attachment 8950358 [details] [diff] [review]
Patch for C-C

Thank you for pursuing this!
Attachment #8950358 - Flags: review?(jorgk) → review+
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
Attached patch Patch for M-C after bug 1417880 (obsolete) — Splinter Review
(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+
Attached patch Patch for C-CSplinter Review
Updated C-C patch with the bug 1417880 changes.
Attachment #8950358 - Attachment is obsolete: true
Attachment #8950539 - Flags: review+
Attachment #8950538 - Attachment is obsolete: true
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
(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?
(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 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
Priority: -- → P2
(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.
Attached patch Patch for M-C (obsolete) — Splinter Review
Changed to BUG_COMPONENT = ('Toolkit', 'WebExtensions: Themes')
Attachment #8950500 - Attachment is obsolete: true
Flags: needinfo?(shindli)
Attachment #8950918 - Flags: review+
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
Attached patch Patch for M-CSplinter Review
Used double quotes now.
Attachment #8950918 - Attachment is obsolete: true
Attachment #8950940 - Flags: review+
Please provide an updated patch, there are conflicts when one tries to apply it.
Flags: needinfo?(richard.marti)
Keywords: checkin-needed
Nevermind, mistake in the console.
Flags: needinfo?(richard.marti)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/45dae5243bef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b74299720d0a
Thunderbird part: Provide ThemeVariableMap.jsm. r=jorgk
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.