Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 7 years ago

There are currently several custom properties for tabs applied on :root:
* --tabs-top-border-width
* --tab-min-height
* --tab-min-width
* --tab-loading-fill
* --tab-line-color

It may be convenient for frontend development to have them set on root, but it's not good for performance.

Since custom properties are inherited (as they should), changing them on the root would trigger a restyle on the whole document. (We can probably optimize it to only cascade custom properties at some point, but again, we still need to update styles on all elements.)

So, it would be helpful, performance-wise, to put custom properties on an ancestor as close as possible to where they are actually used.

In tabs' case, I suppose it should be #tabbrowser-tabs.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 3 • 7 years ago

Comment on attachment 8947837 [details]
Bug 1435122 - move custom properties from :root to tabbrowser where possible,

Looks good to me.

FWIW, we would need both this and bug 1435139 to actually see the potential improvement (if noticable).

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 4 • 7 years ago

And thanks for picking this!

	Tim Nguyen :ntim
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8947837 [details]
Bug 1435122 - move custom properties from :root to tabbrowser where possible,

https://reviewboard.mozilla.org/r/217524/#review223336

Clearing review for issues mentioned on IRC

	:Gijs (he/him) Assignee
	Comment 8 • 7 years ago

Hm, I would have liked to have just left the tabs-border-color thing given #nav-bar needs it, but now we're going to be messing with the tab loading spinner colour stuff at runtime too, so I guess we need to change LWTConsumer anyway, so let's do that here.

	:Gijs (he/him) Assignee
	Comment 10 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ca88f08d06731a77cc09c3c731e777cda4b187a

	Tim Nguyen :ntim
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8947837 [details]
Bug 1435122 - move custom properties from :root to tabbrowser where possible,

https://reviewboard.mozilla.org/r/217524/#review223350

