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

[mediaqueries] How do media queries with calc() where it can be resolved early serialize? #1968

Closed
emilio opened this issue Nov 10, 2017 · 25 comments

Comments

@emilio
Copy link
Collaborator

emilio commented Nov 10, 2017

It's not clear to me from the spec text that media queries serialize with the specified or the "computed" value when integers or numbers are involved.

In particular, should these two serialize identically?

<!doctype html>
<style>
  @media (min-aspect-ratio: 1 / 1) { }
  @media (min-aspect-ratio: calc(1) / calc(1)) { }
</style>
<script>
  alert(document.styleSheets[0].cssRules[0].cssText)
  alert(document.styleSheets[0].cssRules[1].cssText)
</script>
@upsuper upsuper added the mediaqueries-4 Current Work label Nov 11, 2017
@frivoal frivoal self-assigned this Dec 14, 2017
@frivoal
Copy link
Collaborator

frivoal commented Dec 14, 2017

Based on the resolution of #434, I think we should do the same here, and simplify (and clamp, if necessary) early.

However, there's one part of the resolution I don't understand, which was highlighted by @tabatkins in that discussion: we simplify and clamp early, but then we put a calc() around the result of the simplification.

If we apply the same logic to media queries, this means that
@media (min-aspect-ratio: calc(3 - 2) / calc(5 - 4)) { }
and
@media (min-aspect-ratio: calc(1) / calc(1)) { }
would serialize the same, but
@media (min-aspect-ratio: 1 / 1) { }
would not.

I am not sure I follow why. @tabatkins can you explain?

@myakura myakura changed the title [css-media-queries] How do media queries with calc() where it can be resolved early serialize? [mediaqueries] How do media queries with calc() where it can be resolved early serialize? Dec 14, 2017
@emilio
Copy link
Collaborator Author

emilio commented Jan 22, 2018

FWIW that's what currently Firefox implements for normal declaration blocks / computed values.

That is:

<!doctype html>
<style>
  #foo { line-height: calc(1 + 3); }
</style>
<script>
  alert(document.styleSheets[0].cssRules[0].cssText)
</script>

Alerts line-height: calc(4) on Firefox, but line-height: 4; on chromium / blink.

For media queries I didn't know whether we should do that or not, because it's not clear whether it's a computed or a specified value (thus this issue).

@frivoal
Copy link
Collaborator

frivoal commented Jan 22, 2018

@tabatkins ping

@tabatkins
Copy link
Member

Yeah, the resolution ended up what in what I think is the best situation - simplify/clamp as early as possible. In MQs, that's always possible, since the allowed relative units can be absolutized immediately.

We retain the calc() wrapper so that the value keeps the same "shape", particularly in TypedOM.

@frivoal
Copy link
Collaborator

frivoal commented Jan 23, 2018

We retain the calc() wrapper so that the value keeps the same "shape", particularly in TypedOM.

That's the part I'm not 100% I understand the benefit of, but alright.

@csnardi
Copy link
Contributor

csnardi commented Jan 24, 2018

For the two browsers that currently implement this (Firefox and WebKit), both examples above serialize to (min-aspect-ratio: 1 / 1). I'm working on implementing this in Chrome, and the current setup would serialize the same as Firefox/WebKit. It would seem this isn't spec-compliant behavior, but since 2/4 browsers are currently shipping without the calc() wrapper, maybe that should be changed?

@frivoal
Copy link
Collaborator

frivoal commented Jan 24, 2018

That sounds worth bring back to the WG. Agenda+

@emilio
Copy link
Collaborator Author

emilio commented Jan 24, 2018

Does WebKit actually ship this? https://bugs.webkit.org/show_bug.cgi?id=181716 was resolved two days ago.

@csnardi
Copy link
Contributor

csnardi commented Jan 24, 2018

My definition of ship may have been a bit loose; it should be in the next Safari TP. (It should also be noted that I was the implementer there)

@emilio
Copy link
Collaborator Author

emilio commented Jan 24, 2018

Now I look at it, FF still doesn't ship this (it'll be on FF59), but will soon (it's the next release).

Yeah, the resolution ended up what in what I think is the best situation - simplify/clamp as early as possible. In MQs, that's always possible, since the allowed relative units can be absolutized immediately.

I'm not sure we should convert relative units to absolute units (even though it's doable, and in fact trivial).

That'd be inconsistent with how every browser serializes something like:

@media (min-width: 10em) {}

(which preserves the unit).

In any case, the following:

@media (min-width: calc(1em + 10px)) {}

Should serialize either as calc(1em + 10px) or as 26px, IMO. @csnardi what does WebKit do there?

Firefox right now preserves the specified value as-is, which is kind of inconsistent with what we do for numbers, because calc(10px) serializes as calc(10px), not as 10px, unlike calc(1), which serializes to 1.

That's an artifact of how it was implemented and I'm happy to make whatever change the WG decides is sane :)

@csnardi
Copy link
Contributor

