1091001 - Convert tabToolbarNavbarOverlap to a CSS variable

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Brian Grinstead [:bgrins]]

The preprocessor variable @tabToolbarNavbarOverlap@ looks like it could be easily changed to a CSS variable.  Here are all references:

http://dxr.mozilla.org/mozilla-central/search?q=tabToolbarNavbarOverlap&case=true&redirect=true

Comment 1

[Brian Grinstead [:bgrins]]

We should also switch these at the same time:

%define tabCurveWidth 30px
%define tabCurveHalfWidth 15px

And consider using a calc() to define tabCurveHalfWidth so that theme authors could set just the one (tabCurveWidth) instead of both.  We'd need to measure performance with this change.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: tab-variable.patch

This converts the tab preprocessor directives to css variables.

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8523298 [details] [diff] [review]
tab-variable.patch

I haven't yet run this through mozscreenshots, but it ran fine locally.  Please review the changes and let me know if you think I should take a different approach anywhere.  My basic plan is:

* Move the variable declarations into browser.inc
* Anywhere -@foo@ was there I've switched it to calc(0px - var(--foo)).
* I've kept the half curve width in place just to keep the usage more compact.
* There is a workaround for Bug 1099448, but it's not too much extra work

Comment 4

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> * Move the variable declarations into browser.inc

Why did you move tabCurveWidth and tabCurveHalfWidth from tabs.inc.css to browser.css?

> * Anywhere -@foo@ was there I've switched it to calc(0px - var(--foo)).

I'd prefer calc(-1 * var(--foo)).

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8523298 [details] [diff] [review]
tab-variable.patch

Review of attachment 8523298 [details] [diff] [review]:
-----------------------------------------------------------------

What Dão said.

I'm also not sure why this was marked for feedback instead of review? I wonder if we should scope variables to the minimal container element or if they should always be on :root? I'm not sure what's best for perf./maintainability/etc.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > * Move the variable declarations into browser.inc
> 
> Why did you move tabCurveWidth and tabCurveHalfWidth from tabs.inc.css to
> browser.css?

Just to be alongside --tab-toolbar-navbar-overlap and -tab-min-height... They feel like similar variables and it seemed like they should be next to each other.  We could have two separate blocks and (maybe) make the one in tabs.inc.css be more tightly scoped to the container element needed container element.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22; away Nov. 14-19) from comment #5)
> Comment on attachment 8523298 [details] [diff] [review]
> tab-variable.patch
> 
> Review of attachment 8523298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What Dão said.
> 
> I'm also not sure why this was marked for feedback instead of review? I
> wonder if we should scope variables to the minimal container element or if
> they should always be on :root? I'm not sure what's best for
> perf./maintainability/etc.

I think that scoping it to the root could make things easier for maintainability since you don't have to worry about cases where maybe it's used outside of the container you thought it would be.  But I don't know about perf.  Cam, is there a performance benefit to declaring variables only in the scope they are strictly necessary?  For example:

  :root {
    --foo: red;
  }

  ...

  #myelement {
     color: var(--foo);
  }

versus

  #myelement {
    --foo: red;
  }

  ...

  #myelement {
     color: var(--foo);
  }

Comment 8

[Cameron McCormack (:heycam)]

There should be no performance benefit from scoping variables narrowly like that, since we need to compute the CSS variables on every node anyway.  But there will be a memory benefit from limiting the number of elements that have rules with variables targetted at them.  If an element inherits variables from its parent and doesn't set any of its own, it will share the memory used to store the parent's variables.  So if you have:

  :root { --foo: red; }
  #myelement { --bar: green; }

and assuming myelement isn't the root, then there will be two allocations of memory to store variables -- on the root element you'll have a table storing { foo : " red" } and on myelement you'll have a table storing { foo : " red", bar : " green" }.  But if you just have everything on the root:

  :root { --foo: red; }
  :root { --bar: green; }

then there'll only be one table, storing { foo : " red", bar : " green" }.

Comment 9

[Cameron McCormack (:heycam)]

That is also a performance benefit, by the way, since each element where we have at least one variable declaration we need to compute that table by copying the inherited variables and then adding/overwriting them with those on itself.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> I think that scoping it to the root could make things easier for
> maintainability since you don't have to worry about cases where maybe it's
> used outside of the container you thought it would be.

It shouldn't be used outside of the container. Enforcing this seems like a feature, not a bug, as it helps ensure things don't become a mess e.g. when someone has the bright idea to use that variable for some other UI element that coincidentally wants the same value.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > I think that scoping it to the root could make things easier for
> > maintainability since you don't have to worry about cases where maybe it's
> > used outside of the container you thought it would be.
> 
> It shouldn't be used outside of the container. Enforcing this seems like a
> feature, not a bug, as it helps ensure things don't become a mess e.g. when
> someone has the bright idea to use that variable for some other UI element
> that coincidentally wants the same value.

