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-rhythm-1] Why is 'none' an alias to '0'? #1151

Closed
kojiishi opened this issue Mar 31, 2017 · 5 comments
Closed

[css-rhythm-1] Why is 'none' an alias to '0'? #1151

kojiishi opened this issue Mar 31, 2017 · 5 comments
Assignees

Comments

@kojiishi
Copy link
Contributor

'none' was added in one of the spec review cycles, when '0' disables the rounding. Tab said we can define a value as not negative, but positive is not possible, so we can't eliminate '0'.

I've got another feedback asking why we need 'none', but I don't have answer.

Can we eliminate this, or is there any good explanation why this is needed?

@fantasai any thoughts?

@tabatkins
Copy link
Member

You can eliminate the zero, just not syntactically - open ranges aren't allowed. You're allowed to say that there's a minimum value (greater than zero, possibly UA-defined) that it's floored to. We just don't like exposing to authors whether a particular UA rounds .000001 differently from .0000001.

@fantasai
Copy link
Collaborator

fantasai commented Mar 31, 2017

@tabatkins There's no reason to floor the value. Rounding to the nearest 0-sized step effectively disables rounding, but mathematically it's continuous and consistent with just rounding to the nearest n-sized step.

@kojiishi We added none because there was discussion of having line-height step take effect might have some behavior side-effects aside from simply enabling stepping. E.g. it might affect line-height: normal calculation differently. (IIRC that was the context of adding none.) As Tab explains, we can't have zero trigger a discretely different behavior than 0.0001.

@RByers
Copy link
Contributor

RByers commented Apr 5, 2017

@kojiishi, are you happy with @fantasai's answer? Obviously if blink is going to ship, then we wont necessarily be able to change this later.

@kojiishi
Copy link
Contributor Author

kojiishi commented Apr 5, 2017

Sorry I was to respond to this but forgotten. Thank you Rick for the reminder.

So none is needed only when we add a feature like #938, that do something differently from just turning off, correct? I didn't understand that way when we added, sorry, but now it makes sense, thank you.

Currently #938 has i18n concern so I suppose it won't make as it is. Tab said he maybe come up with a new proposal that solves i18n concern, but we haven't got to it yet.

I think we can then:

  • Remove none from the spec for now, since it's just an alias currently.
  • When we've got an idea to add features that do differently from just turning off, add none back.

If UA supports none, changing its behavior is breaking, but if none is not parsed now, adding a new value later is easy, correct?

/cc @upsuper

@kojiishi
Copy link
Contributor Author

kojiishi commented Apr 6, 2017

/cc @kuoe0

kojiishi added a commit that referenced this issue Nov 13, 2017
As per the discussion in #1151. The reason we added is gone, and we can add it back if the reason came back.
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

4 participants