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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: xidorn, Assigned: Gijs)

Tracking

(Blocks 1 bug)

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

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 hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8947837 - Flags: feedback?(xidorn+moz)
Assignee

Updated

Last year
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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).
Attachment #8947837 - Flags: feedback?(xidorn+moz) → feedback+
And thanks for picking this!

Comment 5

Last year
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
Attachment #8947837 - Flags: review?(ntim.bugs)
Comment hidden (mozreview-request)
Assignee

Comment 8

Last year
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.
Depends on: 1426686
Comment hidden (mozreview-request)

Comment 11

Last year
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.
Attachment #8947837 - Flags: review?(ntim.bugs) → review-
Assignee

Comment 12

Last year
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.
Assignee

Updated

Last year
Attachment #8947837 - Flags: review- → review?(ntim.bugs)

Updated

Last year
Attachment #8947837 - Flags: review?(ntim.bugs) → review?(mdeboer)
Mike originally wrote this code, so he should probably review it.
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8947837 - Flags: review?(ntim.bugs) → review?(mdeboer)
Comment hidden (mozreview-request)

Comment 16

Last year
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...
Attachment #8947837 - Flags: review?(jhofmann) → review+

Comment 17

Last year
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!
Attachment #8947837 - Flags: review?(mdeboer) → review+
Assignee

Comment 18

Last year
(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... :-)
Flags: needinfo?(xidorn+moz)
(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

Last year
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
(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.
Flags: needinfo?(xidorn+moz)

Comment 22

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7334fd418e4
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee

Updated

Last year
Depends on: 1435993
Assignee

Comment 23

Last year
(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.
Depends on: 1436100
You need to log in before you can comment on or make changes to this bug.