1434401 - Remove the "root-element" binding and instead set up the lightweight theme consumer on :root elements from JS

Status: RESOLVED FIXED

(Firefox :: Theme, task, P3)

Description

[Brian Grinstead [:bgrins]]

We have a binding "root-element" (https://bgrins.github.io/xbl-analysis/tree/#root-element), which gets bound onto the :root XUL element in minimal-xul.css (https://dxr.mozilla.org/mozilla-central/rev/474d58c9137360c0fa1c85cdd11e3313b33b7cad/toolkit/content/minimal-xul.css#33).

The binding basically looks for the "lightweightthemes" attribute on the root element and if it exists it sets up LightweightThemeConsumer instance. I think there’s a pretty good alternative to doing this via XBL which is listen for the "chrome-document-global-created" notification and do the same check / setup from there. Or sticking it in browser.js, given that's the only window we currently use this feature on in Firefox.

This binding is also extended by "dialog" "wizard" bindings, but I can't find any time where anything other than a <window> element gets the "lightweightthemes" attr.  In fact, in m-c the only instance of this is browser.xul (c-c does have quite a few hits but it's always applied to <window> elements so the same change should work there as well):

* https://searchfox.org/mozilla-central/search?q=lightweightthemes%3D&path=
* https://searchfox.org/comm-central/search?q=lightweightthemes%3D&path=

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1434401 - Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;

Review commit: https://reviewboard.mozilla.org/r/216714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216714/

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8946791 [details]
Bug 1434401 - Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;

Dão, can you review this change or suggest someone else to? I opted to put this into nsBrowserGlue instead of somewhere like gBrowserInit.onDOMContentLoaded in browser.js to stay closer to the current semantics: looking for an attribute in all XUL document roots instead of only browser.xul (even though in practice that's the only place we use it in m-c).

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 8946791 [details]
Bug 1434401 - Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;

https://reviewboard.mozilla.org/r/216714/#review222558

r=me assuming green try. :-)

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8946791 [details]
> Bug 1434401: Remove the "root-element" binding from :root and instead create
> the LightweightThemeConsumer from JS when new windows are created;
> 
> Dão, can you review this change or suggest someone else to? I opted to put
> this into nsBrowserGlue instead of somewhere like
> gBrowserInit.onDOMContentLoaded in browser.js to stay closer to the current
> semantics: looking for an attribute in all XUL document roots instead of
> only browser.xul (even though in practice that's the only place we use it in
> m-c).

Ugh, mozreview. FWIW, I didn't mean to steal this. When I added my self-review-request, mozreview didn't show any other reviewers, nor did it show any sign of a mid-air. Sorry! :-\


Dão, re-pinging you in case you want to review this instead of me.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #3)
> Comment on attachment 8946791 [details]
> Bug 1434401: Remove the "root-element" binding from :root and instead create
> the LightweightThemeConsumer from JS when new windows are created;
> 
> https://reviewboard.mozilla.org/r/216714/#review222558
> 
> r=me assuming green try. :-)

Thanks for the review :). I have a couple of try pushes and both look fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48809a6ebc2dd5a0fdedf015ca12aaae994372a8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99ffbdb997cc90cb793d0727124e8bdc3fa56e0

The second I ran especially to look at android, although since there's a completely different LWT implementation there I didn't expect any issue.

Comment 6

[Brian Grinstead [:bgrins]]

Frank, just a heads up this bug will remove the general.xml#root-element binding and the rule assigning it to :root in minimal-xul.css. See Comment 0 for more info.

