From 2405c54ac358c14cec2aca1675ef563b25fb614b Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 23 Jan 2025 14:22:39 -0800 Subject: [PATCH 1/2] Add CSSDelimeter::OptionalWhitespace and CSSDelimeter::CommaOrWhitespaceOrSolidus (#48828) Summary: 1. Rename `CSSComponentValueDelimeter` to `CSSDelimeter` bc the names are getting way too long. 2. Make the distinction between `Whitespace` and `OptionalWhitespace`. Note that for property values, and function blocks, the value parser will already remove trailing/leading whitespace, but it's weird that whitespace unlike others was not required to be present 3. Add `CSSDelimeter::CommaOrWhitespaceOrSolidus` for simpler parsing in the common pattern of alpha values, and move CSSColor function parsing to use that Changelog: [Internal] Reviewed By: lenaic Differential Revision: D68461968 --- .../react/renderer/css/CSSColorFunction.h | 25 +-- .../ReactCommon/react/renderer/css/CSSRatio.h | 6 +- .../react/renderer/css/CSSSyntaxParser.h | 112 ++++++++------ .../react/renderer/css/CSSValueParser.h | 6 +- .../css/tests/CSSSyntaxParserTest.cpp | 142 +++++++++++++----- 5 files changed, 186 insertions(+), 105 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h b/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h index dc9acbb3a04..4a6e2c75c5c 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h @@ -53,15 +53,15 @@ constexpr std::optional parseRgbFunction(CSSSyntaxParser& parser) { if (std::holds_alternative(firstValue)) { redNumber = std::get(firstValue).value; - auto green = parseNextCSSValue( - parser, CSSComponentValueDelimiter::CommaOrWhitespace); + auto green = + parseNextCSSValue(parser, CSSDelimiter::CommaOrWhitespace); if (!std::holds_alternative(green)) { return {}; } greenNumber = std::get(green).value; - auto blue = parseNextCSSValue( - parser, CSSComponentValueDelimiter::CommaOrWhitespace); + auto blue = + parseNextCSSValue(parser, CSSDelimiter::CommaOrWhitespace); if (!std::holds_alternative(blue)) { return {}; } @@ -70,31 +70,22 @@ constexpr std::optional parseRgbFunction(CSSSyntaxParser& parser) { redNumber = std::get(firstValue).value * 2.55f; auto green = parseNextCSSValue( - parser, CSSComponentValueDelimiter::CommaOrWhitespace); + parser, CSSDelimiter::CommaOrWhitespace); if (!std::holds_alternative(green)) { return {}; } greenNumber = std::get(green).value * 2.55f; auto blue = parseNextCSSValue( - parser, CSSComponentValueDelimiter::CommaOrWhitespace); + parser, CSSDelimiter::CommaOrWhitespace); if (!std::holds_alternative(blue)) { return {}; } blueNumber = std::get(blue).value * 2.55f; } - auto alphaValue = peekNextCSSValue( - parser, CSSComponentValueDelimiter::CommaOrWhitespace); - if (!std::holds_alternative(alphaValue)) { - parser.consumeComponentValue(CSSComponentValueDelimiter::CommaOrWhitespace); - } else { - alphaValue = peekNextCSSValue( - parser, CSSComponentValueDelimiter::Solidus); - if (!std::holds_alternative(alphaValue)) { - parser.consumeComponentValue(CSSComponentValueDelimiter::Solidus); - } - } + auto alphaValue = parseNextCSSValue( + parser, CSSDelimiter::CommaOrWhitespaceOrSolidus); float alphaNumber = std::holds_alternative(alphaValue) ? 1.0f : std::holds_alternative(alphaValue) diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h b/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h index 831c594380d..29940a9ddae 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h @@ -35,11 +35,11 @@ struct CSSDataTypeParser { if (isValidRatioPart(token.numericValue())) { float numerator = token.numericValue(); - auto denominator = peekNextCSSValue( - parser, CSSComponentValueDelimiter::Solidus); + auto denominator = + peekNextCSSValue(parser, CSSDelimiter::Solidus); if (std::holds_alternative(denominator) && isValidRatioPart(std::get(denominator).value)) { - parser.consumeComponentValue(CSSComponentValueDelimiter::Solidus); + parser.consumeComponentValue(CSSDelimiter::Solidus); return CSSRatio{numerator, std::get(denominator).value}; } diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h index b278c2683c9..3f4160a0c84 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h @@ -89,11 +89,13 @@ concept CSSUniqueComponentValueVisitors = /** * Describes the delimeter to expect before the next component value. */ -enum class CSSComponentValueDelimiter { - Comma, +enum class CSSDelimiter { Whitespace, - CommaOrWhitespace, + OptionalWhitespace, Solidus, + Comma, + CommaOrWhitespace, + CommaOrWhitespaceOrSolidus, None, }; @@ -142,7 +144,7 @@ class CSSSyntaxParser { */ template constexpr ReturnT consumeComponentValue( - CSSComponentValueDelimiter delimiter, + CSSDelimiter delimiter, const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors); @@ -172,7 +174,7 @@ class CSSSyntaxParser { */ template constexpr ReturnT peekComponentValue( - CSSComponentValueDelimiter delimiter, + CSSDelimiter delimiter, const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors); @@ -229,38 +231,10 @@ struct CSSComponentValueVisitorDispatcher { CSSSyntaxParser& parser; constexpr ReturnT consumeComponentValue( - CSSComponentValueDelimiter delimiter, + CSSDelimiter delimiter, const VisitorsT&... visitors) { - switch (delimiter) { - case CSSComponentValueDelimiter::Comma: - parser.consumeWhitespace(); - if (parser.peek().type() != CSSTokenType::Comma) { - return ReturnT{}; - } - parser.consumeToken(); - parser.consumeWhitespace(); - break; - case CSSComponentValueDelimiter::Whitespace: - parser.consumeWhitespace(); - break; - case CSSComponentValueDelimiter::CommaOrWhitespace: - parser.consumeWhitespace(); - if (parser.peek().type() == CSSTokenType::Comma) { - parser.consumeToken(); - } - parser.consumeWhitespace(); - break; - case CSSComponentValueDelimiter::Solidus: - parser.consumeWhitespace(); - if (parser.peek().type() != CSSTokenType::Delim || - parser.peek().stringValue() != "/") { - return ReturnT{}; - } - parser.consumeToken(); - parser.consumeWhitespace(); - break; - case CSSComponentValueDelimiter::None: - break; + if (!consumeDelimiter(delimiter)) { + return {}; } if (parser.peek().type() == parser.terminator_) { @@ -301,8 +275,62 @@ struct CSSComponentValueVisitorDispatcher { return ReturnT{}; } + /** + * Consume a delimiter, returning false if a required delimiter is not found. + */ + constexpr bool consumeDelimiter(CSSDelimiter delimiter) { + if (delimiter == CSSDelimiter::None) { + return true; + } + + bool hasWhiteSpace = parser.peek().type() == CSSTokenType::WhiteSpace; + parser.consumeWhitespace(); + + switch (delimiter) { + case CSSDelimiter::Comma: + if (parser.peek().type() == CSSTokenType::Comma) { + parser.consumeToken(); + parser.consumeWhitespace(); + return true; + } + return false; + case CSSDelimiter::Whitespace: + return hasWhiteSpace; + case CSSDelimiter::OptionalWhitespace: + return true; + case CSSDelimiter::CommaOrWhitespace: + if (parser.peek().type() == CSSTokenType::Comma) { + parser.consumeToken(); + parser.consumeWhitespace(); + return true; + } + return hasWhiteSpace; + case CSSDelimiter::Solidus: + if (parser.peek().type() == CSSTokenType::Delim && + parser.peek().stringValue() == "/") { + parser.consumeToken(); + parser.consumeWhitespace(); + return true; + } + return false; + case CSSDelimiter::CommaOrWhitespaceOrSolidus: + if (parser.peek().type() == CSSTokenType::Comma || + (parser.peek().type() == CSSTokenType::Delim && + parser.peek().stringValue() == "/")) { + parser.consumeToken(); + parser.consumeWhitespace(); + return true; + } + return hasWhiteSpace; + case CSSDelimiter::None: + return true; + } + + return false; + } + constexpr ReturnT peekComponentValue( - CSSComponentValueDelimiter delimiter, + CSSDelimiter delimiter, const VisitorsT&... visitors) { auto originalParser = parser; auto ret = consumeComponentValue(delimiter, visitors...); @@ -393,7 +421,7 @@ struct CSSComponentValueVisitorDispatcher { template constexpr ReturnT CSSSyntaxParser::consumeComponentValue( - CSSComponentValueDelimiter delimiter, + CSSDelimiter delimiter, const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors) { @@ -407,13 +435,12 @@ constexpr ReturnT CSSSyntaxParser::consumeComponentValue( const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors) { - return consumeComponentValue( - CSSComponentValueDelimiter::None, visitors...); + return consumeComponentValue(CSSDelimiter::None, visitors...); } template constexpr ReturnT CSSSyntaxParser::peekComponentValue( - CSSComponentValueDelimiter delimiter, + CSSDelimiter delimiter, const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors) { @@ -427,8 +454,7 @@ constexpr ReturnT CSSSyntaxParser::peekComponentValue( const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors) { - return peekComponentValue( - CSSComponentValueDelimiter::None, visitors...); + return peekComponentValue(CSSDelimiter::None, visitors...); } } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h index fe45b378c31..7ce6935e7c6 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h @@ -32,7 +32,7 @@ class CSSValueParser { */ template constexpr std::variant consumeValue( - CSSComponentValueDelimiter delimeter = CSSComponentValueDelimiter::None) { + CSSDelimiter delimeter = CSSDelimiter::None) { using ReturnT = std::variant; return parser_.consumeComponentValue( @@ -174,7 +174,7 @@ constexpr auto parseCSSProperty(std::string_view css) template constexpr auto parseNextCSSValue( CSSSyntaxParser& syntaxParser, - CSSComponentValueDelimiter delimeter = CSSComponentValueDelimiter::None) + CSSDelimiter delimeter = CSSDelimiter::None) -> std::variant { detail::CSSValueParser valueParser(syntaxParser); return valueParser.consumeValue(delimeter); @@ -187,7 +187,7 @@ constexpr auto parseNextCSSValue( template constexpr auto peekNextCSSValue( CSSSyntaxParser& syntaxParser, - CSSComponentValueDelimiter delimeter = CSSComponentValueDelimiter::None) + CSSDelimiter delimeter = CSSDelimiter::None) -> std::variant { auto savedParser = syntaxParser; detail::CSSValueParser valueParser(syntaxParser); diff --git a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp index 4bc7136bc2c..dddccd1f09d 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp @@ -14,8 +14,7 @@ TEST(CSSSyntaxParser, simple) { CSSSyntaxParser parser{"1px solid black"}; auto pxValue = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - [](const CSSPreservedToken& token) { + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Dimension); EXPECT_EQ(token.numericValue(), 1.0f); EXPECT_EQ(token.unit(), "px"); @@ -24,9 +23,7 @@ TEST(CSSSyntaxParser, simple) { EXPECT_EQ(pxValue, 1.0f); auto identValue = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - - [](const CSSPreservedToken& token) { + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "solid"); return token.stringValue(); @@ -34,8 +31,7 @@ TEST(CSSSyntaxParser, simple) { EXPECT_EQ(identValue, "solid"); auto identValue2 = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - [](const CSSPreservedToken& token) { + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "black"); return token.stringValue(); @@ -52,7 +48,6 @@ TEST(CSSSyntaxParser, single_function_no_args) { return function.name; auto hasMoreTokens = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(hasMoreTokens); @@ -70,7 +65,7 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { std::vector args; args.emplace_back(blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); @@ -79,7 +74,7 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { })); args.emplace_back(blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); @@ -88,7 +83,7 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { })); args.emplace_back(blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); @@ -97,7 +92,7 @@ TEST(CSSSyntaxParser, single_function_with_whitespace_delimited_args) { })); auto hasMoreTokens = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(hasMoreTokens); @@ -119,7 +114,7 @@ TEST(CSSSyntaxParser, single_function_with_comma_delimited_args) { std::array rgb{}; rgb[0] = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); EXPECT_EQ(token.numericValue(), 100); @@ -127,23 +122,21 @@ TEST(CSSSyntaxParser, single_function_with_comma_delimited_args) { }); rgb[1] = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Comma, - [](const CSSPreservedToken& token) { + CSSDelimiter::Comma, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); EXPECT_EQ(token.numericValue(), 200); return static_cast(token.numericValue()); }); rgb[2] = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Comma, - [](const CSSPreservedToken& token) { + CSSDelimiter::Comma, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); EXPECT_EQ(token.numericValue(), 50); return static_cast(token.numericValue()); }); auto hasMoreTokens = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(hasMoreTokens); @@ -165,15 +158,14 @@ TEST(CSSSyntaxParser, single_function_with_mixed_delimited_args) { std::array rgb{}; rgb[0] = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::None, - [](const CSSPreservedToken& token) { + CSSDelimiter::None, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); EXPECT_EQ(token.numericValue(), 100); return static_cast(token.numericValue()); }); rgb[1] = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::CommaOrWhitespace, + CSSDelimiter::CommaOrWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); EXPECT_EQ(token.numericValue(), 200); @@ -181,7 +173,7 @@ TEST(CSSSyntaxParser, single_function_with_mixed_delimited_args) { }); rgb[2] = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::CommaOrWhitespace, + CSSDelimiter::CommaOrWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Number); EXPECT_EQ(token.numericValue(), 50); @@ -189,7 +181,7 @@ TEST(CSSSyntaxParser, single_function_with_mixed_delimited_args) { }); auto hasMoreTokens = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(hasMoreTokens); @@ -208,7 +200,7 @@ TEST(CSSSyntaxParser, complex) { [&](const CSSFunctionBlock& function, CSSSyntaxParser& blockParser) { EXPECT_EQ(function.name, "foo"); auto identArg = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "a"); @@ -217,13 +209,13 @@ TEST(CSSSyntaxParser, complex) { EXPECT_EQ(identArg, "a"); auto barFunc = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [&](const CSSFunctionBlock& function, CSSSyntaxParser& nestedBlockParser) { EXPECT_EQ(function.name, "bar"); auto hasMoreTokens = nestedBlockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(hasMoreTokens); @@ -232,7 +224,7 @@ TEST(CSSSyntaxParser, complex) { EXPECT_EQ(barFunc, "bar"); auto hasMoreTokens = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, + CSSDelimiter::Whitespace, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(hasMoreTokens); @@ -248,8 +240,7 @@ TEST(CSSSyntaxParser, complex) { EXPECT_EQ(bazFunc, "baz"); auto pxValue = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - [](const CSSPreservedToken& token) { + CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Dimension); EXPECT_EQ(token.numericValue(), 12.0f); EXPECT_EQ(token.unit(), "px"); @@ -366,8 +357,7 @@ TEST(CSSSyntaxParser, whitespace_surrounding_function_args) { EXPECT_EQ(function.name, "foo"); auto identArg = blockParser.consumeComponentValue( - CSSComponentValueDelimiter::None, - [](const CSSPreservedToken& token) { + CSSDelimiter::None, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "a"); return token.stringValue(); @@ -435,8 +425,7 @@ TEST(CSSSyntaxParser, preserved_token_without_visitor_consumed) { parser.consumeComponentValue(); auto identValue = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - [](const CSSPreservedToken& token) { + CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "bar"); return token.stringValue(); @@ -451,8 +440,7 @@ TEST(CSSSyntaxParser, function_without_visitor_consumed) { parser.consumeComponentValue(); auto identValue = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - [](const CSSPreservedToken& token) { + CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "bar"); return token.stringValue(); @@ -467,8 +455,7 @@ TEST(CSSSyntaxParser, simple_block_without_visitor_consumed) { parser.consumeComponentValue(); auto identValue = parser.consumeComponentValue( - CSSComponentValueDelimiter::Whitespace, - [](const CSSPreservedToken& token) { + CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "bar"); return token.stringValue(); @@ -490,7 +477,7 @@ TEST(CSSSyntaxParser, solidus_delimiter) { EXPECT_EQ(identValue, "foo"); auto identValue2 = parser.consumeComponentValue( - CSSComponentValueDelimiter::Solidus, [](const CSSPreservedToken& token) { + CSSDelimiter::Solidus, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "bar"); return token.stringValue(); @@ -512,10 +499,87 @@ TEST(CSSSyntaxParser, solidus_delimiter_not_present) { EXPECT_EQ(identValue, "foo"); auto identValue2 = parser.consumeComponentValue( - CSSComponentValueDelimiter::Solidus, + CSSDelimiter::Solidus, [](const CSSPreservedToken& /*token*/) { return true; }); EXPECT_FALSE(identValue2); } +TEST(CSSSyntaxParser, required_whitespace_not_present) { + CSSSyntaxParser parser{"foo/"}; + + auto identValue = parser.consumeComponentValue( + [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "foo"); + return token.stringValue(); + }); + + EXPECT_EQ(identValue, "foo"); + + auto delimValue1 = parser.consumeComponentValue( + CSSDelimiter::Whitespace, + [](const CSSPreservedToken& /*token*/) { return true; }); + + EXPECT_FALSE(delimValue1); + + auto delimValue2 = parser.consumeComponentValue( + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Delim); + EXPECT_EQ(token.stringValue(), "/"); + return token.stringValue(); + }); + + EXPECT_EQ(delimValue2, "/"); +} + +TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) { + CSSSyntaxParser parser{"foo, bar / baz potato%"}; + + auto identValue1 = parser.consumeComponentValue( + CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "foo"); + return token.stringValue(); + }); + + EXPECT_EQ(identValue1, "foo"); + + auto identValue2 = parser.consumeComponentValue( + CSSDelimiter::CommaOrWhitespaceOrSolidus, + [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "bar"); + return token.stringValue(); + }); + + EXPECT_EQ(identValue2, "bar"); + + auto identValue3 = parser.consumeComponentValue( + CSSDelimiter::CommaOrWhitespaceOrSolidus, + [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "baz"); + return token.stringValue(); + }); + + EXPECT_EQ(identValue3, "baz"); + + auto identValue4 = parser.consumeComponentValue( + CSSDelimiter::CommaOrWhitespaceOrSolidus, + [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "potato"); + return token.stringValue(); + }); + + EXPECT_EQ(identValue4, "potato"); + + auto delimValue1 = parser.consumeComponentValue( + CSSDelimiter::CommaOrWhitespaceOrSolidus, + [](const CSSPreservedToken& token) { return true; }); + + EXPECT_FALSE(delimValue1); +} + } // namespace facebook::react From 3f49c5dac0dea5cb58fb8a151bc4740c4883c1e4 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 23 Jan 2025 14:22:39 -0800 Subject: [PATCH 2/2] More spec compliant rgb function parsing (#48839) Summary: In the last diff I mixed and matched `` and `` a bit to keep compatiblity with `normalze-color`. Spec noncompliant values have only been allowed since https://github.com/facebook/react-native/pull/34600 with the main issue being that legacy syntax rgb functions are allowed to use the `/` based alpha syntax, and commas can be mixed with whitespace. This seems like an exceedingly rare real-world scenario (there are currently zero usages of slash syntax in RKJSModules validated by `rgb\([^\)]*/`), so I'm going to instead just follow the spec for more sanity. Another bit that I missed was that modern RGB functions allow individual components to be `` or `` compared to legacy functions which only allow the full function to accept one or the other (`normalize-color` doesn't support `` at all), so I fixed that as well. I started sharing a little bit more of the logic here, to make things more readable when adding more functions. Changelog: [Internal] Reviewed By: javache Differential Revision: D68468275 --- .../react/renderer/css/CSSColorFunction.h | 182 +++++++++++++----- .../react/renderer/css/CSSSyntaxParser.h | 9 +- .../react/renderer/css/tests/CSSColorTest.cpp | 35 +++- .../css/tests/CSSSyntaxParserTest.cpp | 15 +- 4 files changed, 169 insertions(+), 72 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h b/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h index 4a6e2c75c5c..82af245a6a9 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSColorFunction.h @@ -17,6 +17,7 @@ #include #include #include +#include #include namespace facebook::react { @@ -33,72 +34,157 @@ constexpr uint8_t clamp255Component(float f) { return static_cast(std::clamp(ceiled, 0, 255)); } +constexpr std::optional normalizeNumberComponent( + const std::variant& component) { + if (std::holds_alternative(component)) { + return std::get(component).value; + } + + return {}; +} + +template + requires( + (std::is_same_v || + std::is_same_v) && + ...) +constexpr std::optional normalizeComponent( + const std::variant& component, + float baseValue) { + if constexpr (traits::containsType()) { + if (std::holds_alternative(component)) { + return std::get(component).value / 100.0f * baseValue; + } + } + + if constexpr (traits::containsType()) { + if (std::holds_alternative(component)) { + return std::get(component).value; + } + } + + return {}; +} + +template +constexpr bool isLegacyColorFunction(CSSSyntaxParser& parser) { + auto lookahead = parser; + auto next = parseNextCSSValue(lookahead); + if (std::holds_alternative(next)) { + return false; + } + + return lookahead.consumeComponentValue( + CSSDelimiter::OptionalWhitespace, [](CSSPreservedToken token) { + return token.type() == CSSTokenType::Comma; + }); +} + /** - * Parses an rgb() or rgba() function and returns a CSSColor if it is valid. - * Some invalid syntax (like mixing commas and whitespace) are allowed for - * backwards compatibility with normalize-color. - * https://www.w3.org/TR/css-color-4/#funcdef-rgb + * Parses a legacy syntax rgb() or rgba() function and returns a CSSColor if it + * is valid. + * https://www.w3.org/TR/css-color-4/#typedef-legacy-rgb-syntax */ template -constexpr std::optional parseRgbFunction(CSSSyntaxParser& parser) { - auto firstValue = parseNextCSSValue(parser); - if (std::holds_alternative(firstValue)) { +constexpr std::optional parseLegacyRgbFunction( + CSSSyntaxParser& parser) { + auto rawRed = parseNextCSSValue(parser); + bool usesNumber = std::holds_alternative(rawRed); + + auto red = normalizeComponent(rawRed, 255.0f); + if (!red.has_value()) { return {}; } - float redNumber = 0; - float greenNumber = 0; - float blueNumber = 0; + auto green = usesNumber + ? normalizeNumberComponent( + parseNextCSSValue(parser, CSSDelimiter::Comma)) + : normalizeComponent( + parseNextCSSValue(parser, CSSDelimiter::Comma), + 255.0f); + if (!green.has_value()) { + return {}; + } - if (std::holds_alternative(firstValue)) { - redNumber = std::get(firstValue).value; + auto blue = usesNumber + ? normalizeNumberComponent( + parseNextCSSValue(parser, CSSDelimiter::Comma)) + : normalizeComponent( + parseNextCSSValue(parser, CSSDelimiter::Comma), + 255.0f); + if (!blue.has_value()) { + return {}; + } - auto green = - parseNextCSSValue(parser, CSSDelimiter::CommaOrWhitespace); - if (!std::holds_alternative(green)) { - return {}; - } - greenNumber = std::get(green).value; + auto alpha = normalizeComponent( + parseNextCSSValue(parser, CSSDelimiter::Comma), + 1.0f); - auto blue = - parseNextCSSValue(parser, CSSDelimiter::CommaOrWhitespace); - if (!std::holds_alternative(blue)) { - return {}; - } - blueNumber = std::get(blue).value; - } else { - redNumber = std::get(firstValue).value * 2.55f; + return CSSColor{ + .r = clamp255Component(*red), + .g = clamp255Component(*green), + .b = clamp255Component(*blue), + .a = alpha.has_value() ? clamp255Component(*alpha * 255.0f) + : static_cast(255u), + }; +} - auto green = parseNextCSSValue( - parser, CSSDelimiter::CommaOrWhitespace); - if (!std::holds_alternative(green)) { - return {}; - } - greenNumber = std::get(green).value * 2.55f; +/** + * Parses a modern syntax rgb() or rgba() function and returns a CSSColor if it + * is valid. + * https://www.w3.org/TR/css-color-4/#typedef-modern-rgb-syntax + */ +template +constexpr std::optional parseModernRgbFunction( + CSSSyntaxParser& parser) { + auto red = normalizeComponent( + parseNextCSSValue(parser), 255.0f); + if (!red.has_value()) { + return {}; + } - auto blue = parseNextCSSValue( - parser, CSSDelimiter::CommaOrWhitespace); - if (!std::holds_alternative(blue)) { - return {}; - } - blueNumber = std::get(blue).value * 2.55f; + auto green = normalizeComponent( + parseNextCSSValue( + parser, CSSDelimiter::Whitespace), + 255.0f); + if (!green.has_value()) { + return {}; } - auto alphaValue = parseNextCSSValue( - parser, CSSDelimiter::CommaOrWhitespaceOrSolidus); + auto blue = normalizeComponent( + parseNextCSSValue( + parser, CSSDelimiter::Whitespace), + 255.0f); + if (!blue.has_value()) { + return {}; + } - float alphaNumber = std::holds_alternative(alphaValue) ? 1.0f - : std::holds_alternative(alphaValue) - ? std::get(alphaValue).value - : std::get(alphaValue).value / 100.0f; + auto alpha = normalizeComponent( + parseNextCSSValue( + parser, CSSDelimiter::SolidusOrWhitespace), + 1.0f); return CSSColor{ - .r = clamp255Component(redNumber), - .g = clamp255Component(greenNumber), - .b = clamp255Component(blueNumber), - .a = clamp255Component(alphaNumber * 255.0f), + .r = clamp255Component(*red), + .g = clamp255Component(*green), + .b = clamp255Component(*blue), + .a = alpha.has_value() ? clamp255Component(*alpha * 255.0f) + : static_cast(255u), }; } + +/** + * Parses an rgb() or rgba() function and returns a CSSColor if it is valid. + * https://www.w3.org/TR/css-color-4/#funcdef-rgb + */ +template +constexpr std::optional parseRgbFunction(CSSSyntaxParser& parser) { + if (isLegacyColorFunction(parser)) { + return parseLegacyRgbFunction(parser); + } else { + return parseModernRgbFunction(parser); + } +} } // namespace detail /** diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h index 3f4160a0c84..5eea737d7fd 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h @@ -93,9 +93,9 @@ enum class CSSDelimiter { Whitespace, OptionalWhitespace, Solidus, + SolidusOrWhitespace, Comma, CommaOrWhitespace, - CommaOrWhitespaceOrSolidus, None, }; @@ -313,10 +313,9 @@ struct CSSComponentValueVisitorDispatcher { return true; } return false; - case CSSDelimiter::CommaOrWhitespaceOrSolidus: - if (parser.peek().type() == CSSTokenType::Comma || - (parser.peek().type() == CSSTokenType::Delim && - parser.peek().stringValue() == "/")) { + case CSSDelimiter::SolidusOrWhitespace: + if (parser.peek().type() == CSSTokenType::Delim && + parser.peek().stringValue() == "/") { parser.consumeToken(); parser.consumeWhitespace(); return true; diff --git a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSColorTest.cpp b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSColorTest.cpp index 3935b400669..ce928caadc0 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSColorTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSColorTest.cpp @@ -122,13 +122,9 @@ TEST(CSSColor, rgb_rgba_values) { EXPECT_EQ(std::get(modernSyntaxValue).a, 255); auto mixedDelimeterValue = parseCSSProperty("rgb(255,255 255)"); - EXPECT_TRUE(std::holds_alternative(mixedDelimeterValue)); - EXPECT_EQ(std::get(mixedDelimeterValue).r, 255); - EXPECT_EQ(std::get(mixedDelimeterValue).g, 255); - EXPECT_EQ(std::get(mixedDelimeterValue).b, 255); - EXPECT_EQ(std::get(mixedDelimeterValue).a, 255); + EXPECT_TRUE(std::holds_alternative(mixedDelimeterValue)); - auto mixedSpacingValue = parseCSSProperty("rgb( 5 4,3)"); + auto mixedSpacingValue = parseCSSProperty("rgb( 5 4 3)"); EXPECT_TRUE(std::holds_alternative(mixedSpacingValue)); EXPECT_EQ(std::get(mixedSpacingValue).r, 5); EXPECT_EQ(std::get(mixedSpacingValue).g, 4); @@ -155,10 +151,19 @@ TEST(CSSColor, rgb_rgba_values) { EXPECT_EQ(std::get(percentageValue).g, 128); EXPECT_EQ(std::get(percentageValue).b, 128); - auto mixedNumberPercentageValue = + auto mixedLegacyNumberPercentageValue = parseCSSProperty("rgb(50%, 0.5, 50%)"); EXPECT_TRUE( - std::holds_alternative(mixedNumberPercentageValue)); + std::holds_alternative(mixedLegacyNumberPercentageValue)); + + auto mixedModernNumberPercentageValue = + parseCSSProperty("rgb(50% 0.5 50%)"); + EXPECT_TRUE( + std::holds_alternative(mixedModernNumberPercentageValue)); + EXPECT_EQ(std::get(mixedModernNumberPercentageValue).r, 128); + EXPECT_EQ(std::get(mixedModernNumberPercentageValue).g, 1); + EXPECT_EQ(std::get(mixedModernNumberPercentageValue).b, 128); + EXPECT_EQ(std::get(mixedModernNumberPercentageValue).a, 255); auto rgbWithNumberAlphaValue = parseCSSProperty("rgb(255 255 255 0.5)"); @@ -169,7 +174,7 @@ TEST(CSSColor, rgb_rgba_values) { EXPECT_EQ(std::get(rgbWithNumberAlphaValue).a, 128); auto rgbWithPercentageAlphaValue = - parseCSSProperty("rgb(255 255 255, 50%)"); + parseCSSProperty("rgb(255 255 255 50%)"); EXPECT_TRUE(std::holds_alternative(rgbWithPercentageAlphaValue)); EXPECT_EQ(std::get(rgbWithPercentageAlphaValue).r, 255); EXPECT_EQ(std::get(rgbWithPercentageAlphaValue).g, 255); @@ -184,6 +189,11 @@ TEST(CSSColor, rgb_rgba_values) { EXPECT_EQ(std::get(rgbWithSolidusAlphaValue).b, 255); EXPECT_EQ(std::get(rgbWithSolidusAlphaValue).a, 128); + auto rgbLegacySyntaxWithSolidusAlphaValue = + parseCSSProperty("rgb(1, 4, 5 /0.5)"); + EXPECT_TRUE(std::holds_alternative( + rgbLegacySyntaxWithSolidusAlphaValue)); + auto rgbaWithSolidusAlphaValue = parseCSSProperty("rgba(255 255 255 / 0.5)"); EXPECT_TRUE(std::holds_alternative(rgbaWithSolidusAlphaValue)); @@ -236,7 +246,12 @@ TEST(CSSColor, rgb_rgba_values) { } TEST(CSSColor, constexpr_values) { - [[maybe_unused]] constexpr auto simpleValue = + [[maybe_unused]] constexpr auto emptyValue = parseCSSProperty(""); + + [[maybe_unused]] constexpr auto hexColorValue = + parseCSSProperty("#fff"); + + [[maybe_unused]] constexpr auto rgbFunctionValue = parseCSSProperty("rgb(255, 255, 255)"); } diff --git a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp index dddccd1f09d..9dc8aa6721a 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp @@ -533,8 +533,8 @@ TEST(CSSSyntaxParser, required_whitespace_not_present) { EXPECT_EQ(delimValue2, "/"); } -TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) { - CSSSyntaxParser parser{"foo, bar / baz potato%"}; +TEST(CSSSyntaxParser, solidus_or_whitespace) { + CSSSyntaxParser parser{"foo bar / baz potato, papaya"}; auto identValue1 = parser.consumeComponentValue( CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) { @@ -546,8 +546,7 @@ TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) { EXPECT_EQ(identValue1, "foo"); auto identValue2 = parser.consumeComponentValue( - CSSDelimiter::CommaOrWhitespaceOrSolidus, - [](const CSSPreservedToken& token) { + CSSDelimiter::SolidusOrWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "bar"); return token.stringValue(); @@ -556,8 +555,7 @@ TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) { EXPECT_EQ(identValue2, "bar"); auto identValue3 = parser.consumeComponentValue( - CSSDelimiter::CommaOrWhitespaceOrSolidus, - [](const CSSPreservedToken& token) { + CSSDelimiter::SolidusOrWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "baz"); return token.stringValue(); @@ -566,8 +564,7 @@ TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) { EXPECT_EQ(identValue3, "baz"); auto identValue4 = parser.consumeComponentValue( - CSSDelimiter::CommaOrWhitespaceOrSolidus, - [](const CSSPreservedToken& token) { + CSSDelimiter::SolidusOrWhitespace, [](const CSSPreservedToken& token) { EXPECT_EQ(token.type(), CSSTokenType::Ident); EXPECT_EQ(token.stringValue(), "potato"); return token.stringValue(); @@ -576,7 +573,7 @@ TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) { EXPECT_EQ(identValue4, "potato"); auto delimValue1 = parser.consumeComponentValue( - CSSDelimiter::CommaOrWhitespaceOrSolidus, + CSSDelimiter::SolidusOrWhitespace, [](const CSSPreservedToken& token) { return true; }); EXPECT_FALSE(delimValue1);