Convert tabToolbarNavbarOverlap to a CSS variable

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Theme
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

Trunk
Firefox 38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1095795
(Assignee)

Updated

4 years ago
Depends on: 1088771
(Assignee)

Comment 2

4 years ago
Created attachment 8523298 [details] [diff] [review]
tab-variable.patch

This converts the tab preprocessor directives to css variables.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 1099448
(Assignee)

Comment 3

4 years ago
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
Attachment #8523298 - Flags: feedback?(MattN+bmo)
(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 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.
Attachment #8523298 - Flags: feedback?(MattN+bmo) → feedback+
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
(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);
  }
Flags: needinfo?(cam)
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" }.
Flags: needinfo?(cam)
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.
(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.
(Assignee)

Comment 11

4 years ago
(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.
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. :-)
(Assignee)

Comment 13

4 years ago
Created attachment 8525670 [details] [diff] [review]
tab-variable.patch

This splits variables declarations into two blocks and switches the negation syntax from calc(-1 * foo) instead of calc(0px - foo).
Attachment #8523298 - Attachment is obsolete: true
Attachment #8525670 - Flags: review?(MattN+bmo)
(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.
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1097595
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
Attachment #8525670 - Flags: review?(MattN+bmo) → review-
(Assignee)

Comment 17

3 years ago
Created attachment 8539523 [details] [diff] [review]
tab-variable.patch

Moved all the CSS into tabs.inc.css
Attachment #8525670 - Attachment is obsolete: true
Attachment #8539523 - Flags: review?(MattN+bmo)
Comment on attachment 8539523 [details] [diff] [review]
tab-variable.patch

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

Sorry for the delay
Attachment #8539523 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 20

3 years ago
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
(Assignee)

Comment 21

3 years ago
Created attachment 8546890 [details] [diff] [review]
interdiff.txt

Interdiff
(Assignee)

Comment 22

3 years ago
(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.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8539523 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8546876 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/1d7257edeb4f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1d7257edeb4f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
(Assignee)

Updated

3 years ago
Blocks: 1122726
You need to log in before you can comment on or make changes to this bug.