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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

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)

(Reporter)

Description

a year 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.
Comment hidden (mozreview-request)
(Assignee)

Updated

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

Updated

a year ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Reporter)

Comment 3

a year 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).
Attachment #8947837 - Flags: feedback?(xidorn+moz) → feedback+
(Reporter)

Comment 4

a year ago
And thanks for picking this!

Comment 5

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

Comment 8

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

Comment 11

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

Comment 12

a year 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.
(Assignee)

Updated

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

Updated

a year ago
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

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

Comment 16

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

Comment 17

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

Comment 18

a year 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... :-)
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

a year 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
(Reporter)

Comment 21

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

Comment 22

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

Updated

a year ago
Depends on: 1435993
(Assignee)

Comment 23

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