[css-properties-values-api] Support fallbacks.

According to css-variables-1, any custom property that participates in a
cycle is invalid. This also applies to registered custom properties.
In the current implementation, however, registered custom properties
with an initial value can not become invalid; they compute to their initial
value instead, as provided by registerProperty. A consequence of this, is
that fallbacks (specified by var()-references) are never triggered if the
referenced property is a registered property with an initial value defined.

The value for any unregistered custom property, if no other value is
specified, is the invalid initial value described by css-variables-1.
This means we can just avoid storing the variable on ComputedStyle, to
signify the invalid initial value.

However, the value for any registered custom property, if no other value is
specified, can be the initial value specified by registerProperty. When
there is no value explicitly stored on ComputedStyle, we check
StyleInitialData and fetch the initial value from there. Hence, we can not
use the absence of a value to signify an invalid registered variable, as
we already use this state to mean "initial value from registerProperty".
This means that we must explicitly store a value for registered properties
that participate in a cycle. This CL adds CSSInvalidVariableValue to do
this.

 * When resolving a registered custom property, if a cycle is detected, set
   the registered value to CSSInvalidVariableValue.
 * When looking up a registered custom property, if we already have the
   value CSSInvalidVariableValue, return nullptr instead of initial data.
   This triggers fallbacks.
 * The code that set the cycle_detected flag was weird; a cycle could be
   marked as detected, even though ResolveTokenRange succeeded. This meant
   that any custom property which referenced a property in a cycle would
   also be treated as part of the cycle, which is wrong. Fixed by only
   setting the cycle_detected flag when we have cycle start points.
 * CSSInterpolationType did not initialize its cycle_detected variable,
   which led to undefined behavior.

R=futhark@chromium.org

Bug: 641877
Change-Id: I2c518b176de26f7b2f05b36b568041a228fcb0ea
Reviewed-on: https://chromium-review.googlesource.com/c/1333758
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608014}

third_party/WebKit/LayoutTests/external/wpt/css/css-properties-values-api/var-reference-registered-properties-cycles.html

@@ -25,14 +25,13 @@ test(function() {
     CSS.registerProperty({name: '--registered-1-d', syntax: '<length>', initialValue: '4px', inherits: false});
 
     computedStyle = getComputedStyle(test1);
-    assert_equals(computedStyle.getPropertyValue('--registered-1-a'), '1px');
-    assert_equals(computedStyle.getPropertyValue('--registered-1-b'), '2px');
-
-    assert_equals(computedStyle.getPropertyValue('--registered-1-c'), '2px');
-    assert_equals(computedStyle.getPropertyValue('--registered-1-d'), '2px');
-    assert_equals(computedStyle.getPropertyValue('--unregistered-1-a'), '1px');
-    assert_equals(computedStyle.left, '1px');
-    assert_equals(computedStyle.top, '2px');
+    assert_equals(computedStyle.getPropertyValue('--registered-1-a'), '');
+    assert_equals(computedStyle.getPropertyValue('--registered-1-b'), '');
+    assert_equals(computedStyle.getPropertyValue('--registered-1-c'), '30px');
+    assert_equals(computedStyle.getPropertyValue('--registered-1-d'), '4px');
+    assert_equals(computedStyle.getPropertyValue('--unregistered-1-a'), '');
+    assert_equals(computedStyle.left, '50px');
+    assert_equals(computedStyle.top, '60px');
 }, "A var() cycle between two registered properties is handled correctly.");
 </script>
 
