Add resolution units support in css image-set

Currently only the 'x' (short alias for dots per pixel) resolution
unit is supported, and parsing fails for the other units (dppx,
dpi, dpcm).

Based on the image set spec, all the resolution units should be
supported.

This change adds image-set support for all the resolution units, and
adds full support for the 'x' resolution unit instead of treating it
as only a parse time alias of the 'dppx' unit.

Spec definitions:
[1]
"The syntax for image-set() is:

<image-set()> = image-set( <image-set-option># )
<image-set-option> = [ <image> | <string> ]
                     [ <resolution> || type(<string>) ]

Each <string> inside image-set() represents a <url>."

[2]
"Resolution units are dimensions denoted by <resolution>.
The resolution unit identifiers are:
'dpi' - Dots per inch.
'dpcm' - Dots per centimeter.
'dppx', 'x' - Dots per px unit.

The <resolution> unit represents the size of a single 'dot' in a
graphical representation by indicating how many of these dots fit
in a CSS 'in', 'cm', or 'px'.

For uses, see e.g. the resolution media query in [MEDIAQ] or the
image-resolution property defined in [CSS3-IMAGES].

All <resolution> units are compatible, and 'dppx' is their canonical
unit."

[1] https://w3c.github.io/csswg-drafts/css-images-4/#image-set-notation
[2] https://www.w3.org/TR/css-values-4/#resolution-value

R=futhark, masonf, pdr

Bug: 1400903, 1412016
Low-Coverage-Reason: Previous image-set resolution parsing logic in css_parsing_utils.cc
Change-Id: Ic05afa3bc1b5ed6927c309426cd55953da7bf0db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4201198
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100612}

third_party/blink/renderer/core/css/css_image_set_value.cc

@@ -28,6 +28,7 @@
 #include <algorithm>
 #include "third_party/blink/public/common/loader/referrer_utils.h"
 #include "third_party/blink/renderer/core/css/css_image_value.h"
+#include "third_party/blink/renderer/core/css/css_numeric_literal_value.h"
 #include "third_party/blink/renderer/core/css/css_primitive_value.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