::: toolkit/modules/LightweightThemeConsumer.jsm:203
(Diff revision 3)
> -    root.style.removeProperty(variableName);
> +    elem.style.removeProperty(variableName);
>    }
>  }
>  
>  function _setProperties(root, active, vars) {
> -  for (let [cssVarName, varsKey] of kCSSVarsMap) {
> +  for (let [cssVarName, varsKey, optionalElementID] of kCSSVarsMap) {

This doesn't work. --tabs-border-color is still set on root.

You can try running `./mach test toolkit/components/extensions/test/browser/browser_ext_themes_separators.js` (which fails with this patch) locally to check that this part is working.

	:Gijs (he/him) Assignee
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8947837 [details]
Bug 1435122 - move custom properties from :root to tabbrowser where possible,

https://reviewboard.mozilla.org/r/217524/#review223356

::: toolkit/modules/LightweightThemeConsumer.jsm:203
(Diff revision 3)
> -    root.style.removeProperty(variableName);
> +    elem.style.removeProperty(variableName);
>    }
>  }
>  
>  function _setProperties(root, active, vars) {
> -  for (let [cssVarName, varsKey] of kCSSVarsMap) {
> +  for (let [cssVarName, varsKey, optionalElementID] of kCSSVarsMap) {

It works for me and the test passes. It works now because `kCSSVarsMap` is no longer a Map, which wasn't in the initial cset. I think you're probably not running the current cset.

	Tim Nguyen :ntim
	Comment 13 • 7 years ago

Mike originally wrote this code, so he should probably review it.

	Johann Hofmann [:johannh]
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8947837 [details]
Bug 1435122 - move custom properties from :root to tabbrowser where possible,

https://reviewboard.mozilla.org/r/217524/#review223598

The tab styling parts look fine to me, though I have to defer to Mike and Tim for the theme stuff (or how this affects theming).

::: browser/themes/shared/tabs.inc.css:9
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  %endif
>  %filter substitution
>  %define horizontalTabPadding 9px
>  
> +/* This can't live on the tabbrowser because it's used for the titlebar height, too. */

As discussed on IRC, it wouldn't be hard to apply this to both tabbrowser and tabs in the three cases, I guess it's a matter of maintainability vs. perf improvements...

	Mike de Boer [:mikedeboer]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8947837 [details]
Bug 1435122 - move custom properties from :root to tabbrowser where possible,

https://reviewboard.mozilla.org/r/217524/#review223620

::: toolkit/modules/LightweightThemeConsumer.jsm:212
(Diff revision 5)
>    }
>  }
>  
>  function _setProperties(root, active, vars) {
> -  for (let [cssVarName, varsKey] of kCSSVarsMap) {
> -    _setProperty(root, active, cssVarName, vars[varsKey]);
> +  for (let [cssVarName, varsKey, optionalElementID] of kCSSVarsMap) {
> +    let elem = optionalElementID ? root.ownerDocument.getElementById(optionalElementID)

I can totally see us ending up with a `this._elemCache[elem] = elemObj`-style caching method. You can do that here, but we're also going to do work here in bug 1397393 and let others deal with it.

Not a deal breaker here!

	:Gijs (he/him) Assignee
	Comment 18 • 7 years ago

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> ::: toolkit/modules/LightweightThemeConsumer.jsm:212
> >  function _setProperties(root, active, vars) {
> > -  for (let [cssVarName, varsKey] of kCSSVarsMap) {
> > -    _setProperty(root, active, cssVarName, vars[varsKey]);
> > +  for (let [cssVarName, varsKey, optionalElementID] of kCSSVarsMap) {
> > +    let elem = optionalElementID ? root.ownerDocument.getElementById(optionalElementID)
> 
> I can totally see us ending up with a `this._elemCache[elem] =
> elemObj`-style caching method. You can do that here, but we're also going to
> do work here in bug 1397393 and let others deal with it.

Off-hand, this seems like it might be tricky, because `this` is a singleton inside this loose function that just lives on the global for this JSM, and obviously windows can go away and we wouldn't want to leak... so I've left this for now.



(In reply to Johann Hofmann [:johannh] from comment #16)
> ::: browser/themes/shared/tabs.inc.css:9
> > +/* This can't live on the tabbrowser because it's used for the titlebar height, too. */
> 
> As discussed on IRC, it wouldn't be hard to apply this to both tabbrowser
> and tabs in the three cases, I guess it's a matter of maintainability vs.
> perf improvements...

Right. Given the theme code is pretty bitrot-prone because of current active development, I elected to land this right now, and we can look into this in a follow-up if necessary.

My understanding was that the initial prompting for this bug being filed was the `tab-min-width` property (which is the only one of these properties that is directly set at runtime via the tabbrowser binding) being set and tripping whole-document restyles. As such I'm not 100% sure what, if any, perf gains are to be expected from moving and duplicating this other custom property (--tab-min-height) between the #titlebar and #tabbrowser-tabs, as opposed to having it on :root, especially because it'll then need 4 descendant selectors for `:root[uidensity=whatever]` for the cases where we adjust it from normal uidensity. Xidorn, do you know more about this? Are we implicitly triggering the same style flush here if we set the *attribute* for uidensity that then changes the value of the custom prop for the entire document? I kind of imagine that works differently, but I don't know the details of how custom props are implemented in style/layout... :-)

	Mike de Boer [:mikedeboer]
	Comment 19 • 7 years ago

(In reply to :Gijs from comment #18)
> Off-hand, this seems like it might be tricky, because `this` is a singleton
> inside this loose function that just lives on the global for this JSM, and
> obviously windows can go away and we wouldn't want to leak... so I've left
> this for now.

Well, we also save the document and window reference on the object and clear it inside the `destroy` member function.

	Pulsebot
	Comment 20 • 7 years ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f7334fd418e4
move custom properties from :root to tabbrowser where possible, r=johannh,mikedeboer

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 21 • 7 years ago

(In reply to :Gijs from comment #18)
> My understanding was that the initial prompting for this bug being filed was
> the `tab-min-width` property (which is the only one of these properties that
> is directly set at runtime via the tabbrowser binding) being set and
> tripping whole-document restyles.

This is correct.

> As such I'm not 100% sure what, if any,
> perf gains are to be expected from moving and duplicating this other custom
> property (--tab-min-height) between the #titlebar and #tabbrowser-tabs, as
> opposed to having it on :root, especially because it'll then need 4
> descendant selectors for `:root[uidensity=whatever]` for the cases where we
> adjust it from normal uidensity. Xidorn, do you know more about this? Are we
> implicitly triggering the same style flush here if we set the *attribute*
> for uidensity that then changes the value of the custom prop for the entire
> document? I kind of imagine that works differently, but I don't know the
> details of how custom props are implemented in style/layout... :-)

If we set the attribute for uidensity which changes the value of custom property on root element, yes we are going to trigger the same style flush. They may start from different entries, but we still need to propagate the new value of the custom property to every element in the document after changing it. Nothing can magically update that for all elements instantly.

	Narcis Beleuzu [:NarcisB]
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f7334fd418e4

	:Gijs (he/him) Assignee
	Comment 23 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #21)
> If we set the attribute for uidensity which changes the value of custom
> property on root element, yes we are going to trigger the same style flush.
> They may start from different entries, but we still need to propagate the
> new value of the custom property to every element in the document after
> changing it. Nothing can magically update that for all elements instantly.

I've filed bug 1435993 for this.