It seems like a trade-off.  Narrowly scoping them has some issues as well:
* It makes it harder for extension / theme authors to override these values - you have to figure out which element the variable was defined on, and keep that up to date for future releases if the source files have changed.
* If you don't pick the right container element you could end up with a variable that works sometimes but not others - and it could be tricky to automatically catch these errors.

Here are more benefits to keeping them on the root, at least in this particular case:
* The performance is better (comments 8 and 9).
* It better matches the semantics of the preprocessor system, which makes updates like this more mechanical and less error-prone.

Comment 12

[Cameron McCormack (:heycam)]

I would encourage you to measure the performance difference between the two approaches, if you are making a decision based on that.  It might well be negligble. :-)

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: tab-variable.patch

This splits variables declarations into two blocks and switches the negation syntax from calc(-1 * foo) instead of calc(0px - foo).

Comment 14

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > (In reply to Brian Grinstead [:bgrins] from comment #7)
> > > I think that scoping it to the root could make things easier for
> > > maintainability since you don't have to worry about cases where maybe it's
> > > used outside of the container you thought it would be.
> > 
> > It shouldn't be used outside of the container. Enforcing this seems like a
> > feature, not a bug, as it helps ensure things don't become a mess e.g. when
> > someone has the bright idea to use that variable for some other UI element
> > that coincidentally wants the same value.
> 
> It seems like a trade-off.  Narrowly scoping them has some issues as well:
> * It makes it harder for extension / theme authors to override these values

Providing a future-proof infrastructure for extensions and themes is largely a non-goal. I think the only real use case we care about here is the devtools "theme", which isn't really a theme but a bunch of stylesheets added to the default theme, which is not how actual themes work.

(In reply to Cameron McCormack (:heycam) from comment #12)
> I would encourage you to measure the performance difference between the two
> approaches, if you are making a decision based on that.  It might well be
> negligble. :-)

I bet it is.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > (In reply to Dão Gottwald [:dao] from comment #10)
> > > (In reply to Brian Grinstead [:bgrins] from comment #7)
> > > > I think that scoping it to the root could make things easier for
> > > > maintainability since you don't have to worry about cases where maybe it's
> > > > used outside of the container you thought it would be.
> > > 
> > > It shouldn't be used outside of the container. Enforcing this seems like a
> > > feature, not a bug, as it helps ensure things don't become a mess e.g. when
> > > someone has the bright idea to use that variable for some other UI element
> > > that coincidentally wants the same value.
> > 
> > It seems like a trade-off.  Narrowly scoping them has some issues as well:
> > * It makes it harder for extension / theme authors to override these values
> 
> Providing a future-proof infrastructure for extensions and themes is largely
> a non-goal. I think the only real use case we care about here is the
> devtools "theme", which isn't really a theme but a bunch of stylesheets
> added to the default theme, which is not how actual themes work.

I should be more specific then.  It will be easier to override these inside the devedition theme if they are all on the root.  I also thought that allowing some control with CSS variables could be useful for other theme types (including lw themes maybe in the future) but I didn't realize this wasn't important.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8525670 [details] [diff] [review]
tab-variable.patch

Review of attachment 8525670 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/browser.inc
@@ +13,5 @@
>  %define inAnyPanel :-moz-any(:not([cui-areatype="toolbar"]), [overflowedItem=true])
> +
> +:root {
> +  --tab-toolbar-navbar-overlap: 1px;
> +  --tab-min-height: 31px;

CSS doesn't really belong in a .inc file IMO

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: tab-variable.patch

Moved all the CSS into tabs.inc.css

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8539523 [details] [diff] [review]
tab-variable.patch

Review of attachment 8539523 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay

Comment 19

[Brian Grinstead [:bgrins]]

Looks like this may cause a tart regression on windows: http://compare-talos.mattn.ca/dev/?oldBranch=Fx-Team&oldRevs=5b030e48e8b0&newBranch=Try&newRev=619ec060234f&submit=true

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: tab-variable-no-curves.patch

This seems to fix the tart regressions by not turning the tab curves into a variable yet: http://compare-talos.mattn.ca/dev/?oldBranch=Fx-Team&oldRevs=b7fb384c126f&newBranch=Try&newRev=d3e49639e78a&submit=true

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: interdiff.txt

Interdiff

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8546876 [details] [diff] [review]
> tab-variable-no-curves.patch
> 
> This seems to fix the tart regressions by not turning the tab curves into a
> variable yet:
> http://compare-talos.mattn.ca/dev/?oldBranch=Fx-
> Team&oldRevs=b7fb384c126f&newBranch=Try&newRev=d3e49639e78a&submit=true

Looks like this is good to go.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1d7257edeb4f

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d7257edeb4f