Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-properties-values-api] inherits should be true by default #434

Closed
LeaVerou opened this issue Jul 29, 2017 · 8 comments
Closed

[css-properties-values-api] inherits should be true by default #434

LeaVerou opened this issue Jul 29, 2017 · 8 comments

Comments

@LeaVerou
Copy link
Member

LeaVerou commented Jul 29, 2017

Spec section: https://www.w3.org/TR/css-properties-values-api-1/#registering-custom-properties

I just had a rather confusing experience using CSS.registerProperty() to make a property animatable. For reference, my code was:

CSS.registerProperty({
	name: "--x",
	syntax: "<number>",
	initialValue: "0"
});

My CSS thus far had been designed with the assumption that the property inherits, which is the default behavior of custom properties. Once CSS.registerProperty() was used, suddenly various parts of my CSS code stopped working. At first I thought I was dealing with an implementation bug! Eventually I realized that inherits is false by default, so my property had stopped inheriting even though I hadn't declared anything about inheritance.

Given that any unregistered properties inherit by default, it doesn't make sense to have a different default for registered properties. I suspect many future authors will face the same confusion as I just did. The defaults of CSS.registerProperty() should be those that cause no changes for things that have not been explicitly specified, whenever possible.

@ghost
Copy link

ghost commented Jul 30, 2017

I was confused by this as well as I was looking at an example and couldn't understand why inherits was set explicitly, since I thought the default was true anyway. I was convinced it was redundant and that I could omit it. It took seeing this issue to figure out that it does need to be set. Unless you want it to be false.

@tabatkins
Copy link
Member

Reason we defaulted it to non-inheriting is because that's the more performant option when you don't need inheritance; inheritance is actually relatively expensive. The confusion y'all bring up is totally reasonable, tho.

We decided to instead just make the "inherits" key required, so you have to specify whether it does or not, and you get an informative error if you leave it out. That should avoid the confusion over omitting it, without defaulting people to the slower option.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed inherits should be true by default (CSS Props and Value), and agreed to the following resolutions:

  • RESOLVED: inherits arg is required
  • RESOLVED: published Properties and Values as WD and announce intent to transition to CR soonish
The full IRC log of that discussion <nainar> Topic: inherits should be true by default (CSS Props and Value)
<nainar> Github: https://github.com//issues/434
<fantasai> [that topic line should shift to replace the ...]
<fantasai> TabAtkins: Question is should registered custom properties be inherited by default
<fantasai> TabAtkins: regular custom properties are inherited by default
<fantasai> TabAtkins: Lea and Anne found it confusing that registered ones don't
<fantasai> TabAtkins: The standard for doing option bags is that an unspecified one defaults to something falsy
<fantasai> TabAtkins: i.e. undef is false
<astearns> s/Anne/Ana/
<fantasai> TabAtkins: if you don't define a flag, we treat it as false
<fantasai> TabAtkins: We could swap the boolean
<fantasai> ....
<dbaron> https://w3ctag.github.io/design-principles/#prefer-dict-to-bool
<fantasai> dbaron: ...
<fantasai> [scintillating naming discussions]
<fantasai> [extra sparkly on acount of all the gold and mirrors]
<fantasai> franremy: Maybe it should be required
<dbaron> ?: can we make the parameter in the dictionary required?
<fantasai> TabAtkins: Seems like best option, to make it a required key
<dbaron> franremy: non-inherited is good for performance
<fantasai> iank_: Good idea to make sure author has to decide
<fantasai> SimonSapin: Gecko uses reset for properties that don't inherit
<fantasai> dbaron: That's not web-exposed though
<surma> s/?/iank_/
<fantasai> TabAtkins: I prefer making arg required, so ppl don't default into non-performant option
<fantasai> RESOLVED: inherits arg is required
<fantasai> [actually that Topic: ... line should be Properties and Values]
<fantasai> iank_: we have just a few more things to fix before shipping
<fantasai> fantasai: So, you're like "we got a few bugs and then we're going to ship it"
<fantasai> fantasai: What's the state of the spec
<fantasai> fantasai: Has it been published recently?
<fantasai> ...
<fantasai> fantasai: If it hasn't been published recently but is reasonably stable, then let's publish ASAP, sendout Last call for Comments announcements, and prep for CR.
<fantasai> fantasai: Any reason not to do that?
<fantasai> RESOLVED: published Properties and Values as WD and announce intent to transition to CR soonish
<fantasai> fantasai: If implementations are getting ready to ship something, and the spec is not in CR, then we should be putting the spec in CR
<fantasai> fantasai: Because the process for doing so triggers review from people who aren't paying attention because last they heard was it was an exploratory early-stage working draft
<SimonSapin> fantasai +1
<fantasai> fantasai: It's better to solicit people's comments before shipping, so we should make an effort to publish and announce the intent to transition before we ship things
<fantasai> (fantasai: Sometimes it's not possible to make the transition, e.g. for Transitons /Transforms / Animations, there's a chunk of work blocking CR even though implementations have shipped already, but in the general case we should try to get the specs to CR ahead of shipping)
<fantasai> This also signals to other implementations who don't have Google's vast resources to rewrite things multiple times that the spec has stabilized and now is a good time to implement :)
<Rossen> Implementation status: Chrome is shipping the an experimental work in Canary. Mozilla has a developer working on it as well. Microsoft and Apple are under consideration

@LeaVerou
Copy link
Member Author

LeaVerou commented Aug 1, 2017

Given that most properties will likely not be registered (since the main benefit of registering is animation) how important is the performance gain on a tiny percentage of properties to warrant the usability hit of lacking sensible defaults? (currently the defaults are not sensible; with this resolution there will be no defaults, so in both cases sensible defaults are lacking)

@FremyCompany
Copy link
Contributor

FremyCompany commented Aug 1, 2017

One of the reasons is that custom properties are currently not being interpolated, so the inherit-induced updates only needs to happen on initial cascade. Otherwise you need to worry about changing its value every frame during animations and transitions. This means invalidating a lot of styles really often.

@tabatkins
Copy link
Member

(Or rather, it only changes once during interpolation. Still much cheaper than updating inherited values every frame.)

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed inherits.

The full IRC log of that discussion <dbaron> Topic: inherits
<franremy> iank_: the issue about { inherits: true } by default
<astearns> https://github.com//issues/434
<dbaron> github: https://github.com//issues/434
<franremy> Rossen: what do we want to discuss further here?
<franremy> Rossen: can we not leave this as a github topic for now
<franremy> iank_: encouraging true by default being a perf footgun is something we really wouldn't like
<franremy> iank_: but if there is strong pushback, we could revisit
<franremy> Rossen: it is not optimal
<franremy> iank_: not strong pushback, so let's keep it as is

@gagyibenedek
Copy link

+1 for what @LeaVerou and @thebabydino are saying. I understand, that it's a performance hit, but on the other hand the use-case most people will use it for is to animate multiple values with the same variable (if there's only one no custom prop is needed), and in this case they have to set it to true anyway.
Making it required seems like a good compromise.

andruud added a commit to andruud/css-houdini-drafts that referenced this issue Jun 27, 2018
shans pushed a commit that referenced this issue Jul 2, 2018
@shans shans closed this as completed Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants