More spec compliant rgb function parsing

NickGerleman · facebook-github-bot · commit 7696073b030a · 2025-01-21T17:39:17.000-08:00
Summary: In the last diff I mixed and matched `<legacy-rgb-syntax>` and `<modern-rgb-syntax>` a bit to keep compatiblity with `normalze-color`. Spec noncompliant values have only been allowed since #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 `<percentage>` or `<number>` compared to legacy functions which only allow the full function to accept one or the other (`normalize-color` doesn't support `<percentage>` 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] Differential Revision: D68468275
diff --git 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 <react/renderer/css/CSSPercentage.h>
 #include <react/renderer/css/CSSSyntaxParser.h>
 #include <react/renderer/css/CSSValueParser.h>
+#include <react/utils/PackTraits.h>
 #include <react/utils/fnv1a.h>
 
 namespace facebook::react {
@@ -33,72 +34,157 @@ constexpr uint8_t clamp255Component(float f) {
   return static_cast<uint8_t>(std::clamp(ceiled, 0, 255));
 }
 
+constexpr std::optional<float> normNumberComponent(
+    const std::variant<std::monostate, CSSNumber>& component) {
+  if (std::holds_alternative<CSSNumber>(component)) {
+    return std::get<CSSNumber>(component).value;
+  }
+
+  return {};
+}
+
+template <typename... ComponentT>
+  requires(
+      (std::is_same_v<CSSNumber, ComponentT> ||
+       std::is_same_v<CSSPercentage, ComponentT>) &&
+      ...)
+constexpr std::optional<float> normComponent(
+    const std::variant<std::monostate, ComponentT...>& component,
+    float fullPercentage) {
+  if constexpr (traits::containsType<CSSPercentage, ComponentT...>()) {
+    if (std::holds_alternative<CSSPercentage>(component)) {
+      return std::get<CSSPercentage>(component).value / 100.0f * fullPercentage;
+    }
+  }
+
+  if constexpr (traits::containsType<CSSNumber, ComponentT...>()) {
+    if (std::holds_alternative<CSSNumber>(component)) {
+      return std::get<CSSNumber>(component).value;
+    }
+  }
+
+  return {};
+}
+
+template <CSSDataType... FirstComponentAllowedTypesT>
+constexpr bool isLegacyColorFunction(CSSSyntaxParser& parser) {
+  auto lookahead = parser;
+  auto next = parseNextCSSValue<FirstComponentAllowedTypesT...>(lookahead);
+  if (std::holds_alternative<std::monostate>(next)) {
+    return false;
+  }
+
+  return lookahead.peekComponentValue<bool>(
+      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 <typename CSSColor>
-constexpr std::optional<CSSColor> parseRgbFunction(CSSSyntaxParser& parser) {
-  auto firstValue = parseNextCSSValue<CSSNumber, CSSPercentage>(parser);
-  if (std::holds_alternative<std::monostate>(firstValue)) {
+constexpr std::optional<CSSColor> parseLegacyRgbFunction(
+    CSSSyntaxParser& parser) {
+  auto rawRed = parseNextCSSValue<CSSNumber, CSSPercentage>(parser);
+  bool usesNumber = std::holds_alternative<CSSNumber>(rawRed);
+
+  auto red = normComponent(rawRed, 255.0f);
+  if (!red.has_value()) {
     return {};
   }
 
-  float redNumber = 0;
-  float greenNumber = 0;
-  float blueNumber = 0;
+  auto green = usesNumber
+      ? normNumberComponent(
+            parseNextCSSValue<CSSNumber>(parser, CSSDelimiter::Comma))
+      : normComponent(
+            parseNextCSSValue<CSSPercentage>(parser, CSSDelimiter::Comma),
+            255.0f);
+  if (!green.has_value()) {
+    return {};
+  }
 
-  if (std::holds_alternative<CSSNumber>(firstValue)) {
-    redNumber = std::get<CSSNumber>(firstValue).value;
+  auto blue = usesNumber
+      ? normNumberComponent(
+            parseNextCSSValue<CSSNumber>(parser, CSSDelimiter::Comma))
+      : normComponent(
+            parseNextCSSValue<CSSPercentage>(parser, CSSDelimiter::Comma),
+            255.0f);
+  if (!blue.has_value()) {
+    return {};
+  }
 
-    auto green =
-        parseNextCSSValue<CSSNumber>(parser, CSSDelimiter::CommaOrWhitespace);
-    if (!std::holds_alternative<CSSNumber>(green)) {
-      return {};
-    }
-    greenNumber = std::get<CSSNumber>(green).value;
+  auto alpha = normComponent(
+      parseNextCSSValue<CSSNumber, CSSPercentage>(parser, CSSDelimiter::Comma),
+      1.0f);
 
-    auto blue =
-        parseNextCSSValue<CSSNumber>(parser, CSSDelimiter::CommaOrWhitespace);
-    if (!std::holds_alternative<CSSNumber>(blue)) {
-      return {};
-    }
-    blueNumber = std::get<CSSNumber>(blue).value;
-  } else {
-    redNumber = std::get<CSSPercentage>(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<uint8_t>(255u),
+  };
+}
 
-    auto green = parseNextCSSValue<CSSPercentage>(
-        parser, CSSDelimiter::CommaOrWhitespace);
-    if (!std::holds_alternative<CSSPercentage>(green)) {
-      return {};
-    }
-    greenNumber = std::get<CSSPercentage>(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 <typename CSSColor>
+constexpr std::optional<CSSColor> parseModernRgbFunction(
+    CSSSyntaxParser& parser) {
+  auto red = normComponent(
+      parseNextCSSValue<CSSNumber, CSSPercentage>(parser), 255.0f);
+  if (!red.has_value()) {
+    return {};
+  }
 
-    auto blue = parseNextCSSValue<CSSPercentage>(
-        parser, CSSDelimiter::CommaOrWhitespace);
-    if (!std::holds_alternative<CSSPercentage>(blue)) {
-      return {};
-    }
-    blueNumber = std::get<CSSPercentage>(blue).value * 2.55f;
+  auto green = normComponent(
+      parseNextCSSValue<CSSNumber, CSSPercentage>(
+          parser, CSSDelimiter::Whitespace),
+      255.0f);
+  if (!green.has_value()) {
+    return {};
   }
 
-  auto alphaValue = parseNextCSSValue<CSSNumber, CSSPercentage>(
-      parser, CSSDelimiter::CommaOrWhitespaceOrSolidus);
+  auto blue = normComponent(
+      parseNextCSSValue<CSSNumber, CSSPercentage>(
+          parser, CSSDelimiter::Whitespace),
+      255.0f);
+  if (!blue.has_value()) {
+    return {};
+  }
 
-  float alphaNumber = std::holds_alternative<std::monostate>(alphaValue) ? 1.0f
-      : std::holds_alternative<CSSNumber>(alphaValue)
-      ? std::get<CSSNumber>(alphaValue).value
-      : std::get<CSSPercentage>(alphaValue).value / 100.0f;
+  auto alpha = normComponent(
+      parseNextCSSValue<CSSNumber, CSSPercentage>(
+          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<uint8_t>(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 <typename CSSColor>
+constexpr std::optional<CSSColor> parseRgbFunction(CSSSyntaxParser& parser) {
+  if (isLegacyColorFunction<CSSNumber, CSSPercentage>(parser)) {
+    return parseLegacyRgbFunction<CSSColor>(parser);
+  } else {
+    return parseModernRgbFunction<CSSColor>(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
@@ -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
@@ -122,13 +122,9 @@ TEST(CSSColor, rgb_rgba_values) {
   EXPECT_EQ(std::get<CSSColor>(modernSyntaxValue).a, 255);
 
   auto mixedDelimeterValue = parseCSSProperty<CSSColor>("rgb(255,255 255)");
-  EXPECT_TRUE(std::holds_alternative<CSSColor>(mixedDelimeterValue));
-  EXPECT_EQ(std::get<CSSColor>(mixedDelimeterValue).r, 255);
-  EXPECT_EQ(std::get<CSSColor>(mixedDelimeterValue).g, 255);
-  EXPECT_EQ(std::get<CSSColor>(mixedDelimeterValue).b, 255);
-  EXPECT_EQ(std::get<CSSColor>(mixedDelimeterValue).a, 255);
+  EXPECT_TRUE(std::holds_alternative<std::monostate>(mixedDelimeterValue));
 
-  auto mixedSpacingValue = parseCSSProperty<CSSColor>("rgb( 5   4,3)");
+  auto mixedSpacingValue = parseCSSProperty<CSSColor>("rgb( 5   4 3)");
   EXPECT_TRUE(std::holds_alternative<CSSColor>(mixedSpacingValue));
   EXPECT_EQ(std::get<CSSColor>(mixedSpacingValue).r, 5);
   EXPECT_EQ(std::get<CSSColor>(mixedSpacingValue).g, 4);
@@ -155,10 +151,19 @@ TEST(CSSColor, rgb_rgba_values) {
   EXPECT_EQ(std::get<CSSColor>(percentageValue).g, 128);
   EXPECT_EQ(std::get<CSSColor>(percentageValue).b, 128);
 
-  auto mixedNumberPercentageValue =
+  auto mixedLegacyNumberPercentageValue =
       parseCSSProperty<CSSColor>("rgb(50%, 0.5, 50%)");
   EXPECT_TRUE(
-      std::holds_alternative<std::monostate>(mixedNumberPercentageValue));
+      std::holds_alternative<std::monostate>(mixedLegacyNumberPercentageValue));
+
+  auto mixedModernNumberPercentageValue =
+      parseCSSProperty<CSSColor>("rgb(50% 0.5 50%)");
+  EXPECT_TRUE(
+      std::holds_alternative<CSSColor>(mixedModernNumberPercentageValue));
+  EXPECT_EQ(std::get<CSSColor>(mixedModernNumberPercentageValue).r, 128);
+  EXPECT_EQ(std::get<CSSColor>(mixedModernNumberPercentageValue).g, 1);
+  EXPECT_EQ(std::get<CSSColor>(mixedModernNumberPercentageValue).b, 128);
+  EXPECT_EQ(std::get<CSSColor>(mixedModernNumberPercentageValue).a, 255);
 
   auto rgbWithNumberAlphaValue =
       parseCSSProperty<CSSColor>("rgb(255 255 255 0.5)");
@@ -169,7 +174,7 @@ TEST(CSSColor, rgb_rgba_values) {
   EXPECT_EQ(std::get<CSSColor>(rgbWithNumberAlphaValue).a, 128);
 
   auto rgbWithPercentageAlphaValue =
-      parseCSSProperty<CSSColor>("rgb(255 255 255, 50%)");
+      parseCSSProperty<CSSColor>("rgb(255 255 255 50%)");
   EXPECT_TRUE(std::holds_alternative<CSSColor>(rgbWithPercentageAlphaValue));
   EXPECT_EQ(std::get<CSSColor>(rgbWithPercentageAlphaValue).r, 255);
   EXPECT_EQ(std::get<CSSColor>(rgbWithPercentageAlphaValue).g, 255);
@@ -184,6 +189,11 @@ TEST(CSSColor, rgb_rgba_values) {
   EXPECT_EQ(std::get<CSSColor>(rgbWithSolidusAlphaValue).b, 255);
   EXPECT_EQ(std::get<CSSColor>(rgbWithSolidusAlphaValue).a, 128);
 
+  auto rgbLegacySyntaxWithSolidusAlphaValue =
+      parseCSSProperty<CSSColor>("rgb(1, 4, 5 /0.5)");
+  EXPECT_TRUE(std::holds_alternative<std::monostate>(
+      rgbLegacySyntaxWithSolidusAlphaValue));
+
   auto rgbaWithSolidusAlphaValue =
       parseCSSProperty<CSSColor>("rgba(255 255 255 / 0.5)");
   EXPECT_TRUE(std::holds_alternative<CSSColor>(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<CSSColor>("");
+
+  [[maybe_unused]] constexpr auto hexColorValue =
+      parseCSSProperty<CSSColor>("#fff");
+
+  [[maybe_unused]] constexpr auto rgbFunctionValue =
       parseCSSProperty<CSSColor>("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
@@ -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<std::string_view>(
       CSSDelimiter::OptionalWhitespace, [](const CSSPreservedToken& token) {
@@ -546,8 +546,7 @@ TEST(CSSSyntaxParser, comma_or_whitespace_or_solidus) {
   EXPECT_EQ(identValue1, "foo");
 
   auto identValue2 = parser.consumeComponentValue<std::string_view>(
-      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<std::string_view>(
-      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<std::string_view>(
-      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<bool>(
-      CSSDelimiter::CommaOrWhitespaceOrSolidus,
+      CSSDelimiter::SolidusOrWhitespace,
       [](const CSSPreservedToken& token) { return true; });
 
   EXPECT_FALSE(delimValue1);