Instead we are going to watch for new windows being created in js to set up the LWT consumer - the current patch uses the "chrome-document-global-created" notification. comm-central uses this feature in a few documents in editor/ mail/ and suite/ (https://searchfox.org/comm-central/search?q=lightweightthemes%3D&path=).  You could either restore the binding (minus the call to destroy() which will now be handled within the LWT consumer) or copy over the changes from nsBrowserGlue into the appropriate place(s) in comm-central.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8946791 [details]
> Bug 1434401: Remove the "root-element" binding from :root and instead create
> the LightweightThemeConsumer from JS when new windows are created;
> 
> Dão, can you review this change or suggest someone else to? I opted to put
> this into nsBrowserGlue instead of somewhere like
> gBrowserInit.onDOMContentLoaded in browser.js to stay closer to the current
> semantics: looking for an attribute in all XUL document roots instead of
> only browser.xul (even though in practice that's the only place we use it in
> m-c).

I think we should put this in gBrowserInit as I don't anticipate that we'll use this for other windows. Most non-browser.xul windows are small and transient. The long-term plan for the Library window is to move it into a tab.

Comment 8

[Frank-Rainer Grahl (:frg)]

Thanks. Will take care of it for SeaMonkey.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8946791 [details]
Bug 1434401 - Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216714/diff/1-2/

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8946791 [details]
Bug 1434401 - Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;

https://reviewboard.mozilla.org/r/216714/#review224784

::: browser/base/content/browser.js:1182
(Diff revision 2)
>  
>  var gBrowserInit = {
>    delayedStartupFinished: false,
>  
>    onDOMContentLoaded() {
> +    if (document.documentElement.hasAttribute("lightweightthemes")) {

Let's remove this attribute.

::: browser/base/content/browser.js:1183
(Diff revision 2)
>  var gBrowserInit = {
>    delayedStartupFinished: false,
>  
>    onDOMContentLoaded() {
> +    if (document.documentElement.hasAttribute("lightweightthemes")) {
> +      new LightweightThemeConsumer(document);

This seems oddly placed next to the very basic setup happening in onDOMContentLoaded. Any reason why you didn't put this in the onLoad method (e.g. grouped with the "Misc. inits.") other than that the xbl constructor probably ran pretty early too?

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #10)
> Comment on attachment 8946791 [details]
> Bug 1434401: Remove the "root-element" binding from :root and create the
> LightweightThemeConsumer directly from browser.js;
> 
> https://reviewboard.mozilla.org/r/216714/#review224784
> 
> ::: browser/base/content/browser.js:1182
> (Diff revision 2)
> >  
> >  var gBrowserInit = {
> >    delayedStartupFinished: false,
> >  
> >    onDOMContentLoaded() {
> > +    if (document.documentElement.hasAttribute("lightweightthemes")) {
> 
> Let's remove this attribute.

I did that at first but then realized that we load browser.js in hiddenWindow.xul (where we wouldn't want to apply the lwt). Although now that I read closer: onDOMContentLoaded / onLoad shouldn't actually fire in that case so I can remove the attribute.

> ::: browser/base/content/browser.js:1183
> (Diff revision 2)
> >  var gBrowserInit = {
> >    delayedStartupFinished: false,
> >  
> >    onDOMContentLoaded() {
> > +    if (document.documentElement.hasAttribute("lightweightthemes")) {
> > +      new LightweightThemeConsumer(document);
> 
> This seems oddly placed next to the very basic setup happening in
> onDOMContentLoaded. Any reason why you didn't put this in the onLoad method
> (e.g. grouped with the "Misc. inits.") other than that the xbl constructor
> probably ran pretty early too?

I put it there because I figured we'd want the lwt to be set up ASAP to avoid a FOUC. The XBL constructor should actually run before DOMContentLoaded (the documentElement gets accessed during the evaluation of browser.js when checking windowtype). I don't feel strongly about any of these but we could either:

1) Keep it in DOMContentLoaded
2) Move it into onLoad
3) Move it as inline execution

In (3) we'd keep the current behavior, but we'd want to keep the "lightweightthemes" attr so it didn't run in hiddenWindow.xul.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> > >    onDOMContentLoaded() {
> > > +    if (document.documentElement.hasAttribute("lightweightthemes")) {
> > > +      new LightweightThemeConsumer(document);
> > 
> > This seems oddly placed next to the very basic setup happening in
> > onDOMContentLoaded. Any reason why you didn't put this in the onLoad method
> > (e.g. grouped with the "Misc. inits.") other than that the xbl constructor
> > probably ran pretty early too?
> 
> I put it there because I figured we'd want the lwt to be set up ASAP to
> avoid a FOUC. The XBL constructor should actually run before
> DOMContentLoaded (the documentElement gets accessed during the evaluation of
> browser.js when checking windowtype). I don't feel strongly about any of
> these but we could either:
> 
> 1) Keep it in DOMContentLoaded
> 2) Move it into onLoad
> 3) Move it as inline execution

I believe onLoad right now should run before the first paint, although this may change as a result of bug 1336227. I'd probably start with (2), and revert to (1) only if needed. If you want to keep it in DOMContentLoaded, please move it down after the more critical stuff.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8946791 [details]
Bug 1434401 - Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216714/diff/2-3/

Comment 14

[Brian Grinstead [:bgrins]]

Switched it to onLoad and removed the "lightweightthemes" attribute

Comment 15

[Brian Grinstead [:bgrins]]

It looks like the lightweightthemes attribute is used in mail/ editor/ and suite/. You may want to either restore the root-element binding in comm-central, or move the LightweightThemeConsumer instantiation to JS as this patch does.

Comment 16

[Richard Marti (:Paenglab)]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> It looks like the lightweightthemes attribute is used in mail/ editor/ and
> suite/. You may want to either restore the root-element binding in
> comm-central, or move the LightweightThemeConsumer instantiation to JS as
> this patch does.

Thank you for the info. I will check what way is better for us.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86af6b6108d8
Remove the "root-element" binding from :root and create the LightweightThemeConsumer directly from browser.js;r=dao,Gijs

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/86af6b6108d8