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.

Sign up for GitHub

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

Jump to bottom

[css-env-1] Meaning of “syntactically valid” in env() has led to differing nonsense behaviour in browsers: it needs clarifying #3792

Closed
chris-morgan opened this issue Apr 2, 2019 · 4 comments
Closed

[css-env-1] Meaning of “syntactically valid” in env() has led to differing nonsense behaviour in browsers: it needs clarifying #3792

chris-morgan opened this issue Apr 2, 2019 · 4 comments

Comments

@chris-morgan
Copy link

Refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1539708#c2 for background. An issue will need to be filed on Chromium, the precise shape of which will depend on the outcome of this issue. Maybe WebKit too; not sure.

The problem relates to https://drafts.csswg.org/css-env-1/#env-function:

If a property contains one or more env() functions, and those functions are syntactically valid, the entire property’s grammar must be assumed to be valid at parse time. It is only syntax-checked at computed-time, after env() functions have been substituted.

I’m not confident in my understanding of the terms involved, but I think that is just saying that you can’t classify a value as invalid out-of-hand at parse time, but will need to wait until you compute it, to decide whether it’s valid. (If that’s not what it does actually mean, I will glibly assert that it’s still what it should mean, and what a non-spec-reading human will expect it to do.)

However, Firefox seems to have only implemented the first part and not compute-time syntax-checking, and Chrome also seems to have missed the compute-time syntax-checking, and moreover sensibly ignored a small part of the stipulated parse time validity checking. Not sure about Safari, as I don’t have access to a Mac.

I’ve created this issue to confirm the correct interpretation, and request that a clarifying note be added to this place in the spec, since this is evidently a fiddly detail for implementers.

I think this is a fairly important issue for css-env, due to interactions with other upcoming specs. The code that precipitated my coworker finding this seems quite reasonable, and was roughly this:

margin-bottom: 15px;
margin-bottom: env(safe-area-inset-bottom, 15px);
margin-bottom: max(env(safe-area-inset-bottom, 15px), 15px);

That one could need to wrap the third line up in @supports (margin-bottom: env(safe-area-inset-bottom)) and (margin-bottom: max(0, 15px)) would be decidedly unfortunate.

Test case 1: env() that is substituted with an invalid value

<body style="background-color:limegreen;background-color:env(bar, 1)">

A human reading this will expect it to be limegreen, since background-color: env(bar, 1) will become, by substitution, background-color: 1, which is invalid.

Firefox accepts the second rule as valid, but equates it to… I’m not actually sure what it is technically, but initial or unset, at a guess? So the limegreen background is overridden, and you get a white background.

Chrome does the same.

Safari I have no access to, so I’m not sure what it may do. (Feel free to update this text with the result of testing it in Safari; or I will if someone comments it.)

Test case 2: env() used inside an unknown function

<body style="margin:10em;margin:foo(env(bar, blue))">

A human reading this will expect margin: foo(env(bar, blue)) to be ignored, since there is no function foo(); and so for the body margin to be 10em.

Firefox treats it as valid but the default value of zero, again.

Chrome decides that since it doesn’t have a foo() function, the value is invalid, and uses 10em. I’d guess it does this at parse time.

Safari I have no access to, so I’m not sure what it may do.
@emilio
Copy link
Collaborator

emilio commented Apr 2, 2019

Chrome decides that since it doesn’t have a foo() function, the value is invalid, and uses 10em. I’d guess it does this at parse time.

This is a Chrome bug, and a fixed one at that (https://bugs.chromium.org/p/chromium/issues/detail?id=921152).

Safari, Firefox, the current spec text, and Chrome (Canary and dev at least, not sure about whether that Chromium patch has made it to a release yet) agree on behavior here.

@SimonSapin filed #3285 with a proposal that I believe would give the behavior you want. I think we should close this as a duplicate of that.

That one could need to wrap the third line up in @supports would be decidedly unfortunate.

I sort of agree on it being unfortunate. I think it's kind of fall-out from env() being initially implemented using the custom properties machinery, and getting an spec retro-fitted... FWIW you only need @supports (margin-bottom: max(0px, 1px)) or such, though, right?

@emilio
Copy link
Collaborator

emilio commented Apr 2, 2019

However, Firefox seems to have only implemented the first part and not compute-time syntax-checking, and Chrome also seems to have missed the compute-time syntax-checking, and moreover sensibly ignored a small part of the stipulated parse time validity checking. Not sure about Safari, as I don’t have access to a Mac.

This is wrong, Firefox does do syntax-checking at computed-value time. The result of it failing however is different from what you expect. From https://drafts.csswg.org/css-variables/#invalid-at-computed-value-time:

When this happens, the computed value of the property is either the property’s inherited value or its initial value depending on whether the property is inherited or not, respectively, as if the property’s value had been specified as the unset keyword.

@chris-morgan
Copy link
Author

chris-morgan commented Apr 2, 2019 •
edited

Alas, I was afraid that my interpretation of the spec would be wrong. Then yes, the current status is that env() behaves in a way that is obviously foolish and counterintuitive to a human. ☹ I guess I shall have to hold out hope for #3285.

FWIW you only need @supports (margin-bottom: max(0px, 1px)) or such, though, right?

If you assume that all environments that support max() also support env(), sure. If not, no. Actually I guess it would be after all.

@chris-morgan chris-morgan mentioned this issue Apr 3, 2019
Open
@emilio
Copy link
Collaborator

emilio commented Apr 3, 2019

Closing as a dupe of #3285 since that's an already-accepted spec change that would address this. Thanks for filing, and let me know if you disagree and we can reopen it or what not.

@emilio emilio closed this as completed Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@chris-morgan @emilio