Bug 1857724 - [css-properties-values-api] Invalid @property declarations should be dropped. r=emilio

Currently Firefox properly performs validation of an @property rule, as
defined in [1]. However when such a rule is invalid, it only does not
register the custom property instead of dropping the whole rule. Other
implementations also follow that aproach and existing web platform tests
disagree with the specification [2].

This patch aligns Firefox's behavior with the specification, by moving
@property validation during parsing and dropping invalid rules. Tests
are updated as follows:

1. /css/css-properties-values-api/at-property-cssom.html

  Existing tests that don't have the three descriptors (syntax,
  inherit, initial-value) are invalid and now the test verifies
  no corresponding rules are exposed via CSSOM. `--no-initial-value`
  is renamed `--no-initial-color-value` and its legacy tests are
  kept for a new @property `--no-initial-universal-value` which uses
  the universal syntax (so initial value is optional). Some dummy
  descriptors are added for --tab\ttab so that it remains valid.
  Similarly, we ensure --valid-whitespace's syntax (space-separated)
  and initial-value (comma-separated) agree.

2. /css/css-properties-values-api/at-property.html

  Existing `test_descriptor()` tests are trying an @property with
  a single specified descriptor and so are always invalid. To work
  around that, we tweak `test_descriptor()` so that it can build a
  valid descriptor instead. The `syntax` and `inherits` fallback
  to universal and true respectively while the `initial-value`
  descriptor is built from the `syntax`. An extra parameters is
  introduced in case the caller wants to provide these values
  directly. Finally, when the expected value is null the function
  instead verifies that the rule is really dropped.

  2.1. Some existing syntax tests are registering rules with unquoted
  syntax value 'red', 'rgb(255, 0, 0)', 'color', 'foo | bar' and
  expect to obtain a rule with an empty syntax string, suggesting some
  kind of invalidity handling (cf similar tests). We interpret the
  first two as "specifying a color value", quotes are added and the
  first one actually becomes a valid custom-ident. The last two already
  have a similar quoted version, so we just interpret them as
  "missing quotes".

  2.2. Given the previous 'red' custom-ident, we add tests for invalid
  custom-ident as defined in [3].

  2.3. Some existing `syntax` tests are checking that we must have
  "pipe between components" and no "leading bar" and are again expecting
  a rule with an empty syntax string. We fix the apparent mistake of
  missing quotes and provide initial values that could potentially be
  interpreted as correct by implementations accepting these invalid
  syntaxes.

  2.4. One `initial-value` test is checking "var(--x)" but that is
  not computationally independent so tweak the test to check that
  makes the @property rule invalid. Also add a similar '3em' test
  mentioned in the spec.

  2.5. Some `inherits` tests verify that invalid rules are interpreted
  as false. It seems they should instead be treated as if it does not
  exist and so should make the @property rule invalid.

[1] https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule
[2] https://github.com/w3c/css-houdini-drafts/issues/1098
[3] https://drafts.csswg.org/css-values-4/#custom-idents

Differential Revision: https://phabricator.services.mozilla.com/D190444
This commit is contained in:
Frederic Wang 2023-10-12 05:18:00 +00:00
Родитель 72ec7a68f3
Коммит b89f794f6b
6 изменённых файлов: 183 добавлений и 113 удалений

Просмотреть файл