csnardi commented Jan 24, 2018

@emilio WebKit serializes that as calc(1em + 10px). After more testing, it appears that calc with a length in all properties (in both WebKit and Chrome) is serialized to calc(<length>) whereas calc with a number is serialized to <number>.

@emilio
Copy link
Collaborator Author

emilio commented Jan 24, 2018

Thanks! That matches Firefox as of right now.

Also just realized that we can't just absolutize the lenghts, because the initial font size used for em and such depends on the zoom level, so we would have to reparse the media queries instead of re-evaluate them when the zoom level changes, which would be unnecessarily complex.

@dbaron
Copy link
Member

dbaron commented Feb 6, 2018

It seems like dropping calc() lead to a risk of producing something that's syntactically invalid. For example, the aspect-ratio media feature requires positive integers, but range restrictions generally change from parse-time to compute-time inside of calc() (although is this still true for calc() in media queries?).

This means, I think (but maybe I'm wrong??), that:

@media (aspect-ratio: calc(-1) / calc(-1)) { ... }

is syntactically valid, but if it were serialized as:

@media (aspect-ratio: -1 / -1) { ... }

then that would be an invalid media feature value, which when reparsed and serialized again would produce:

@media not all { ... }

This breaks two of the fundamental principles of serialization (always serialize to valid syntax, serialize to something that when reparsed will serialize to the same thing).

@emilio
Copy link
Collaborator Author

emilio commented Feb 7, 2018

At least Firefox and WebKit (I suspect Blink too), serialize directly to:

@media not all { ... }

Which means that the rule is invalid at parse time. That's probably ok, but it's definitely observable.

IIUC, clamping after parsing would involve evaluating that media query as if it was (aspect-ratio: 1 / 1), right? Happy to make that change.

@css-meeting-bot
Copy link
Member

The Working Group just discussed end.

The full IRC log of that discussion <dael> topic: end
<dael> Rossen_: 2 minutes left. Anyone in APAC hae something that fits in 2 minutes?
<dael> Rossen_: Can we talk about MQ?
<dael> github: https://github.com//issues/1968
<dael> florian: Had a resolution a while back. Turns out impl of MQ do remove the calc when possible. Conflict between what we said and what we're actually doing.
<dael> florian: No opinion on how we could and how we're doing it, there's a mismatch, though.
<dael> Rossen_: Is this impl catching up?
<dael> florian: I don't htink so. It was not considering enough details when we resolved. We may want to maintain resolution but that's more changes. Maybe some people realized all the changes, but some were caught off guard.
<dael> Rossen_: People are starting to fall off, so let's push to next week. It's on people's radar.

@css-meeting-bot
Copy link
Member

The Working Group just discussed [mediaqueries] How do media queries with calc() where it can be resolved early serialize?, and agreed to the following resolutions:

  • RESOLVED: Have calc serialize in MQ same as properties, have that called out in the spec, and then test to see if we can follow that.
The full IRC log of that discussion <dael> Topic: [mediaqueries] How do media queries with calc() where it can be resolved early serialize?
<dael> github: https://github.com//issues/1968
<dael> florian: A few weeks back we decided that calc is supposed to simplify down as soon as it can, but not simplify away.
<dael> florian: Raised against MQ. Impl were new, but 2 new impl we not doing that. Chrome was about to agree to not do that which means remove the calc when serializing. In same discussion it was raised if combining length units early was if we wanted.
<dael> florian: I don't have a stand, but since impl sort of disagree on the resolution we need to revisit. I think TabAtkins thought what w resolved was good.
<dael> TabAtkins: I'm not strong either side. Whatever works.
<dael> florian: Do we have anyone who wanted to impl the other thing?
<dael> astearns: Not enough strong opinions on the call.
<dael> astearns: Reading dbaron comment it sounds like you'd be against dropping calc?
<dael> dbaron: I'm catching up on the issue
<dael> dbaron: I was pointing out one issue wiht dropping calc it sometimes makes a thing valid that wouldn't be valid without the calc. We don't check range restrictions in calc
<dael> florian: IN FF and webkit that's what they do. If you have aspect ratio of -1/-1 with a calc it serialized through as all which is what you'd expect.
<dael> dbaron: Maybe...I don't know how well calc validity rules are spec. Changing validity rule would need to be spec carefully
<dael> florian: In same discussion MQ in em serializes to em but em+px serializes to px in a calc and why.
<dael> florian: Do we need to revisit how calc seralizes in MQ? Happy with existing rules? Revist based on impl?
<dael> florian: Asking differently: when we resolved on rules for calc in general context were people in favor of what we resolved remembering MQ or did we forget and need to investigate?
<dael> astearns: I do not recall MQ being part of that convo
<dael> fremy: I don't see a point of making them different. We don't support calc but if we did it would be same as properties. If no one is arguing different we should stick with what we have.
<dael> florian: Yep. It was pointed out all impl don't do that but it could be a bug. There isn't a problem of web compat because this is new. But if we resolve one thing in spec and everyone impl something else it's not helping.
<dael> florian: IN other words, I'm happy to close won't fix and I'm happy to take tests from fremy and then file bugs one people. Deal?
<dael> fremy: Why not ^-^
<dael> astearns: Proposal is close, have clamping in MQ calc be exactly the same as for property values. We'll then have tests to see what's been impl and see if it matches reality?
<dael> florian: Informal tests say they don't, but formal let's us file bugs.
<dael> astearns: I like not having something special for MQ. But I think it's useful to have something explicitly discuss this in MQ spec so we can hang a test off it.
<dael> florian: Sure...
<dael> florian: I'll phrase it to explicitly refer to the cannonical text. Sounds good.
<dael> astearns: Proposal is have calc serialize in MQ same as properties, have that called out in the spec, and then test to see if we can follow that.
<dael> astearns: Obj?
<dael> REOSLVED: Have calc serialize in MQ same as properties, have that called out in the spec, and then test to see if we can follow that.
<dael> RESOLVED: Have calc serialize in MQ same as properties, have that called out in the spec, and then test to see if we can follow that.

@tabatkins
Copy link
Member

The spec is now clear that math functions are simplified immediately at parse time as much as possible, then again at computed/used value time (which isn't relevant for MQs).

Since MQs can convert all units to their canonical one, this means that all math functions in MQs should be resolvable immediately upon parsing.

Serialization is clear that fully-collapsed calc()s only serialize as their argument (without the calc() wrapper) if you're serializing a computed-time or later value. MQs are not computed or later, so they should never remove their calc() wrapper.

Note that, as currently specified, non-calc() math functions do not disappear if they're top-level even if they can be fully resolved; they retain their identify as the specific function and simplify their arguments. So a sin(.5) will serialize as such, not as .479....

Since this should all be answered in the spec, I'm closing this issue. Feel free to reopen if there are any issues or confusing bits.

@emilio
Copy link
Collaborator Author

emilio commented Oct 4, 2019

@tabatkins so do the new calc() changes mean that stuff like z-index: calc(-1); should produce something like 0 rather than calc(-1) from div.style.zIndex for example?

@emilio
Copy link
Collaborator Author

emilio commented Oct 4, 2019

That's a behavior change, it'd be nice to have tests for it.

@tabatkins
Copy link
Member

(Deleted previous comment, I misread.)

Yes, it does mean that, but also, that's been the documented behavior for a good while; it's not a new consequence of my edits. I can go dig up the resolution covering it if you'd like.

(See https://drafts.csswg.org/date/2019-09-21T18:35:38/css-values-4/#calc-serialize for this being present before my latest round of edits; the original change that added this is four years old.)

@tabatkins
Copy link
Member

OR WAIT DANG IT I STILL MISREAD

No, div.style.zIndex is a specified value, it'll won't drop its calc() wrapper in any circumstance. The dropping only happens at computed-value time or later. (That was also specified four years ago, but in a different commit.)

@emilio
Copy link
Collaborator Author

emilio commented Oct 6, 2019

Ok, that seemed to conflict with your quote above abut "math functions are simplified immediately at parse time as much as possible".

I was reading that as "calc() can go away too if possible" (that's a simplification we do at computed-value time). https://drafts.csswg.org/css-values-4/#calc-computed-value seems to imply that is a valid simplification. It'd be nice to clarify in the spec that dropping calc() at parse time is not a valid simplification.

@tabatkins
Copy link
Member

"Simplified" doesn't have a technical meaning. ^_^

I'm a little confused - you're linking to the text about computed value, but implying it says something about specified value? The "calc() wrapper goes away" isn't even specified in that section, it's part of serialization, and is clear about the condition that it occurs in.

@emilio
Copy link
Collaborator Author

emilio commented Oct 6, 2019

I was linking to the computed value section because it says:

with used value time information, a math function always simplifies down to a single numeric value.

Which to me seemed to imply that simplifying the computed value to a single numeric value (which I expect to mean "drops the calc from the internal representation") was also a valid part of the simplification algorithm referenced from that section.

But if the calc going away is only meant to be a serialization tweak, not part of the simplification algorithm, that's good to know. No further questions spec-wise I guess...

I assume then this is only observable from typed OM... Is that right? My reading of the spec says that line-height: calc(1px) is supposed to be a math function value in typed OM, not a CSSUnitValue, which is 100% not what Blink does... Blink drops the calc() at parse-time. Are there tests for this?

@tabatkins
Copy link
Member

I assume then this is only observable from typed OM... Is that right? My reading of the spec says that line-height: calc(1px) is supposed to be a math function value in typed OM, not a CSSUnitValue, which is 100% not what Blink does... Blink drops the calc() at parse-time. Are there tests for this?

TypedOM doesn't currently have the same "drop the wrapper" rules in its reification stuff, no. It might be better to be consistent there, tho? I don't have a strong preference either way, but I do like consistency when possible.

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

7 participants