@@ -63,18 +62,18 @@ test(function() {
     CSS.registerProperty({name: '--registered-2-e', syntax: '<length>', initialValue: '5px', inherits: false});
 
     computedStyle = getComputedStyle(test2);
-    assert_equals(computedStyle.getPropertyValue('--registered-2-a'), '1px');
+    assert_equals(computedStyle.getPropertyValue('--registered-2-a'), '');
     assert_equals(computedStyle.getPropertyValue('--unregistered-2-a'), '');
 
-    assert_equals(computedStyle.getPropertyValue('--registered-2-b'), '1px');
-    assert_equals(computedStyle.getPropertyValue('--registered-2-c'), '1px');
+    assert_equals(computedStyle.getPropertyValue('--registered-2-b'), '30px');
+    assert_equals(computedStyle.getPropertyValue('--registered-2-c'), '3px');
     assert_equals(computedStyle.getPropertyValue('--registered-2-d'), '40px');
     assert_equals(computedStyle.getPropertyValue('--registered-2-e'), '5px');
-    assert_equals(computedStyle.getPropertyValue('--unregistered-2-b'), '1px');
-    assert_equals(computedStyle.getPropertyValue('--unregistered-2-c'), '1px');
+    assert_equals(computedStyle.getPropertyValue('--unregistered-2-b'), '50px');
+    assert_equals(computedStyle.getPropertyValue('--unregistered-2-c'), '');
     assert_equals(computedStyle.getPropertyValue('--unregistered-2-d'), '60px');
     assert_equals(computedStyle.getPropertyValue('--unregistered-2-e'), '');
-    assert_equals(computedStyle.left, '1px');
+    assert_equals(computedStyle.left, '70px');
     assert_equals(computedStyle.top, '80px');
 }, "A var() cycle between a registered properties and an unregistered property is handled correctly.");
 </script>

third_party/blink/renderer/core/BUILD.gn