@@ -49,18 +50,14 @@ CSSImageSetValue::CSSImageSetValue()
 CSSImageSetValue::~CSSImageSetValue() = default;
 
 void CSSImageSetValue::FillImageSet() {
-  wtf_size_t length = this->length();
-  wtf_size_t i = 0;
-  while (i < length) {
-    wtf_size_t image_index = i;
+  for (wtf_size_t i = 0, length = this->length(); i < length; ++i) {
+    auto image_index = i;
 
     ++i;
     SECURITY_DCHECK(i < length);
-    const auto& scale_factor_value = To<CSSPrimitiveValue>(Item(i));
+    float scale_factor = To<CSSPrimitiveValue>(Item(i)).ComputeDotsPerPixel();
 
-    images_in_set_.push_back(
-        ImageWithScale{image_index, scale_factor_value.GetFloatValue()});
-    ++i;
+    images_in_set_.push_back(ImageWithScale{image_index, scale_factor});
   }
 
   // Sort the images so that they are stored in order from lowest resolution to
@@ -129,9 +126,7 @@ String CSSImageSetValue::CustomCSSText() const {
 
   result.Append("image-set(");
 
-  wtf_size_t length = this->length();
-  wtf_size_t i = 0;
-  while (i < length) {
+  for (wtf_size_t i = 0, length = this->length(); i < length; ++i) {
     if (i > 0) {
       result.Append(", ");
     }
@@ -144,12 +139,6 @@ String CSSImageSetValue::CustomCSSText() const {
     SECURITY_DCHECK(i < length);
     const CSSValue& scale_factor_value = Item(i);
     result.Append(scale_factor_value.CssText());
-    // FIXME: Eventually the scale factor should contain it's own unit
-    // http://wkb.ug/100120.
-    // For now 'x' is hard-coded in the parser, so we hard-code it here too.
-    result.Append('x');
-
-    ++i;
   }
 
   result.Append(')');
@@ -171,12 +160,27 @@ void CSSImageSetValue::TraceAfterDispatch(blink::Visitor* visitor) const {
   CSSValueList::TraceAfterDispatch(visitor);
 }
 
-CSSImageSetValue* CSSImageSetValue::ValueWithURLsMadeAbsolute() {
+CSSImageSetValue* CSSImageSetValue::ComputedCSSValue() {
   auto* value = MakeGarbageCollected<CSSImageSetValue>();
   for (auto& item : *this) {
     auto* image_value = DynamicTo<CSSImageValue>(item.Get());
-    image_value ? value->Append(*image_value->ValueWithURLMadeAbsolute())
-                : value->Append(*item);
+    if (image_value != nullptr) {
+      value->Append(*image_value->ValueWithURLMadeAbsolute());
+      continue;
+    }
+
+    auto* resolution = DynamicTo<CSSNumericLiteralValue>(item.Get());
+    if (resolution != nullptr && resolution->IsResolution() &&
+        resolution->GetType() != CSSPrimitiveValue::UnitType::kDotsPerPixel &&
+        RuntimeEnabledFeatures::CSSImageSetEnabled()) {
+      auto* canonical_resolution = CSSNumericLiteralValue::Create(
+          resolution->ComputeDotsPerPixel(),
+          CSSPrimitiveValue::UnitType::kDotsPerPixel);
+      value->Append(*canonical_resolution);
+      continue;
+    }
+
+    value->Append(*item);
   }
 
   return value;

third_party/blink/renderer/core/css/css_image_set_value.h

@@ -52,7 +52,7 @@ class CORE_EXPORT CSSImageSetValue : public CSSValueList {
 
   String CustomCSSText() const;
 
-  CSSImageSetValue* ValueWithURLsMadeAbsolute();
+  CSSImageSetValue* ComputedCSSValue();
 
   bool HasFailedOrCanceledSubresources() const;
 

third_party/blink/renderer/core/css/css_math_expression_node.cc

@@ -170,6 +170,7 @@ static bool HasDoubleValue(CSSPrimitiveValue::UnitType type) {
     case CSSPrimitiveValue::UnitType::kContainerMin:
     case CSSPrimitiveValue::UnitType::kContainerMax:
     case CSSPrimitiveValue::UnitType::kDotsPerPixel:
+    case CSSPrimitiveValue::UnitType::kX:
     case CSSPrimitiveValue::UnitType::kDotsPerInch:
     case CSSPrimitiveValue::UnitType::kDotsPerCentimeter:
     case CSSPrimitiveValue::UnitType::kFraction:

third_party/blink/renderer/core/css/css_numeric_literal_value.cc

@@ -210,6 +210,7 @@ String CSSNumericLiteralValue::CustomCSSText() const {
     case UnitType::kPixels:
     case UnitType::kCentimeters:
     case UnitType::kDotsPerPixel:
+    case UnitType::kX:
     case UnitType::kDotsPerInch:
     case UnitType::kDotsPerCentimeter:
     case UnitType::kMillimeters:
@@ -306,6 +307,7 @@ bool CSSNumericLiteralValue::Equals(const CSSNumericLiteralValue& other) const {
     case UnitType::kPixels:
     case UnitType::kCentimeters:
     case UnitType::kDotsPerPixel:
+    case UnitType::kX:
     case UnitType::kDotsPerInch:
     case UnitType::kDotsPerCentimeter:
     case UnitType::kMillimeters:

third_party/blink/renderer/core/css/css_primitive_value.cc

@@ -114,6 +114,7 @@ CSSPrimitiveValue::UnitCategory CSSPrimitiveValue::UnitTypeToUnitCategory(
     case UnitType::kKilohertz:
       return CSSPrimitiveValue::kUFrequency;
     case UnitType::kDotsPerPixel:
+    case UnitType::kX:
     case UnitType::kDotsPerInch:
     case UnitType::kDotsPerCentimeter:
       return CSSPrimitiveValue::kUResolution;
@@ -790,6 +791,8 @@ const char* CSSPrimitiveValue::UnitTypeToString(UnitType type) {
       return "cm";
     case UnitType::kDotsPerPixel:
       return "dppx";
+    case UnitType::kX:
+      return "x";
     case UnitType::kDotsPerInch:
       return "dpi";
     case UnitType::kDotsPerCentimeter:

third_party/blink/renderer/core/css/css_primitive_value.h

@@ -142,6 +142,7 @@ class CORE_EXPORT CSSPrimitiveValue : public CSSValue {
     kKilohertz,
     // Resolution
     kDotsPerPixel,
+    kX,  // Short alias for kDotsPerPixel
     kDotsPerInch,
     kDotsPerCentimeter,
     // Other units

third_party/blink/renderer/core/css/css_primitive_value_test.cc

@@ -114,6 +114,8 @@ TEST_F(CSSPrimitiveValueTest, IsResolution) {
   EXPECT_FALSE(Create({5.0, UnitType::kNumber})->IsResolution());
   EXPECT_FALSE(Create({5.0, UnitType::kDegrees})->IsResolution());
   EXPECT_TRUE(Create({5.0, UnitType::kDotsPerPixel})->IsResolution());
+  EXPECT_TRUE(Create({5.0, UnitType::kX})->IsResolution());
+  EXPECT_TRUE(Create({5.0, UnitType::kDotsPerInch})->IsResolution());
   EXPECT_TRUE(Create({5.0, UnitType::kDotsPerCentimeter})->IsResolution());
 }
 

third_party/blink/renderer/core/css/css_primitive_value_units.json5

@@ -84,7 +84,7 @@
     },
     {
       name: "x",
-      unit_type: "kDotsPerPixel",
+      unit_type: "kX",
     },
     {
       name: "vw",

third_party/blink/renderer/core/css/css_value_list.h

@@ -60,6 +60,7 @@ class CORE_EXPORT CSSValueList : public CSSValue {
 
   wtf_size_t length() const { return values_.size(); }
   const CSSValue& Item(wtf_size_t index) const { return *values_[index]; }
+  const CSSValue& First() const { return *values_.front(); }
   const CSSValue& Last() const { return *values_.back(); }
 
   void Append(const CSSValue& value);

third_party/blink/renderer/core/css/cssom/css_numeric_value_type.cc

@@ -80,6 +80,7 @@ CSSNumericValueType::BaseType UnitTypeToBaseType(
     case UnitType::kKilohertz:
       return BaseType::kFrequency;
     case UnitType::kDotsPerPixel:
+    case UnitType::kX:
     case UnitType::kDotsPerInch:
     case UnitType::kDotsPerCentimeter:
       return BaseType::kResolution;

third_party/blink/renderer/core/css/cssom/css_unit_values.h

@@ -291,6 +291,10 @@ class CSSUnitValues {
         value, CSSPrimitiveValue::UnitType::kDotsPerCentimeter);
   }
 
+  static CSSUnitValue* x(double value) {
+    return CSSUnitValue::Create(value, CSSPrimitiveValue::UnitType::kX);
+  }
+
   static CSSUnitValue* dppx(double value) {
     return CSSUnitValue::Create(value,
                                 CSSPrimitiveValue::UnitType::kDotsPerPixel);

third_party/blink/renderer/core/css/parser/css_property_parser_test.cc

@@ -674,101 +674,77 @@ TEST_F(CSSPropertyUseCounterTest, CSSPropertyBackgroundImageImageSet) {
   EXPECT_TRUE(IsCounted(feature));
 }
 
-TEST(CSSPropertyParserTest, ImageSetDefaultResolution) {
+void TestImageSetParsing(const String& testValue,
+                         const String& expectedCssText) {
   const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set(url(foo))",
+      CSSPropertyID::kBackgroundImage, testValue,
       StrictCSSParserContext(SecureContextMode::kSecureContext));
   ASSERT_NE(value, nullptr);
 
   const CSSValueList* val_list = To<CSSValueList>(value);
-  ASSERT_NE(val_list, nullptr);
   ASSERT_EQ(val_list->length(), 1U);
 
   const CSSImageSetValue& image_set_value =
-      To<CSSImageSetValue>(val_list->Last());
-  EXPECT_EQ("image-set(url(\"foo\") 1x)", image_set_value.CustomCSSText());
+      To<CSSImageSetValue>(val_list->First());
+  EXPECT_EQ(expectedCssText, image_set_value.CustomCSSText());
 }
 
-TEST(CSSPropertyParserTest, ImageSetUrlFunction) {
-  const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set(url('foo') 1x)",
-      StrictCSSParserContext(SecureContextMode::kSecureContext));
-  ASSERT_NE(value, nullptr);
-
-  const CSSValueList* val_list = To<CSSValueList>(value);
-  ASSERT_NE(val_list, nullptr);
-  ASSERT_EQ(val_list->length(), 1U);
+TEST(CSSPropertyParserTest, ImageSetDefaultResolution) {
+  TestImageSetParsing("image-set(url(foo))", "image-set(url(\"foo\") 1x)");
+}
 
-  const CSSImageSetValue& image_set_value =
-      To<CSSImageSetValue>(val_list->Last());
-  EXPECT_EQ("image-set(url(\"foo\") 1x)", image_set_value.CustomCSSText());
+TEST(CSSPropertyParserTest, ImageSetResolutionUnitX) {
+  TestImageSetParsing("image-set(url(foo) 3x)", "image-set(url(\"foo\") 3x)");
 }
 
-TEST(CSSPropertyParserTest, ImageSetUrlFunctionEmptyStrUrl) {
-  const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set(url('') 1x)",
-      StrictCSSParserContext(SecureContextMode::kSecureContext));
-  ASSERT_NE(value, nullptr);
+TEST(CSSPropertyParserTest, ImageSetResolutionUnitDppx) {
+  TestImageSetParsing("image-set(url(foo) 3dppx)",
+                      "image-set(url(\"foo\") 3dppx)");
+}
 
-  const CSSValueList* val_list = To<CSSValueList>(value);
-  ASSERT_NE(val_list, nullptr);
-  ASSERT_EQ(val_list->length(), 1U);
+TEST(CSSPropertyParserTest, ImageSetResolutionUnitDpi) {
+  TestImageSetParsing("image-set(url(foo) 96dpi)",
+                      "image-set(url(\"foo\") 96dpi)");
+}
 
-  const CSSImageSetValue& image_set_value =
-      To<CSSImageSetValue>(val_list->Last());
-  EXPECT_EQ("image-set(url(\"\") 1x)", image_set_value.CustomCSSText());
+TEST(CSSPropertyParserTest, ImageSetResolutionUnitDpcm) {
+  TestImageSetParsing("image-set(url(foo) 37dpcm)",
+                      "image-set(url(\"foo\") 37dpcm)");
 }
 
-TEST(CSSPropertyParserTest, ImageSetUrlFunctionNoQuotationMarks) {
-  const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set(url(foo) 1x)",
-      StrictCSSParserContext(SecureContextMode::kSecureContext));
-  ASSERT_NE(value, nullptr);
+TEST(CSSPropertyParserTest, ImageSetUrlFunction) {
+  TestImageSetParsing("image-set(url('foo') 1x)", "image-set(url(\"foo\") 1x)");
+}
 
-  const CSSValueList* val_list = To<CSSValueList>(value);
-  ASSERT_NE(val_list, nullptr);
-  ASSERT_EQ(val_list->length(), 1U);
+TEST(CSSPropertyParserTest, ImageSetUrlFunctionEmptyStrUrl) {
+  TestImageSetParsing("image-set(url('') 1x)", "image-set(url(\"\") 1x)");
+}
 
-  const CSSImageSetValue& image_set_value =
-      To<CSSImageSetValue>(val_list->Last());
-  EXPECT_EQ("image-set(url(\"foo\") 1x)", image_set_value.CustomCSSText());
+TEST(CSSPropertyParserTest, ImageSetUrlFunctionNoQuotationMarks) {
+  TestImageSetParsing("image-set(url(foo) 1x)", "image-set(url(\"foo\") 1x)");
 }
 
 TEST(CSSPropertyParserTest, ImageSetNoUrlFunction) {
-  const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set('foo' 1x)",
-      StrictCSSParserContext(SecureContextMode::kSecureContext));
-  ASSERT_NE(value, nullptr);
-
-  const CSSValueList* val_list = To<CSSValueList>(value);
-  ASSERT_NE(val_list, nullptr);
-  ASSERT_EQ(val_list->length(), 1U);
-
-  const CSSImageSetValue& image_set_value =
-      To<CSSImageSetValue>(val_list->Last());
-  EXPECT_EQ("image-set(url(\"foo\") 1x)", image_set_value.CustomCSSText());
+  TestImageSetParsing("image-set('foo' 1x)", "image-set(url(\"foo\") 1x)");
 }
 
 TEST(CSSPropertyParserTest, ImageSetEmptyStrUrl) {
+  TestImageSetParsing("image-set('' 1x)", "image-set(url(\"\") 1x)");
+}
+
+void TestImageSetParsingFailure(const String& testValue) {
   const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set('' 1x)",
+      CSSPropertyID::kBackgroundImage, testValue,
       StrictCSSParserContext(SecureContextMode::kSecureContext));
-  ASSERT_NE(value, nullptr);
-
-  const CSSValueList* val_list = To<CSSValueList>(value);
-  ASSERT_NE(val_list, nullptr);
-  ASSERT_EQ(val_list->length(), 1U);
+  ASSERT_EQ(value, nullptr);
+}
 
-  const CSSImageSetValue& image_set_value =
-      To<CSSImageSetValue>(val_list->Last());
-  EXPECT_EQ("image-set(url(\"\") 1x)", image_set_value.CustomCSSText());
+TEST(CSSPropertyParserTest, ImageSetEmpty) {
+  TestImageSetParsingFailure("image-set()");
 }
 
 TEST(CSSPropertyParserTest, ImageSetMissingUrl) {
-  const CSSValue* value = CSSParser::ParseSingleValue(
-      CSSPropertyID::kBackgroundImage, "image-set(1x)",
-      StrictCSSParserContext(SecureContextMode::kSecureContext));
-  ASSERT_EQ(value, nullptr);
+  TestImageSetParsingFailure("image-set(1x)");
 }
 
 TEST(CSSPropertyParserTest, InternalLightDarkAuthor) {

third_party/blink/renderer/core/css/properties/css_parsing_utils.cc

@@ -1316,18 +1316,19 @@ CSSPrimitiveValue* ConsumeTime(CSSParserTokenRange& range,
 
 CSSPrimitiveValue* ConsumeResolution(CSSParserTokenRange& range) {
   const CSSParserToken& token = range.Peek();
+
   // Unlike the other types, calc() does not work with <resolution>.
   if (token.GetType() != kDimensionToken) {
     return nullptr;
   }
+
   CSSPrimitiveValue::UnitType unit = token.GetUnitType();
-  if (unit == CSSPrimitiveValue::UnitType::kDotsPerPixel ||
-      unit == CSSPrimitiveValue::UnitType::kDotsPerInch ||
-      unit == CSSPrimitiveValue::UnitType::kDotsPerCentimeter) {
-    return CSSNumericLiteralValue::Create(
-        range.ConsumeIncludingWhitespace().NumericValue(), unit);
+  if (!CSSPrimitiveValue::IsResolution(unit)) {
+    return nullptr;
   }
-  return nullptr;
+
+  return CSSNumericLiteralValue::Create(
+      range.ConsumeIncludingWhitespace().NumericValue(), unit);
 }
 
 // https://drafts.csswg.org/css-values-4/#ratio-value
@@ -3345,23 +3346,19 @@ static CSSValue* ConsumeImageSet(CSSParserTokenRange& range,
     if (args.Peek().GetType() != kDimensionToken &&
         RuntimeEnabledFeatures::CSSImageSetEnabled()) {
       image_set->Append(*CSSNumericLiteralValue::Create(
-          1, CSSPrimitiveValue::UnitType::kNumber));
+          1.0, CSSPrimitiveValue::UnitType::kX));
     } else {
-      const CSSParserToken& token = args.ConsumeIncludingWhitespace();
-
-      if (token.GetType() != kDimensionToken) {
-        return nullptr;
-      }
-      if (token.Value() != "x") {
+      if (args.Peek().GetUnitType() != CSSPrimitiveValue::UnitType::kX &&
+          !RuntimeEnabledFeatures::CSSImageSetEnabled()) {
         return nullptr;
       }
-      DCHECK(token.GetUnitType() == CSSPrimitiveValue::UnitType::kDotsPerPixel);
-      double image_scale_factor = token.NumericValue();
-      if (image_scale_factor <= 0) {
+
+      const CSSPrimitiveValue* resolution = ConsumeResolution(args);
+      if (resolution == nullptr || resolution->GetDoubleValue() <= 0.0) {
         return nullptr;
       }
-      image_set->Append(*CSSNumericLiteralValue::Create(
-          image_scale_factor, CSSPrimitiveValue::UnitType::kNumber));
+
+      image_set->Append(*resolution);
     }
   } while (ConsumeCommaIncludingWhitespace(args));
 

third_party/blink/renderer/core/style/style_fetched_image_set.cc

@@ -81,7 +81,7 @@ CSSValue* StyleFetchedImageSet::CssValue() const {
 CSSValue* StyleFetchedImageSet::ComputedCSSValue(
     const ComputedStyle& style,
     bool allow_visited_style) const {
-  return image_set_value_->ValueWithURLsMadeAbsolute();
+  return image_set_value_->ComputedCSSValue();
 }
 
 bool StyleFetchedImageSet::CanRender() const {