Allow whitespace (or nothing) in the initial-value of @property with universal syntax
#657
Conversation
I know that this isn't 100% correct because we _only_ want to do that
for the `initial-value` property and not for everything.
We also "abuse" the fact that `initial-value:;` results in an
`EndOfInput` error and then we conver that to `Whitespace("")`.
But the idea is as follows:
Instead of skipping whitespace, allow it and even allow "nothing". If we
skip whitespace, the current parser expects a valid token but there is
none and therefore results in an `EndOfInput`.
But in case of `initial-value: ;` or `initial-value:;` these values are
allowed.
Later, we can "ignore" all the whitespace, and only emit a single one
(or nothing) for the `initial-value`.
src/values/syntax.rs
Outdated
| .. | ||
| }) => &Token::WhiteSpace(""), | ||
| Err(e) => return Err(e.into()), | ||
| }, |
There was a problem hiding this comment.
I guess as you mentioned we might want this function to require a value in other cases, e.g. when parsing custom at rules (which also use this grammar). One idea would be to move this to property.rs when parsing the initial-value property. There is already some special handling for universal syntax strings there.
lightningcss/src/rules/property.rs
Lines 78 to 85 in b543fe7
so basically something like
match syntax.parse_value(&mut parser) {
Ok(v) => Some(v),
Err(BasicParseError {
kind: BasicParseErrorKind::EndOfInput,
..
}) => Some(ParsedComponent::Token(Token::WhiteSpace(""))),
Err(e) => return Err(e)
}There was a problem hiding this comment.
or maybe better than checking for an error, you could use parser.is_exhausted() to tell if it's already at the end
There was a problem hiding this comment.
Yep, the only problem I see with parser.is_exchausted() is that it will be true for both initial-value:; and initial-value: ; and in the second case we do want to know the space exists or not to be collected.
The nice part I think is that is_exhausted() would be files if an actual error apart from EndOfInput occured. Therefore we can match all Err safely, I think.
I've updated the code with your suggestions and with the above as well. I think this looks better than the original version I implemented, does this look good to you too now that it is hoisted?
There was a problem hiding this comment.
Wait hold on. I think both :; and : ; should be allowed, but maybe they both end up as : ;. That makes it even simpler.
Let me update the code for that.
There was a problem hiding this comment.
Alright updated. Right now both initial-value:; and initial-value: ; and even initial-value: ; should be parseable. But in all these scenarios we now print it as initial-value: ;.
If you don't agree with this change, we should be able to revert the last commit 👍
This is the better spot because now it is specific to the
`initial-value` of an `@property` with Universal syntax.
Used `is_exhausted` to know that we are "done" or not, it will skip
whitespace so it will be true in both the `initial-value:;` and
`initial-value: ;` case.
Then we still use `parser.next_including_whitespace()` because we are
still interested in the whitespace if there is some (in case of
`initial-value: ;`). If there is no whitespace, then it results in an
end of input error which we can convert to a `WhiteSpace("")` token.
We do capture the error to default to an empty whitespace token, but we
can capture _all_ errors safely because if _something_ was actually
wrong, the wrapping `parser.is_exhausted()` would not have been ok and
therefore false.
RobinMalfait
force-pushed
the
fix/issue-654
branch
from
8796060
to
803c631
Compare
When declaring a
@propertywithsyntax: '*', then theinitial-valuecan be empty or can containe whitespace. Currently this errors with anUnexpected end of input.This PR fixes that by allowing the whitespace in the
initial-valuespot.I know it is 100% correct because we do want to allow whitespace but just not print all of it.
syntax.rsfile and it's not only for theinitial-valuebut for everything that uses that parser withSyntaxString::Universalbut I'm not 100% sure on how to fix this. A new property on the parser? An additional argument with some options?EndOfInputerror in case ofinitial-value:;, this definitely feels hacky but I got it to work this way. Catching theEndOfInputallows us to convert it toWhitespce("")(also hacky).I started with adding the tests so that we can work from there, then updated the parser and the printer.
Looking forward to feedback and obvious improvements!
Fixes: #654