@@ -1729,6 +1729,7 @@ jumbo_source_set("unit_tests") {
     "css/css_computed_style_declaration_test.cc",
     "css/css_font_face_source_test.cc",
     "css/css_gradient_value_test.cc",
+    "css/css_invalid_variable_value_test.cc",
     "css/css_page_rule_test.cc",
     "css/css_paint_value_test.cc",
     "css/css_primitive_value_test.cc",

third_party/blink/renderer/core/animation/css_interpolation_type.cc

@@ -120,7 +120,7 @@ class ResolvedRegisteredCustomPropertyChecker
   bool IsValid(const InterpolationEnvironment& environment,
                const InterpolationValue&) const final {
     DCHECK(ToCSSInterpolationEnvironment(environment).HasVariableResolver());
-    bool cycle_detected;
+    bool cycle_detected = false;
     scoped_refptr<CSSVariableData> resolved_tokens =
         ToCSSInterpolationEnvironment(environment)
             .VariableResolver()
@@ -233,7 +233,7 @@ InterpolationValue CSSInterpolationType::MaybeConvertCustomPropertyDeclaration(
 
   scoped_refptr<CSSVariableData> resolved_tokens;
   if (declaration.Value()->NeedsVariableResolution()) {
-    bool cycle_detected;
+    bool cycle_detected = false;
     resolved_tokens = variable_resolver.ResolveCustomPropertyAnimationKeyframe(
         declaration, cycle_detected);
     DCHECK(!cycle_detected);

third_party/blink/renderer/core/css/BUILD.gn

@@ -92,6 +92,8 @@ blink_core_sources("css") {
     "css_inherited_value.h",
     "css_initial_value.cc",
     "css_initial_value.h",
+    "css_invalid_variable_value.cc",
+    "css_invalid_variable_value.h",
     "css_keyframe_rule.cc",
     "css_keyframe_rule.h",
     "css_keyframes_rule.cc",

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/css/css_invalid_variable_value.h"
+
+#include "third_party/blink/renderer/core/css/css_value_pool.h"
+#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+
+namespace blink {
+
+CSSInvalidVariableValue* CSSInvalidVariableValue::Create() {
+  return CssValuePool().InvalidVariableValue();
+}
+
+String CSSInvalidVariableValue::CustomCSSText() const {
+  return "";
+}
+
+}  // namespace blink

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_INVALID_VARIABLE_VALUE_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_INVALID_VARIABLE_VALUE_H_
+
+#include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/core/css/css_value.h"
+
+namespace blink {
+
+class CORE_EXPORT CSSInvalidVariableValue : public CSSValue {
+ public:
+  static CSSInvalidVariableValue* Create();
+
+  String CustomCSSText() const;
+
+  bool Equals(const CSSInvalidVariableValue&) const { return true; }
+
+  void TraceAfterDispatch(blink::Visitor* visitor) {
+    CSSValue::TraceAfterDispatch(visitor);
+  }
+
+ private:
+  friend class CSSValuePool;
+
+  CSSInvalidVariableValue() : CSSValue(kInvalidVariableValueClass) {}
+};
+
+DEFINE_CSS_VALUE_TYPE_CASTS(CSSInvalidVariableValue, IsInvalidVariableValue());
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_INVALID_VARIABLE_VALUE_H_

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/css/css_invalid_variable_value.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+
+namespace blink {
+namespace {
+
+TEST(CSSInvalidVariableValueTest, Create) {
+  EXPECT_TRUE(CSSInvalidVariableValue::Create());
+}
+
+TEST(CSSInvalidVariableValueTest, Pool) {
+  const CSSInvalidVariableValue* value1 = CSSInvalidVariableValue::Create();
+  const CSSInvalidVariableValue* value2 = CSSInvalidVariableValue::Create();
+  EXPECT_EQ(value1, value2);
+}
+
+TEST(CSSInvalidVariableValueTest, Equals) {
+  const CSSInvalidVariableValue* value1 = CSSInvalidVariableValue::Create();
+  const CSSInvalidVariableValue* value2 = CSSInvalidVariableValue::Create();
+  EXPECT_TRUE(value1->Equals(*value2));
+}
+
+TEST(CSSInvalidVariableValueTest, CustomCSSText) {
+  EXPECT_EQ("", CSSInvalidVariableValue::Create()->CustomCSSText());
+}
+
+}  // namespace
+}  // namespace blink

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

@@ -50,6 +50,7 @@
 #include "third_party/blink/renderer/core/css/css_image_value.h"
 #include "third_party/blink/renderer/core/css/css_inherited_value.h"
 #include "third_party/blink/renderer/core/css/css_initial_value.h"
+#include "third_party/blink/renderer/core/css/css_invalid_variable_value.h"
 #include "third_party/blink/renderer/core/css/css_layout_function_value.h"
 #include "third_party/blink/renderer/core/css/css_paint_value.h"
 #include "third_party/blink/renderer/core/css/css_path_value.h"
@@ -246,6 +247,8 @@ bool CSSValue::operator==(const CSSValue& other) const {
         return CompareCSSValues<CSSVariableReferenceValue>(*this, other);
       case kPendingSubstitutionValueClass:
         return CompareCSSValues<CSSPendingSubstitutionValue>(*this, other);
+      case kInvalidVariableValueClass:
+        return CompareCSSValues<CSSInvalidVariableValue>(*this, other);
     }
     NOTREACHED();
     return false;
@@ -351,6 +354,8 @@ String CSSValue::CssText() const {
       return ToCSSCustomPropertyDeclaration(this)->CustomCSSText();
     case kPendingSubstitutionValueClass:
       return ToCSSPendingSubstitutionValue(this)->CustomCSSText();
+    case kInvalidVariableValueClass:
+      return ToCSSInvalidVariableValue(this)->CustomCSSText();
   }
   NOTREACHED();
   return String();
@@ -503,6 +508,9 @@ void CSSValue::FinalizeGarbageCollectedObject() {
     case kPendingSubstitutionValueClass:
       ToCSSPendingSubstitutionValue(this)->~CSSPendingSubstitutionValue();
       return;
+    case kInvalidVariableValueClass:
+      ToCSSInvalidVariableValue(this)->~CSSInvalidVariableValue();
+      return;
   }
   NOTREACHED();
 }
@@ -653,6 +661,9 @@ void CSSValue::Trace(blink::Visitor* visitor) {
     case kPendingSubstitutionValueClass:
       ToCSSPendingSubstitutionValue(this)->TraceAfterDispatch(visitor);
       return;
+    case kInvalidVariableValueClass:
+      ToCSSInvalidVariableValue(this)->TraceAfterDispatch(visitor);
+      return;
   }
   NOTREACHED();
 }

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

@@ -161,6 +161,9 @@ class CORE_EXPORT CSSValue : public GarbageCollectedFinalized<CSSValue> {
   bool IsPendingSubstitutionValue() const {
     return class_type_ == kPendingSubstitutionValueClass;
   }
+  bool IsInvalidVariableValue() const {
+    return class_type_ == kInvalidVariableValueClass;
+  }
 
   bool HasFailedOrCanceledSubresources() const;
   bool MayContainUrl() const;
@@ -235,6 +238,7 @@ class CORE_EXPORT CSSValue : public GarbageCollectedFinalized<CSSValue> {
     kVariableReferenceClass,
     kCustomPropertyDeclarationClass,
     kPendingSubstitutionValueClass,
+    kInvalidVariableValueClass,
     kLayoutFunctionClass,
 
     kCSSContentDistributionClass,

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

@@ -47,6 +47,7 @@ CSSValuePool::CSSValuePool()
     : inherited_value_(new CSSInheritedValue),
       initial_value_(new CSSInitialValue()),
       unset_value_(new CSSUnsetValue),
+      invalid_variable_value_(new CSSInvalidVariableValue),
       color_transparent_(new CSSColorValue(Color::kTransparent)),
       color_white_(new CSSColorValue(Color::kWhite)),
       color_black_(new CSSColorValue(Color::kBlack)) {
@@ -60,6 +61,7 @@ void CSSValuePool::Trace(blink::Visitor* visitor) {
   visitor->Trace(inherited_value_);
   visitor->Trace(initial_value_);
   visitor->Trace(unset_value_);
+  visitor->Trace(invalid_variable_value_);
   visitor->Trace(color_transparent_);
   visitor->Trace(color_white_);
   visitor->Trace(color_black_);

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

@@ -35,6 +35,7 @@
 #include "third_party/blink/renderer/core/css/css_identifier_value.h"
 #include "third_party/blink/renderer/core/css/css_inherited_value.h"
 #include "third_party/blink/renderer/core/css/css_initial_value.h"
+#include "third_party/blink/renderer/core/css/css_invalid_variable_value.h"
 #include "third_party/blink/renderer/core/css/css_primitive_value.h"
 #include "third_party/blink/renderer/core/css/css_unset_value.h"
 #include "third_party/blink/renderer/core/css/css_value_list.h"
@@ -67,6 +68,9 @@ class CORE_EXPORT CSSValuePool
   CSSInheritedValue* InheritedValue() { return inherited_value_; }
   CSSInitialValue* InitialValue() { return initial_value_; }
   CSSUnsetValue* UnsetValue() { return unset_value_; }
+  CSSInvalidVariableValue* InvalidVariableValue() {
+    return invalid_variable_value_;
+  }
 
   // Vector caches.
   CSSIdentifierValue* IdentifierCacheValue(CSSValueID ident) {
@@ -126,6 +130,7 @@ class CORE_EXPORT CSSValuePool
   Member<CSSInheritedValue> inherited_value_;
   Member<CSSInitialValue> initial_value_;
   Member<CSSUnsetValue> unset_value_;
+  Member<CSSInvalidVariableValue> invalid_variable_value_;
   Member<CSSColorValue> color_transparent_;
   Member<CSSColorValue> color_white_;
   Member<CSSColorValue> color_black_;

third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/css/resolver/css_variable_resolver.h"
 
 #include "third_party/blink/renderer/core/css/css_custom_property_declaration.h"
+#include "third_party/blink/renderer/core/css/css_invalid_variable_value.h"
 #include "third_party/blink/renderer/core/css/css_pending_substitution_value.h"
 #include "third_party/blink/renderer/core/css/css_unset_value.h"
 #include "third_party/blink/renderer/core/css/css_variable_data.h"
@@ -109,11 +110,31 @@ scoped_refptr<CSSVariableData> CSSVariableResolver::ValueForCustomProperty(
 
   CSSVariableData* variable_data = GetVariable(name, registration);
 
-  if (!variable_data)
-    return registration ? registration->InitialVariableData() : nullptr;
+  if (!variable_data) {
+    // For unregistered properties, not having a CSSVariableData here means
+    // that it either never existed, or we have resolved it earlier, but
+    // resolution failed. Either way, we return nullptr to signify that this is
+    // an invalid variable.
+    if (!registration)
+      return nullptr;
+    // For registered properties, it's more complicated. Here too, it can mean
+    // that it never existed, or that resolution failed earlier, but now we need
+    // to know which; in the former case we must provide the initial value, and
+    // in the latter case the variable is invalid.
+    return IsRegisteredVariableInvalid(name, *registration)
+               ? nullptr
+               : registration->InitialVariableData();
+  }
 
-  scoped_refptr<CSSVariableData> resolved_data =
-      ResolveCustomPropertyIfNeeded(name, variable_data, options);
+  bool cycle_detected = false;
+  scoped_refptr<CSSVariableData> resolved_data = ResolveCustomPropertyIfNeeded(
+      name, variable_data, options, cycle_detected);
+
+  if (!resolved_data && cycle_detected) {
+    if (options.absolutize)
+      SetInvalidVariable(name, registration);
+    return nullptr;
+  }
 
   if (resolved_data) {
     if (IsVariableDisallowed(*resolved_data, options, registration))
@@ -198,9 +219,10 @@ scoped_refptr<CSSVariableData> CSSVariableResolver::ResolveCustomProperty(
   bool success = ResolveTokenRange(variable_data.Tokens(), options, result);
   variables_seen_.erase(name);
 
-  if (!success || !cycle_start_points_.IsEmpty()) {
-    cycle_start_points_.erase(name);
+  if (!cycle_start_points_.IsEmpty())
     cycle_detected = true;
+  if (!success || cycle_detected) {
+    cycle_start_points_.erase(name);
     return nullptr;
   }
   cycle_detected = false;
@@ -220,14 +242,14 @@ scoped_refptr<CSSVariableData>
 CSSVariableResolver::ResolveCustomPropertyIfNeeded(
     AtomicString name,
     CSSVariableData* variable_data,
-    const Options& options) {
+    const Options& options,
+    bool& cycle_detected) {
   DCHECK(variable_data);
   bool resolve_urls = ShouldResolveRelativeUrls(name, *variable_data);
   if (!variable_data->NeedsVariableResolution() && !resolve_urls)
     return variable_data;
-  bool unused_cycle_detected;
   return ResolveCustomProperty(name, *variable_data, options, resolve_urls,
-                               unused_cycle_detected);
+                               cycle_detected);
 }
 
 void CSSVariableResolver::ResolveRelativeUrls(
@@ -320,6 +342,25 @@ void CSSVariableResolver::SetRegisteredVariable(
   }
 }
 
+void CSSVariableResolver::SetInvalidVariable(
+    const AtomicString& name,
+    const PropertyRegistration* registration) {
+  // TODO(andruud): Use RemoveVariable instead, but currently it also does
+  // a lookup in the registered map, which seems wasteful.
+  SetVariable(name, registration, nullptr);
+  if (registration) {
+    const CSSValue* value = CSSInvalidVariableValue::Create();
+    SetRegisteredVariable(name, *registration, value);
+  }
+}
+
+bool CSSVariableResolver::IsRegisteredVariableInvalid(
+    const AtomicString& name,
+    const PropertyRegistration& registration) {
+  const CSSValue* value = GetRegisteredVariable(name, registration);
+  return value && value->IsInvalidVariableValue();
+}
+
 bool CSSVariableResolver::ResolveVariableReference(CSSParserTokenRange range,
                                                    const Options& options,
                                                    bool is_env_variable,

third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

@@ -163,7 +163,8 @@ class CORE_EXPORT CSSVariableResolver {
   scoped_refptr<CSSVariableData> ResolveCustomPropertyIfNeeded(
       AtomicString name,
       CSSVariableData*,
-      const Options&);
+      const Options&,
+      bool& cycle_detected);
   // Rewrites (in-place) kUrlTokens and kFunctionToken/CSSValueUrls to contain
   // absolute URLs.
   void ResolveRelativeUrls(Vector<CSSParserToken>& tokens,
@@ -191,6 +192,10 @@ class CORE_EXPORT CSSVariableResolver {
   void SetRegisteredVariable(const AtomicString& name,
                              const PropertyRegistration&,
                              const CSSValue*);
+  void SetInvalidVariable(const AtomicString& name,
+                          const PropertyRegistration*);
+  bool IsRegisteredVariableInvalid(const AtomicString& name,
+                                   const PropertyRegistration&);
 
   const StyleResolverState& state_;
   StyleInheritedVariables* inherited_variables_;