@ -7,7 +7,6 @@
//! https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule
use super::{
registry::PropertyRegistration,
syntax::{Descriptor, ParsedDescriptor},
value::{AllowComputationallyDependent, SpecifiedValue as SpecifiedRegisteredValue},
};
@ -19,8 +18,8 @@ use crate::str::CssStringWriter;
use crate::stylesheets::UrlExtraData;
use crate::values::serialize_atom_name;
use cssparser::{
AtRuleParser, CowRcStr, DeclarationParser, ParseErrorKind, Parser, ParserInput,
QualifiedRuleParser, RuleBodyItemParser, RuleBodyParser, SourceLocation,
AtRuleParser, BasicParseErrorKind, CowRcStr, DeclarationParser, ParseErrorKind, Parser,
ParserInput, QualifiedRuleParser, RuleBodyItemParser, RuleBodyParser, SourceLocation,
};
use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
use selectors::parser::SelectorParseErrorKind;
@ -33,12 +32,12 @@ use to_shmem::{SharedMemoryBuilder, ToShmem};
///
/// Valid `@property` rules result in a registered custom property, as if `registerProperty()` had
/// been called with equivalent parameters.
pub fn parse_property_block(
pub fn parse_property_block<'i, 't>(
context: &ParserContext,
input: &mut Parser,
input: &mut Parser<'i, 't>,
name: PropertyRuleName,
location: SourceLocation,
) -> PropertyRuleData {
) -> Result<PropertyRuleData, ParseError<'i>> {
let mut rule = PropertyRuleData::empty(name, location);
let mut parser = PropertyRuleParser {
context,
@ -55,14 +54,22 @@ pub fn parse_property_block(
error.kind,
ParseErrorKind::Custom(StyleParseErrorKind::PropertySyntaxField(_))
) {
// If the provided string is not a valid syntax string (if it
// returns failure when consume a syntax definition is called on
// it), the descriptor is invalid and must be ignored.
ContextualParseError::UnsupportedValue(slice, error)
} else {
// Unknown descriptors are invalid and ignored, but do not
// invalidate the @property rule.
ContextualParseError::UnsupportedPropertyDescriptor(slice, error)
};
context.log_css_error(location, error);
}
}
rule
if rule.validate_registration(context.url_data).is_err() {
return Err(input.new_error(BasicParseErrorKind::AtRuleBodyInvalid));
}
Ok(rule)
}
struct PropertyRuleParser<'a, 'b: 'a> {
@ -179,9 +186,9 @@ property_descriptors! {
"initial-value" initial_value: InitialValue,
}
/// Errors that can happen when turning a property rule into a PropertyRegistration.
/// Errors that can happen when registering a property.
#[allow(missing_docs)]
pub enum ToRegistrationError {
pub enum PropertyRegistrationError {
MissingSyntax,
MissingInherits,
NoInitialValue,
@ -209,7 +216,7 @@ impl PropertyRuleData {
syntax: &Descriptor,
initial_value: Option<&InitialValue>,
url_data: &UrlExtraData,
) -> Result<(), ToRegistrationError> {
) -> Result<(), PropertyRegistrationError> {
use crate::properties::CSSWideKeyword;
// If the value of the syntax descriptor is the universal syntax definition, then the
// initial-value descriptor is optional. If omitted, the initial value of the property is
@ -222,12 +229,14 @@ impl PropertyRuleData {
// the following conditions must be met for the @property rule to be valid:
// The initial-value descriptor must be present.
let Some(initial) = initial_value else { return Err(ToRegistrationError::NoInitialValue) };
let Some(initial) = initial_value else {
return Err(PropertyRegistrationError::NoInitialValue)
};
// A value that references the environment or other variables is not computationally
// independent.
if initial.has_references() {
return Err(ToRegistrationError::InitialValueNotComputationallyIndependent);
return Err(PropertyRegistrationError::InitialValueNotComputationallyIndependent);
}
let mut input = ParserInput::new(initial.css_text());
@ -236,7 +245,7 @@ impl PropertyRuleData {
// The initial-value cannot include CSS-wide keywords.
if input.try_parse(CSSWideKeyword::parse).is_ok() {
return Err(ToRegistrationError::InitialValueNotComputationallyIndependent);
return Err(PropertyRegistrationError::InitialValueNotComputationallyIndependent);
}
match SpecifiedRegisteredValue::parse(
@ -246,23 +255,18 @@ impl PropertyRuleData {
AllowComputationallyDependent::No,
) {
Ok(_) => {},
Err(_) => return Err(ToRegistrationError::InvalidInitialValue),
Err(_) => return Err(PropertyRegistrationError::InvalidInitialValue),
}
Ok(())
}
/// Performs relevant rule validity checks.
///
/// If these don't pass, we shouldn't end up with a property registration.
///
/// NOTE(emilio): Currently per spec these should happen at parse-time, but I think that's just
/// a spec bug, see https://github.com/w3c/css-houdini-drafts/issues/1098
pub fn to_valid_registration(
fn validate_registration(
&self,
url_data: &UrlExtraData,
) -> Result<PropertyRegistration, ToRegistrationError> {
use self::ToRegistrationError::*;
) -> Result<(), PropertyRegistrationError> {
use self::PropertyRegistrationError::*;
// https://drafts.css-houdini.org/css-properties-values-api-1/#the-syntax-descriptor:
//
@ -274,16 +278,9 @@ impl PropertyRuleData {
//
// The inherits descriptor is required for the @property rule to be valid; if its
// missing, the @property rule is invalid.
let Some(ref inherits) = self.inherits else { return Err(MissingInherits) };
if self.inherits.is_none() { return Err(MissingInherits) };
Self::validate_initial_value(syntax.descriptor(), self.initial_value.as_ref(), url_data)?;
Ok(PropertyRegistration {
syntax: syntax.descriptor().clone(),
inherits: *inherits == Inherits::True,
initial_value: self.initial_value.clone(),
url_data: url_data.clone(),
})
Self::validate_initial_value(syntax.descriptor(), self.initial_value.as_ref(), url_data)
}
}

Просмотреть файл

@ -774,13 +774,13 @@ impl<'a, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'i> {
})))
},
AtRulePrelude::Property(name) => self.nest_for_rule(CssRuleType::Property, |p| {
CssRule::Property(Arc::new(parse_property_block(
let rule_data = parse_property_block(
&p.context,
input,
name,
start.source_location(),
)))
}),
start.source_location())?;
Ok::<CssRule, ParseError<'i>>(CssRule::Property(Arc::new(rule_data)))
})?,
AtRulePrelude::Document(condition) => {
if !cfg!(feature = "gecko") {
unreachable!()

Просмотреть файл

@ -23,6 +23,7 @@ use crate::media_queries::Device;
use crate::properties::{self, CascadeMode, ComputedValues, FirstLineReparenting};
use crate::properties::{AnimationDeclarations, PropertyDeclarationBlock};
use crate::properties_and_values::registry::{ScriptRegistry as CustomPropertyScriptRegistry, PropertyRegistration};
use crate::properties_and_values::rule::Inherits;
use crate::rule_cache::{RuleCache, RuleCacheConditions};
use crate::rule_collector::RuleCollector;
use crate::rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
@ -2946,13 +2947,18 @@ impl CascadeData {
},
CssRule::Property(ref rule) => {
let url_data = stylesheet.contents().url_data.read();
if let Ok(registration) = rule.to_valid_registration(&url_data) {
self.custom_property_registrations.try_insert(
rule.name.0.clone(),
registration,
containing_rule_state.layer_id,
)?;
}
// FIXME(emilio, bug 1858160): Simplify storage.
let registration = PropertyRegistration {
syntax: rule.syntax.as_ref().unwrap().descriptor().clone(),
inherits: rule.inherits == Some(Inherits::True),
initial_value: rule.initial_value.clone(),
url_data: url_data.clone(),
};
self.custom_property_registrations.try_insert(
rule.name.0.clone(),
registration,
containing_rule_state.layer_id,
)?;
},
#[cfg(feature = "gecko")]
CssRule::FontFace(ref rule) => {

Просмотреть файл

@ -8550,7 +8550,7 @@ pub extern "C" fn Servo_RegisterCustomProperty(
) -> RegisterCustomPropertyResult {
use self::RegisterCustomPropertyResult::*;
use style::custom_properties::SpecifiedValue;
use style::properties_and_values::rule::{PropertyRuleData, ToRegistrationError};
use style::properties_and_values::rule::{PropertyRuleData, PropertyRegistrationError};
use style::properties_and_values::syntax::Descriptor;
let mut per_doc_data = per_doc_data.borrow_mut();
@ -8593,11 +8593,11 @@ pub extern "C" fn Servo_RegisterCustomProperty(
PropertyRuleData::validate_initial_value(&syntax, initial_value.as_ref(), url_data)
{
return match error {
ToRegistrationError::MissingInherits |
ToRegistrationError::MissingSyntax => unreachable!(),
ToRegistrationError::InitialValueNotComputationallyIndependent => InitialValueNotComputationallyIndependent,
ToRegistrationError::InvalidInitialValue => InvalidInitialValue,
ToRegistrationError::NoInitialValue=> NoInitialValue,
PropertyRegistrationError::MissingInherits |
PropertyRegistrationError::MissingSyntax => unreachable!(),
PropertyRegistrationError::InitialValueNotComputationallyIndependent => InitialValueNotComputationallyIndependent,
PropertyRegistrationError::InvalidInitialValue => InvalidInitialValue,
PropertyRegistrationError::NoInitialValue=> NoInitialValue,
}
}

Просмотреть файл

@ -18,7 +18,7 @@
inherits: false;
}
@property --valid-whitespace {
syntax: " <color>+ ";
syntax: " <color># ";
inherits: false;
initial-value: red, blue;
}
@ -38,10 +38,14 @@
syntax: "<color> | none";
initial-value: red;
}
@property --no-initial-value {
@property --no-initial-color-value {
syntax: "<color> | none";
inherits: false;
}
@property --no-initial-universal-value {
syntax: "*";
inherits: false;
}
@property --syntax-only {
syntax: "<color> | none";
}
@ -52,7 +56,10 @@
initial-value: red;
}
/* U+0009 CHARACTER TABULATION */
@property --tab\9 tab { }
@property --tab\9 tab {
syntax: "*";
inherits: true;
}
</style>
<script>
@ -66,6 +73,13 @@ function find_at_property_rule(name) {
return null;
}
function test_invalid(name) {
test(() => {
let rule = find_at_property_rule(name);
assert_true(!rule);
}, `Rule for ${name} is invalid`);
}
function test_css_text(name, expected) {
test(() => {
let rule = find_at_property_rule(name);
@ -106,22 +120,26 @@ function test_initial_value(name, expected) {
}, `Rule for ${name} returns expected value for CSSPropertyRule.initialValue`);
}
// Invalid @property rules.
test_invalid('--no-descriptors');
test_invalid('--no-syntax');
test_invalid('--no-inherits');
test_invalid('--no-initial-color-value');
test_invalid('--syntax-only', '@property --syntax-only { syntax: "<color> | none"; }');
test_invalid('--inherits-only', '@property --inherits-only { inherits: true; }');
test_invalid('--initial-value-only', '@property --initial-value-only { initial-value: red; }');
// CSSPropertyRule.cssText
test_css_text('--valid', '@property --valid { syntax: "<color> | none"; inherits: false; initial-value: red; }');
test_css_text('--valid-reverse', '@property --valid-reverse { syntax: "<length>"; inherits: true; initial-value: 0px; }');
test_css_text('--valid-universal', '@property --valid-universal { syntax: "*"; inherits: false; }');
test_css_text('--valid-whitespace', '@property --valid-whitespace { syntax: " <color>+ "; inherits: false; initial-value: red, blue; }');
test_css_text('--valid-whitespace', '@property --valid-whitespace { syntax: " <color># "; inherits: false; initial-value: red, blue; }');
test_css_text('--vALId', '@property --vALId { syntax: "<color> | none"; inherits: false; initial-value: red; }');
test_css_text('--no-descriptors', '@property --no-descriptors { }');
test_css_text('--no-syntax', '@property --no-syntax { inherits: false; initial-value: red; }');
test_css_text('--no-inherits', '@property --no-inherits { syntax: "<color> | none"; initial-value: red; }');
test_css_text('--no-initial-value', '@property --no-initial-value { syntax: "<color> | none"; inherits: false; }');
test_css_text('--syntax-only', '@property --syntax-only { syntax: "<color> | none"; }');
test_css_text('--inherits-only', '@property --inherits-only { inherits: true; }');
test_css_text('--initial-value-only', '@property --initial-value-only { initial-value: red; }');
test_css_text('--tab\ttab', '@property --tab\\9 tab { }');
test_css_text('--no-initial-universal-value', '@property --no-initial-universal-value { syntax: "*"; inherits: false; }');
test_css_text('--tab\ttab', '@property --tab\\9 tab { syntax: "*"; inherits: true; }');
// CSSRule.type
@ -138,29 +156,17 @@ test_name('--valid-universal');
test_name('--valid-whitespace');
test_name('--vALId');
test_name('--no-descriptors');
test_name('--no-syntax');
test_name('--no-inherits');
test_name('--no-initial-value');
test_name('--syntax-only');
test_name('--inherits-only');
test_name('--initial-value-only');
test_name('--no-initial-universal-value');
// CSSPropertyRule.syntax
test_syntax('--valid', '<color> | none');
test_syntax('--valid-reverse', '<length>');
test_syntax('--valid-universal', '*');
test_syntax('--valid-whitespace', ' <color>+ ');
test_syntax('--valid-whitespace', ' <color># ');
test_syntax('--vALId', '<color> | none');
test_syntax('--no-descriptors', '');
test_syntax('--no-syntax', '');
test_syntax('--no-inherits', '<color> | none');
test_syntax('--no-initial-value', '<color> | none');
test_syntax('--syntax-only', '<color> | none');
test_syntax('--inherits-only', '');
test_syntax('--initial-value-only', '');
test_syntax('--no-initial-universal-value', '*');
// CSSPropertyRule.inherits
@ -170,13 +176,7 @@ test_inherits('--valid-universal', false);
test_inherits('--valid-whitespace', false);
test_inherits('--vALId', false);
test_inherits('--no-descriptors', false);
test_inherits('--no-syntax', false);
test_inherits('--no-inherits', false);
test_inherits('--no-initial-value', false);
test_inherits('--syntax-only', false);
test_inherits('--inherits-only', true);
test_inherits('--initial-value-only', false);
test_inherits('--no-initial-universal-value', false);
// CSSPropertyRule.initialValue
@ -186,12 +186,6 @@ test_initial_value('--valid-universal', null);
test_initial_value('--valid-whitespace', 'red, blue');
test_initial_value('--vALId', 'red');
test_initial_value('--no-descriptors', null);
test_initial_value('--no-syntax', 'red');
test_initial_value('--no-inherits', 'red');
test_initial_value('--no-initial-value', null);
test_initial_value('--syntax-only', null);
test_initial_value('--inherits-only', null);
test_initial_value('--initial-value-only', 'red');
test_initial_value('--no-initial-universal-value', null);
</script>

Просмотреть файл

@ -30,13 +30,66 @@ function get_cssom_descriptor_value(rule, descriptor) {
// Test that for the given descriptor (e.g. 'syntax'), the specified value
// will yield the expected_value when observed using CSSOM. If the expected_value
// is omitted, it is the same as the specified value.
function test_descriptor(descriptor, specified_value, expected_value) {
let camel = to_camel_case(descriptor);
if (typeof(expected_value) === 'undefined')
expected_value = specified_value;
test_with_at_property({ [camel]: specified_value }, (name, rule) => {
assert_equals(get_cssom_descriptor_value(rule, descriptor), expected_value);
}, `Attribute '${descriptor}' returns expected value for [${specified_value}]`);
function test_descriptor(descriptor, specified_value, expected_value, other_descriptors) {
// Try and build a valid @property form the specified descriptor.
let at_property = { [to_camel_case(descriptor)]: specified_value };
// If extra values are specified in other_descriptors, just use them.
if (typeof(other_descriptors) !== 'unspecified') {
for (let name in other_descriptors) {
if (other_descriptors.hasOwnProperty(name)) {
if (name == descriptor) {
throw `Unexpected ${name} in other_descriptors`;
}
at_property[to_camel_case(name)] = other_descriptors[name];
}
}
}
if (!('syntax' in at_property)) {
// The syntax descriptor is required. Use the universal one as a fallback.
// https://drafts.css-houdini.org/css-properties-values-api-1/#the-syntax-descriptor
at_property.syntax = '"*"';
}
if (!('inherits' in at_property)) {
// The inherits descriptor is required. Make it true as a fallback.
// https://drafts.css-houdini.org/css-properties-values-api-1/#inherits-descriptor
at_property.inherits = true;
}
if (!at_property.syntax.match(/^"\s*\*\s*"$/) &&
!('initialValue' in at_property)) {
// The initial-value is required for non-universal syntax.
// Pick a computationally independent value that follows specified syntax.
// https://drafts.css-houdini.org/css-properties-values-api-1/#the-syntax-descriptor
at_property.initialValue = (() => {
let first_syntax_component = specified_value
.replace(/^"(.*)"$/, '$1') // unquote
.replace(/[\s\uFEFF\xA0]+/g, ' ') // collapse whitespaces
.match(/^[^|\#\+]*/)[0] // pick first component
.trim();
switch (first_syntax_component) {
case '<color>': return 'blue';
case '<length>': return '42px';
default:
if (first_syntax_component.startsWith('<')) {
throw `Unsupported data type name '${first_syntax_component}'`;
}
return first_syntax_component; // <custom-ident>
}
})();
}
if (expected_value === null) {
test_with_at_property(at_property, (name, rule) => {
assert_true(!rule);
}, `Attribute '${descriptor}' makes the @property rule invalid for [${specified_value}]`);
} else {
if (typeof(expected_value) === 'undefined')
expected_value = specified_value;
test_with_at_property(at_property, (name, rule) => {
assert_equals(get_cssom_descriptor_value(rule, descriptor), expected_value);
}, `Attribute '${descriptor}' returns expected value for [${specified_value}]`);
}
}
// syntax
@ -52,19 +105,36 @@ for (const syntax of ["*", " * ", "* ", "\t*\t"]) {
test_descriptor('syntax', `"${syntax}"`, syntax);
}
test_descriptor('syntax', 'red', '');
test_descriptor('syntax', 'rgb(255, 0, 0)', '');
test_descriptor('syntax', '<color>', '');
test_descriptor('syntax', 'foo | bar', '');
// syntax: <color> value
test_descriptor('syntax', '"red"', "red"); // treated as <custom-ident>.
test_descriptor('syntax', '"rgb(255, 0, 0)"', null);
// syntax: missing quotes
test_descriptor('syntax', '<color>', null);
test_descriptor('syntax', 'foo | bar', null);
// syntax: invalid <custom-ident>
// https://drafts.csswg.org/css-values-4/#custom-idents
for (const syntax of
["default",
"initial",
"inherit",
"unset",
"revert",
"revert-layer",
]) {
test_descriptor('syntax', `"${syntax}"`, null);
test_descriptor('syntax', `"${uppercase_first(syntax)}"`, null);
}
// syntax: pipe between components
test_descriptor('syntax', 'foo bar', '');
test_descriptor('syntax', 'Foo <length>', '');
test_descriptor('syntax', 'foo, bar', '');
test_descriptor('syntax', '<length> <percentage>', '');
test_descriptor('syntax', '"foo bar"', null, {'initial-value': 'foo bar'});
test_descriptor('syntax', '"Foo <length>"', null, {'initial-value': 'Foo 42px'});
test_descriptor('syntax', '"foo, bar"', null, {'initial-value': 'foo, bar'});
test_descriptor('syntax', '"<length> <percentage>"', null, {'initial-value': '42px 100%'});
// syntax: leaading bar
test_descriptor('syntax', '|<length>', '');
// syntax: leading bar
test_descriptor('syntax', '"|<length>"', null, {'initial-value': '42px'});
// initial-value
test_descriptor('initial-value', '10px');
@ -72,18 +142,21 @@ test_descriptor('initial-value', 'rgb(1, 2, 3)');
test_descriptor('initial-value', 'red');
test_descriptor('initial-value', 'foo');
test_descriptor('initial-value', 'if(){}');
test_descriptor('initial-value', 'var(--x)');
// initial-value: not computationally independent
test_descriptor('initial-value', '3em', null, {'syntax': '"<length>"'});
test_descriptor('initial-value', 'var(--x)', null);
// inherits
test_descriptor('inherits', 'true', true);
test_descriptor('inherits', 'false', false);
test_descriptor('inherits', 'none', false);
test_descriptor('inherits', '0', false);
test_descriptor('inherits', '1', false);
test_descriptor('inherits', '"true"', false);
test_descriptor('inherits', '"false"', false);
test_descriptor('inherits', 'calc(0)', false);
test_descriptor('inherits', 'none', null);
test_descriptor('inherits', '0', null);
test_descriptor('inherits', '1', null);
test_descriptor('inherits', '"true"', null);
test_descriptor('inherits', '"false"', null);
test_descriptor('inherits', 'calc(0)', null);
test_with_style_node('@property foo { }', (node) => {
assert_equals(node.sheet.rules.length, 0);