Skip to content

GitLab

T
tangled
Commit a634522b authored by Anders Hartvoll Ruud's avatar Anders Hartvoll Ruud Committed by Commit Bot
Browse files
Options

CSS Typed OM: Proper types for custom props during iteration. 

Currently, you get CSSUnparsedValues when iterating entries on the
.computedStyleMap(), even for properties which are registered with a
type. However, if you do .computedStyleMap().get(...) with a registered
property, you _do_ get the correct type.

This is because ComputedStyleCSSValueMapping::Get properly looks up the
typed values on the ComputedStyle, but ::GetVariables does not; instead it
directly returns the CSSVariableData for everything, regardless of
registrations.

This patch changes the behavior of iteration to use the same code path
as .get(). This ensures that we either create a CSSCustomProperty-
Declaration on the fly for unregistered properties, or we re-use the
existing (and typed) CSSValue for registered properties.

Note: I initially wanted to do a nested iterator thing here, but dropped
      it because we would anyway need a temp HashSet to avoid potential
      duplicates from the StyleInheritedVariables-root.

Note: One caller of GetVariables() just wants the size. It's possibly a
      little overkill to create entire HashMap just to count something,
      but I don't want to resolve that here in the same patch.

R=futhark@chromium.org, haraken@chromium.org

Bug: 850072
Change-Id: I64bfb3cc22a377cf956420a9e64d47ca3daac1e2
Reviewed-on: https://chromium-review.googlesource.com/1090848Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565605}
parent db4d3abf
Hide whitespace changes
Inline Side-by-side
Showing with 54 additions and 36 deletions
third_party/WebKit/LayoutTests/external/wpt/css/cssom/computed-style-set-property.html
View file @ a634522b
<!DOCTYPE> <!DOCTYPE html>
<title>NoModificationAllowedError when mutating read only properties</title> <title>NoModificationAllowedError when mutating read only properties</title>
<link rel="author" title="Anders Hartvoll Ruud" href="andruud@chromium.org"> <link rel="author" title="Anders Hartvoll Ruud" href="andruud@chromium.org">
<link rel="help" href="https://www.w3.org/TR/cssom-1/#dom-cssstyledeclaration-setpropertyvalue"> <link rel="help" href="https://www.w3.org/TR/cssom-1/#dom-cssstyledeclaration-setpropertyvalue">
......
third_party/WebKit/LayoutTests/typedcssom/computedstyle/custom-properties-expected.txt
View file @ a634522b
...@@ -9,5 +9,6 @@ FAIL Getting a height with a calc type containing var values returns a CSSCalcVa ...@@ -9,5 +9,6 @@ FAIL Getting a height with a calc type containing var values returns a CSSCalcVa
PASS Getting the value of a registered property that isn't set on the element returns the initial value for the property PASS Getting the value of a registered property that isn't set on the element returns the initial value for the property
PASS Getting the value of a property that isn't registered returns null PASS Getting the value of a property that isn't registered returns null
PASS getAll for a length type registered property returns a single value PASS getAll for a length type registered property returns a single value
PASS Inherited custom property produces correct type during iteration
Harness: the test ran to completion. Harness: the test ran to completion.
third_party/WebKit/LayoutTests/typedcssom/computedstyle/custom-properties.html
View file @ a634522b
...@@ -140,6 +140,15 @@ test(function() { ...@@ -140,6 +140,15 @@ test(function() {
assert_equals(result[0].toString(), '100px'); assert_equals(result[0].toString(), '100px');
}, 'getAll for a length type registered property returns a single value'); }, 'getAll for a length type registered property returns a single value');
test(function() {
var result = Array.from(computedStyleMap)
.filter(m => m[0] == '--my-inherited-length')
.flat(2);
assert_equals(result.length, 2);
assert_equals(result[1].constructor.name, CSSUnitValue.name);
assert_equals(result[1].toString(), '200px');
}, 'Inherited custom property produces correct type during iteration');
document.onreadystatechange = function() { document.onreadystatechange = function() {
if(document.readyState == 'complete') { if(document.readyState == 'complete') {
t1Callback(); t1Callback();
......
third_party/blink/renderer/core/css/computed_style_css_value_mapping.cc
View file @ a634522b
...@@ -57,13 +57,21 @@ const CSSValue* ComputedStyleCSSValueMapping::Get( ...@@ -57,13 +57,21 @@ const CSSValue* ComputedStyleCSSValueMapping::Get(
return CSSCustomPropertyDeclaration::Create(custom_property_name, data); return CSSCustomPropertyDeclaration::Create(custom_property_name, data);
} }
HashMap<AtomicString, scoped_refptr<CSSVariableData>> HeapHashMap<AtomicString, Member<const CSSValue>>
ComputedStyleCSSValueMapping::GetVariables(const ComputedStyle& style) { ComputedStyleCSSValueMapping::GetVariables(const ComputedStyle& style,
const PropertyRegistry* registry) {
HeapHashMap<AtomicString, Member<const CSSValue>> variables;
// TODO(timloh): Also return non-inherited variables // TODO(timloh): Also return non-inherited variables
StyleInheritedVariables* variables = style.InheritedVariables(); StyleInheritedVariables* inherited = style.InheritedVariables();
if (variables) if (inherited) {
return variables->GetVariables(); for (const auto& name : inherited->GetCustomPropertyNames()) {
return {}; const CSSValue* value =
ComputedStyleCSSValueMapping::Get(name, style, registry);
if (value)
variables.Set(name, value);
}
}
return variables;
} }
} // namespace blink } // namespace blink
third_party/blink/renderer/core/css/computed_style_css_value_mapping.h
View file @ a634522b
...@@ -10,7 +10,6 @@ ...@@ -10,7 +10,6 @@
namespace blink { namespace blink {
class CSSVariableData;
class ComputedStyle; class ComputedStyle;
class PropertyRegistry; class PropertyRegistry;
...@@ -21,8 +20,9 @@ class ComputedStyleCSSValueMapping { ...@@ -21,8 +20,9 @@ class ComputedStyleCSSValueMapping {
static const CSSValue* Get(const AtomicString custom_property_name, static const CSSValue* Get(const AtomicString custom_property_name,
const ComputedStyle&, const ComputedStyle&,
const PropertyRegistry*); const PropertyRegistry*);
static HashMap<AtomicString, scoped_refptr<CSSVariableData>> GetVariables( static HeapHashMap<AtomicString, Member<const CSSValue>> GetVariables(
const ComputedStyle&); const ComputedStyle& style,
const PropertyRegistry*);
}; };
} // namespace blink } // namespace blink
......
third_party/blink/renderer/core/css/css_computed_style_declaration.cc
View file @ a634522b
...@@ -316,12 +316,14 @@ const CSSValue* CSSComputedStyleDeclaration::GetPropertyCSSValue( ...@@ -316,12 +316,14 @@ const CSSValue* CSSComputedStyleDeclaration::GetPropertyCSSValue(
StyledNode()->GetDocument().GetPropertyRegistry()); StyledNode()->GetDocument().GetPropertyRegistry());
} }
HashMap<AtomicString, scoped_refptr<CSSVariableData>> HeapHashMap<AtomicString, Member<const CSSValue>>
CSSComputedStyleDeclaration::GetVariables() const { CSSComputedStyleDeclaration::GetVariables() const {
const ComputedStyle* style = ComputeComputedStyle(); const ComputedStyle* style = ComputeComputedStyle();
if (!style) if (!style)
return {}; return {};
return ComputedStyleCSSValueMapping::GetVariables(*style); DCHECK(StyledNode());
return ComputedStyleCSSValueMapping::GetVariables(
*style, StyledNode()->GetDocument().GetPropertyRegistry());
} }
const CSSValue* CSSComputedStyleDeclaration::GetPropertyCSSValue( const CSSValue* CSSComputedStyleDeclaration::GetPropertyCSSValue(
......
third_party/blink/renderer/core/css/css_computed_style_declaration.h
View file @ a634522b
...@@ -33,7 +33,6 @@ ...@@ -33,7 +33,6 @@
namespace blink { namespace blink {
class CSSVariableData;
class ExceptionState; class ExceptionState;
class LayoutObject; class LayoutObject;
class MutableCSSPropertyValueSet; class MutableCSSPropertyValueSet;
...@@ -61,7 +60,7 @@ class CORE_EXPORT CSSComputedStyleDeclaration final ...@@ -61,7 +60,7 @@ class CORE_EXPORT CSSComputedStyleDeclaration final
const CSSValue* GetPropertyCSSValue(const CSSProperty&) const; const CSSValue* GetPropertyCSSValue(const CSSProperty&) const;
const CSSValue* GetPropertyCSSValue(AtomicString custom_property_name) const; const CSSValue* GetPropertyCSSValue(AtomicString custom_property_name) const;
HashMap<AtomicString, scoped_refptr<CSSVariableData>> GetVariables() const; HeapHashMap<AtomicString, Member<const CSSValue>> GetVariables() const;
const CSSValue* GetFontSizeCSSValuePreferringKeyword() const; const CSSValue* GetFontSizeCSSValuePreferringKeyword() const;
bool IsMonospaceFont() const; bool IsMonospaceFont() const;
......
third_party/blink/renderer/core/css/cssom/computed_style_property_map.cc
View file @ a634522b
...@@ -167,8 +167,11 @@ unsigned int ComputedStylePropertyMap::size() { ...@@ -167,8 +167,11 @@ unsigned int ComputedStylePropertyMap::size() {
if (!style) if (!style)
return 0; return 0;
DCHECK(StyledNode());
return CSSComputedStyleDeclaration::ComputableProperties().size() + return CSSComputedStyleDeclaration::ComputableProperties().size() +
ComputedStyleCSSValueMapping::GetVariables(*style).size(); ComputedStyleCSSValueMapping::GetVariables(
*style, StyledNode()->GetDocument().GetPropertyRegistry())
.size();
} }
bool ComputedStylePropertyMap::ComparePropertyNames(const String& a, bool ComputedStylePropertyMap::ComparePropertyNames(const String& a,
...@@ -263,13 +266,12 @@ void ComputedStylePropertyMap::ForEachProperty( ...@@ -263,13 +266,12 @@ void ComputedStylePropertyMap::ForEachProperty(
values.emplace_back(property->GetPropertyNameAtomicString(), value); values.emplace_back(property->GetPropertyNameAtomicString(), value);
} }
PropertyRegistry* registry =
StyledNode()->GetDocument().GetPropertyRegistry();
for (const auto& name_value : for (const auto& name_value :
ComputedStyleCSSValueMapping::GetVariables(*style)) { ComputedStyleCSSValueMapping::GetVariables(*style, registry)) {
if (name_value.value) { values.emplace_back(name_value.key, name_value.value);
values.emplace_back(name_value.key,
CSSCustomPropertyDeclaration::Create(
name_value.key, name_value.value));
}
} }
std::sort(values.begin(), values.end(), [](const auto& a, const auto& b) { std::sort(values.begin(), values.end(), [](const auto& a, const auto& b) {
......
third_party/blink/renderer/core/inspector/inspector_css_agent.cc
View file @ a634522b
...@@ -1157,11 +1157,9 @@ Response InspectorCSSAgent::getComputedStyleForNode( ...@@ -1157,11 +1157,9 @@ Response InspectorCSSAgent::getComputedStyleForNode(
} }
for (const auto& it : computed_style_info->GetVariables()) { for (const auto& it : computed_style_info->GetVariables()) {
if (!it.value)
continue;
(*style)->addItem(protocol::CSS::CSSComputedStyleProperty::create() (*style)->addItem(protocol::CSS::CSSComputedStyleProperty::create()
.setName(it.key) .setName(it.key)
.setValue(it.value->TokenRange().Serialize()) .setValue(it.value->CssText())
.build()); .build());
} }
return Response::OK(); return Response::OK();
......
third_party/blink/renderer/core/style/style_inherited_variables.cc
View file @ a634522b
...@@ -74,16 +74,15 @@ void StyleInheritedVariables::RemoveVariable(const AtomicString& name) { ...@@ -74,16 +74,15 @@ void StyleInheritedVariables::RemoveVariable(const AtomicString& name) {
iterator->value = nullptr; iterator->value = nullptr;
} }
HashMap<AtomicString, scoped_refptr<CSSVariableData>> HashSet<AtomicString> StyleInheritedVariables::GetCustomPropertyNames() const {
StyleInheritedVariables::GetVariables() const { HashSet<AtomicString> names;
if (root_) { if (root_) {
HashMap<AtomicString, scoped_refptr<CSSVariableData>> result(root_->data_); for (const auto& pair : root_->data_)
for (const auto& pair : data_) names.insert(pair.key);
result.Set(pair.key, pair.value);
return result;
} else {
return data_;
} }
for (const auto& pair : data_)
names.insert(pair.key);
return names;
} }
} // namespace blink } // namespace blink
third_party/blink/renderer/core/style/style_inherited_variables.h
View file @ a634522b
...@@ -39,10 +39,10 @@ class StyleInheritedVariables : public RefCounted<StyleInheritedVariables> { ...@@ -39,10 +39,10 @@ class StyleInheritedVariables : public RefCounted<StyleInheritedVariables> {
void SetRegisteredVariable(const AtomicString&, const CSSValue*); void SetRegisteredVariable(const AtomicString&, const CSSValue*);
const CSSValue* RegisteredVariable(const AtomicString&) const; const CSSValue* RegisteredVariable(const AtomicString&) const;
// This map will contain null pointers if variables are invalid due to // Note that not all custom property names returned here necessarily have
// cycles or referencing invalid variables without using a fallback. // valid values, due to cycles or references to invalid variables without
// Note that this method is slow as a new map is constructed. // using a fallback.
HashMap<AtomicString, scoped_refptr<CSSVariableData>> GetVariables() const; HashSet<AtomicString> GetCustomPropertyNames() const;
private: private:
StyleInheritedVariables() : root_(nullptr) {} StyleInheritedVariables() : root_(nullptr) {}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment