1435122 - Consider moving custom properties for tabs from :root to #tabbrowser-tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1435122 - move custom properties from :root to tabbrowser where possible,

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

Comment 2

[:Gijs (he/him)]

Screenshots trypush:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89cf63b53bd7f7e62b2bd84b4a7b8a1142d868be

m-c base was https://hg.mozilla.org/mozilla-central/rev/17ade9f88b6e

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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).

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

And thanks for picking this!

Comment 5

[Tim Nguyen :ntim]

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

Comment 6

[:Gijs (he/him)]

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

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

Comment 7

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=758acd152217e28dd35dde4e0aea6423442299e2

Comment 8

[:Gijs (he/him)]

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.

Comment 9

[:Gijs (he/him)]

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

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

Comment 10

[:Gijs (he/him)]

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

Comment 11

[Tim Nguyen :ntim]

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.

Comment 12

[:Gijs (he/him)]

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.

Comment 13

[Tim Nguyen :ntim]

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

Comment 14

[:Gijs (he/him)]

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

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

Comment 15

[:Gijs (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217524/diff/4-5/

Comment 16

[Johann Hofmann [:johannh]]

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...

Comment 17

[Mike de Boer [:mikedeboer]]

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!

Comment 18

[:Gijs (he/him)]

(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... :-)

Comment 19

[Mike de Boer [:mikedeboer]]

(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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7334fd418e4
move custom properties from :root to tabbrowser where possible, r=johannh,mikedeboer

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 22

[Narcis Beleuzu [:NarcisB]]

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

Comment 23

[:Gijs (he/him)]

(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.