From e45d08541b28203ca89fbb20f9cee5102fb75352 Mon Sep 17 00:00:00 2001 From: Mats Palmgren Date: Fri, 30 Aug 2019 00:15:37 +0000 Subject: [PATCH] Bug 1576821 - [css-lists-3] Make 'none' invalid as a in counter()/counters(). r=emilio CSSWG resolution: https://github.com/w3c/csswg-drafts/issues/4163#issuecomment-521331100 Spec: https://drafts.csswg.org/css-lists-3/#counter-functions Differential Revision: https://phabricator.services.mozilla.com/D43893 --HG-- extra : moz-landing-system : lando --- layout/reftests/counters/reftest.list | 3 +- .../counters/t1202-counter-01-b-test.html | 34 --------------- .../t1202-counters-01-b-reference.html | 29 ------------- layout/style/test/property_database.js | 4 +- servo/components/style/gecko/values.rs | 23 ++++------ .../components/style/properties/gecko.mako.rs | 27 +++++++++--- .../style/properties/shorthands/list.mako.rs | 16 ++----- .../style/values/generics/counters.rs | 6 +-- servo/components/style/values/generics/mod.rs | 26 +++++------ .../style/values/specified/counters.rs | 8 ++-- .../components/style/values/specified/list.rs | 20 +++++---- .../content-counter-001-ref.xht | 43 ------------------- .../generated-content/content-counter-001.xht | 19 +------- .../css-lists/parsing/content-invalid.html | 41 ++++++++++++++++++ 14 files changed, 108 insertions(+), 191 deletions(-) delete mode 100644 layout/reftests/counters/t1202-counter-01-b-test.html delete mode 100644 layout/reftests/counters/t1202-counters-01-b-reference.html delete mode 100644 testing/web-platform/tests/css/CSS2/generated-content/content-counter-001-ref.xht create mode 100644 testing/web-platform/tests/css/css-lists/parsing/content-invalid.html diff --git a/layout/reftests/counters/reftest.list b/layout/reftests/counters/reftest.list index 0b2dee8cd611..529243fa8c77 100644 --- a/layout/reftests/counters/reftest.list +++ b/layout/reftests/counters/reftest.list @@ -1,6 +1,5 @@ == counter-name-case-sensitive.html counter-name-case-sensitive-ref.html == t1202-counter-00-b-test.html t1202-counter-00-b-reference.html -== t1202-counter-01-b-test.html t1202-counter-01-b-reference.html == t1202-counter-02-b-test.html t1202-counter-02-b-reference.html == t1202-counter-03-b-test.html t1202-counter-03-b-reference.html == t1202-counter-04-b-test.html t1202-counter-04-b-reference.html @@ -17,7 +16,7 @@ random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == t1202-counter-10-b-test. == t1202-counter-15-b-test.html t1202-counter-15-b-reference.html == t1202-counter-16-f-test.html t1202-counter-16-f-reference.html == t1202-counters-00-b-test.html t1202-counters-00-b-reference.html -== t1202-counters-01-b-test.html t1202-counters-01-b-reference.html +== t1202-counters-01-b-test.html about:blank == t1202-counters-02-b-test.html t1202-counters-02-b-reference.html == t1202-counters-03-b-test.html t1202-counters-03-b-reference.html == t1202-counters-04-b-test.html t1202-counters-04-b-reference.html diff --git a/layout/reftests/counters/t1202-counter-01-b-test.html b/layout/reftests/counters/t1202-counter-01-b-test.html deleted file mode 100644 index 9ca7eff581ab..000000000000 --- a/layout/reftests/counters/t1202-counter-01-b-test.html +++ /dev/null @@ -1,34 +0,0 @@ - - - - CSS 2.1 Test Suite: content: counter(c, none) - - - - - - - -
- - - - - - - - - - - - -
- - - diff --git a/layout/reftests/counters/t1202-counters-01-b-reference.html b/layout/reftests/counters/t1202-counters-01-b-reference.html deleted file mode 100644 index e28d0b4bb6a0..000000000000 --- a/layout/reftests/counters/t1202-counters-01-b-reference.html +++ /dev/null @@ -1,29 +0,0 @@ - - - - CSS 2.1 Test Suite: content: counters(c, ".", none) - - - - - - -

- -
- .z - .z - .z - .z - .z - .z - .z - .z - .z - .z - .z - .z -
- - - diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js index 2e6e155d4c82..9a980d5cf547 100644 --- a/layout/style/test/property_database.js +++ b/layout/style/test/property_database.js @@ -5287,8 +5287,6 @@ var gCSSProperties = { "no-open-quote", "no-close-quote", "close-quote attr(title) counters(foo, '.', upper-alpha)", - "counter(foo, none)", - "counters(bar, '.', none)", "attr(\\32)", "attr(\\2)", "attr(-\\2)", @@ -5307,6 +5305,8 @@ var gCSSProperties = { "counters(foo, '.', symbols(numeric '0' '1'))", ], invalid_values: [ + "counter(foo, none)", + "counters(bar, '.', none)", "counters(foo)", 'counter(foo, ".")', 'attr("title")', diff --git a/servo/components/style/gecko/values.rs b/servo/components/style/gecko/values.rs index f0c998f8f081..b31485345b60 100644 --- a/servo/components/style/gecko/values.rs +++ b/servo/components/style/gecko/values.rs @@ -8,7 +8,7 @@ use crate::counter_style::{Symbol, Symbols}; use crate::gecko_bindings::structs::CounterStylePtr; -use crate::values::generics::CounterStyleOrNone; +use crate::values::generics::CounterStyle; use crate::values::Either; use crate::Atom; use app_units::Au; @@ -49,19 +49,17 @@ pub fn round_border_to_device_pixels(width: Au, au_per_device_px: Au) -> Au { } } -impl CounterStyleOrNone { +impl CounterStyle { /// Convert this counter style to a Gecko CounterStylePtr. pub fn to_gecko_value(self, gecko_value: &mut CounterStylePtr) { use crate::gecko_bindings::bindings::Gecko_SetCounterStyleToName as set_name; use crate::gecko_bindings::bindings::Gecko_SetCounterStyleToSymbols as set_symbols; match self { - CounterStyleOrNone::None => unsafe { - set_name(gecko_value, atom!("none").into_addrefed()); - }, - CounterStyleOrNone::Name(name) => unsafe { + CounterStyle::Name(name) => unsafe { + debug_assert_ne!(name.0, atom!("none")); set_name(gecko_value, name.0.into_addrefed()); }, - CounterStyleOrNone::Symbols(symbols_type, symbols) => { + CounterStyle::Symbols(symbols_type, symbols) => { let symbols: Vec<_> = symbols .0 .iter() @@ -86,7 +84,7 @@ impl CounterStyleOrNone { } } - /// Convert Gecko CounterStylePtr to CounterStyleOrNone or String. + /// Convert Gecko CounterStylePtr to CounterStyle or String. pub fn from_gecko_value(gecko_value: &CounterStylePtr) -> Either { use crate::gecko_bindings::bindings; use crate::values::generics::SymbolsType; @@ -95,11 +93,8 @@ impl CounterStyleOrNone { let name = unsafe { bindings::Gecko_CounterStyle_GetName(gecko_value) }; if !name.is_null() { let name = unsafe { Atom::from_raw(name) }; - if name == atom!("none") { - Either::First(CounterStyleOrNone::None) - } else { - Either::First(CounterStyleOrNone::Name(CustomIdent(name))) - } + debug_assert_ne!(name, atom!("none")); + Either::First(CounterStyle::Name(CustomIdent(name))) } else { let anonymous = unsafe { bindings::Gecko_CounterStyle_GetAnonymous(gecko_value).as_ref() }.unwrap(); @@ -113,7 +108,7 @@ impl CounterStyleOrNone { .iter() .map(|gecko_symbol| Symbol::String(gecko_symbol.to_string().into())) .collect(); - Either::First(CounterStyleOrNone::Symbols(symbol_type, Symbols(symbols))) + Either::First(CounterStyle::Symbols(symbol_type, Symbols(symbols))) } } } diff --git a/servo/components/style/properties/gecko.mako.rs b/servo/components/style/properties/gecko.mako.rs index f751e872f3ff..526304d87efd 100644 --- a/servo/components/style/properties/gecko.mako.rs +++ b/servo/components/style/properties/gecko.mako.rs @@ -2199,10 +2199,15 @@ fn static_assert() { } pub fn set_list_style_type(&mut self, v: longhands::list_style_type::computed_value::T) { + use crate::gecko_bindings::bindings::Gecko_SetCounterStyleToName; use crate::gecko_bindings::bindings::Gecko_SetCounterStyleToString; use nsstring::{nsACString, nsCStr}; use self::longhands::list_style_type::computed_value::T; match v { + T::None => unsafe { + Gecko_SetCounterStyleToName(&mut self.gecko.mCounterStyle, + atom!("none").into_addrefed()); + } T::CounterStyle(s) => s.to_gecko_value(&mut self.gecko.mCounterStyle), T::String(s) => unsafe { Gecko_SetCounterStyleToString(&mut self.gecko.mCounterStyle, @@ -2224,9 +2229,19 @@ fn static_assert() { pub fn clone_list_style_type(&self) -> longhands::list_style_type::computed_value::T { use self::longhands::list_style_type::computed_value::T; use crate::values::Either; - use crate::values::generics::CounterStyleOrNone; + use crate::values::generics::CounterStyle; + use crate::gecko_bindings::bindings; - let result = CounterStyleOrNone::from_gecko_value(&self.gecko.mCounterStyle); + let name = unsafe { + bindings::Gecko_CounterStyle_GetName(&self.gecko.mCounterStyle) + }; + if !name.is_null() { + let name = unsafe { Atom::from_raw(name) }; + if name == atom!("none") { + return T::None; + } + } + let result = CounterStyle::from_gecko_value(&self.gecko.mCounterStyle); match result { Either::First(counter_style) => T::CounterStyle(counter_style), Either::Second(string) => T::String(string), @@ -2573,7 +2588,7 @@ clip-path pub fn set_content(&mut self, v: longhands::content::computed_value::T) { use crate::values::CustomIdent; use crate::values::generics::counters::{Content, ContentItem}; - use crate::values::generics::CounterStyleOrNone; + use crate::values::generics::CounterStyle; use crate::gecko_bindings::structs::nsStyleContentData; use crate::gecko_bindings::structs::nsStyleContentAttr; use crate::gecko_bindings::structs::StyleContentType; @@ -2594,7 +2609,7 @@ clip-path content_type: StyleContentType, name: CustomIdent, sep: &str, - style: CounterStyleOrNone, + style: CounterStyle, ) { debug_assert!(content_type == StyleContentType::Counter || content_type == StyleContentType::Counters); @@ -2724,7 +2739,7 @@ clip-path use crate::gecko_bindings::structs::StyleContentType; use crate::values::generics::counters::{Content, ContentItem}; use crate::values::{CustomIdent, Either}; - use crate::values::generics::CounterStyleOrNone; + use crate::values::generics::CounterStyle; use crate::values::specified::Attr; if self.gecko.mContents.is_empty() { @@ -2769,7 +2784,7 @@ clip-path Atom::from_raw(gecko_function.mIdent.mRawPtr) }); let style = - CounterStyleOrNone::from_gecko_value(&gecko_function.mCounterStyle); + CounterStyle::from_gecko_value(&gecko_function.mCounterStyle); let style = match style { Either::First(counter_style) => counter_style, Either::Second(_) => diff --git a/servo/components/style/properties/shorthands/list.mako.rs b/servo/components/style/properties/shorthands/list.mako.rs index f2e8e28b19d6..8e842cd153e1 100644 --- a/servo/components/style/properties/shorthands/list.mako.rs +++ b/servo/components/style/properties/shorthands/list.mako.rs @@ -61,31 +61,23 @@ let position = unwrap_or_initial!(list_style_position, position); - fn list_style_type_none() -> list_style_type::SpecifiedValue { - % if engine == "gecko": - use crate::values::generics::CounterStyleOrNone; - list_style_type::SpecifiedValue::CounterStyle(CounterStyleOrNone::None) - % else: - list_style_type::SpecifiedValue::None - % endif - } - // If there are two `none`s, then we can't have a type or image; if there is one `none`, // then we can't have both a type *and* an image; if there is no `none` then we're fine as // long as we parsed something. + use self::list_style_type::SpecifiedValue as ListStyleType; match (any, nones, list_style_type, image) { (true, 2, None, None) => { Ok(expanded! { list_style_position: position, list_style_image: ImageUrlOrNone::none(), - list_style_type: list_style_type_none(), + list_style_type: ListStyleType::None, }) } (true, 1, None, Some(image)) => { Ok(expanded! { list_style_position: position, list_style_image: image, - list_style_type: list_style_type_none(), + list_style_type: ListStyleType::None, }) } (true, 1, Some(list_style_type), None) => { @@ -99,7 +91,7 @@ Ok(expanded! { list_style_position: position, list_style_image: ImageUrlOrNone::none(), - list_style_type: list_style_type_none(), + list_style_type: ListStyleType::None, }) } (true, 0, list_style_type, image) => { diff --git a/servo/components/style/values/generics/counters.rs b/servo/components/style/values/generics/counters.rs index fbb6927b9f1c..9f3135d867d5 100644 --- a/servo/components/style/values/generics/counters.rs +++ b/servo/components/style/values/generics/counters.rs @@ -7,7 +7,7 @@ #[cfg(feature = "servo")] use crate::computed_values::list_style_type::T as ListStyleType; #[cfg(feature = "gecko")] -use crate::values::generics::CounterStyleOrNone; +use crate::values::generics::CounterStyle; #[cfg(feature = "gecko")] use crate::values::specified::Attr; use crate::values::CustomIdent; @@ -127,7 +127,7 @@ impl Counters { type CounterStyleType = ListStyleType; #[cfg(feature = "gecko")] -type CounterStyleType = CounterStyleOrNone; +type CounterStyleType = CounterStyle; #[cfg(feature = "servo")] #[inline] @@ -138,7 +138,7 @@ fn is_decimal(counter_type: &CounterStyleType) -> bool { #[cfg(feature = "gecko")] #[inline] fn is_decimal(counter_type: &CounterStyleType) -> bool { - *counter_type == CounterStyleOrNone::decimal() + *counter_type == CounterStyle::decimal() } /// The specified value for the `content` property. diff --git a/servo/components/style/values/generics/mod.rs b/servo/components/style/values/generics/mod.rs index 3bc564dd3858..933ad38f7dba 100644 --- a/servo/components/style/values/generics/mod.rs +++ b/servo/components/style/values/generics/mod.rs @@ -92,13 +92,10 @@ impl SymbolsType { /// /// -/// Since wherever is used, 'none' is a valid value as -/// well, we combine them into one type to make code simpler. +/// Note that 'none' is not a valid name. #[cfg_attr(feature = "gecko", derive(MallocSizeOf))] #[derive(Clone, Debug, Eq, PartialEq, ToComputedValue, ToCss, ToResolvedValue, ToShmem)] -pub enum CounterStyleOrNone { - /// `none` - None, +pub enum CounterStyle { /// `` Name(CustomIdent), /// `symbols()` @@ -111,28 +108,25 @@ fn is_symbolic(symbols_type: &SymbolsType) -> bool { *symbols_type == SymbolsType::Symbolic } -impl CounterStyleOrNone { +impl CounterStyle { /// disc value pub fn disc() -> Self { - CounterStyleOrNone::Name(CustomIdent(atom!("disc"))) + CounterStyle::Name(CustomIdent(atom!("disc"))) } /// decimal value pub fn decimal() -> Self { - CounterStyleOrNone::Name(CustomIdent(atom!("decimal"))) + CounterStyle::Name(CustomIdent(atom!("decimal"))) } } -impl Parse for CounterStyleOrNone { +impl Parse for CounterStyle { fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { if let Ok(name) = input.try(|i| parse_counter_style_name(i)) { - return Ok(CounterStyleOrNone::Name(name)); - } - if input.try(|i| i.expect_ident_matching("none")).is_ok() { - return Ok(CounterStyleOrNone::None); + return Ok(CounterStyle::Name(name)); } input.expect_function_matching("symbols")?; input.parse_nested_block(|input| { @@ -151,12 +145,12 @@ impl Parse for CounterStyleOrNone { if symbols.0.iter().any(|sym| !sym.is_allowed_in_symbols()) { return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)); } - Ok(CounterStyleOrNone::Symbols(symbols_type, symbols)) + Ok(CounterStyle::Symbols(symbols_type, symbols)) }) } } -impl SpecifiedValueInfo for CounterStyleOrNone { +impl SpecifiedValueInfo for CounterStyle { fn collect_completion_keywords(f: KeywordsCollectFn) { // XXX The best approach for implementing this is probably // having a CounterStyleName type wrapping CustomIdent, and @@ -165,7 +159,7 @@ impl SpecifiedValueInfo for CounterStyleOrNone { // approach here. macro_rules! predefined { ($($name:expr,)+) => { - f(&["none", "symbols", $($name,)+]); + f(&["symbols", $($name,)+]); } } include!("../../counter_style/predefined.rs"); diff --git a/servo/components/style/values/specified/counters.rs b/servo/components/style/values/specified/counters.rs index 262e7765a4a0..98e9a27752ce 100644 --- a/servo/components/style/values/specified/counters.rs +++ b/servo/components/style/values/specified/counters.rs @@ -12,7 +12,7 @@ use crate::values::generics::counters::CounterIncrement as GenericCounterIncreme use crate::values::generics::counters::CounterPair; use crate::values::generics::counters::CounterSetOrReset as GenericCounterSetOrReset; #[cfg(feature = "gecko")] -use crate::values::generics::CounterStyleOrNone; +use crate::values::generics::CounterStyle; use crate::values::specified::url::SpecifiedImageUrl; #[cfg(feature = "gecko")] use crate::values::specified::Attr; @@ -98,13 +98,13 @@ impl Content { } #[cfg(feature = "gecko")] - fn parse_counter_style(context: &ParserContext, input: &mut Parser) -> CounterStyleOrNone { + fn parse_counter_style(context: &ParserContext, input: &mut Parser) -> CounterStyle { input .try(|input| { input.expect_comma()?; - CounterStyleOrNone::parse(context, input) + CounterStyle::parse(context, input) }) - .unwrap_or(CounterStyleOrNone::decimal()) + .unwrap_or(CounterStyle::decimal()) } } diff --git a/servo/components/style/values/specified/list.rs b/servo/components/style/values/specified/list.rs index 084889a3a223..1c93f8e8a589 100644 --- a/servo/components/style/values/specified/list.rs +++ b/servo/components/style/values/specified/list.rs @@ -6,7 +6,7 @@ use crate::parser::{Parse, ParserContext}; #[cfg(feature = "gecko")] -use crate::values::generics::CounterStyleOrNone; +use crate::values::generics::CounterStyle; #[cfg(feature = "gecko")] use crate::values::CustomIdent; use cssparser::{Parser, Token}; @@ -27,8 +27,10 @@ use style_traits::{ParseError, StyleParseErrorKind}; ToShmem, )] pub enum ListStyleType { - /// | none - CounterStyle(CounterStyleOrNone), + /// `none` + None, + /// + CounterStyle(CounterStyle), /// String(String), } @@ -38,7 +40,7 @@ impl ListStyleType { /// Initial specified value for `list-style-type`. #[inline] pub fn disc() -> Self { - ListStyleType::CounterStyle(CounterStyleOrNone::disc()) + ListStyleType::CounterStyle(CounterStyle::disc()) } /// Convert from gecko keyword to list-style-type. @@ -50,10 +52,10 @@ impl ListStyleType { use crate::gecko_bindings::structs; if value == structs::NS_STYLE_LIST_STYLE_NONE { - return ListStyleType::CounterStyle(CounterStyleOrNone::None); + return ListStyleType::None; } - ListStyleType::CounterStyle(CounterStyleOrNone::Name(CustomIdent(match value { + ListStyleType::CounterStyle(CounterStyle::Name(CustomIdent(match value { structs::NS_STYLE_LIST_STYLE_DISC => atom!("disc"), structs::NS_STYLE_LIST_STYLE_CIRCLE => atom!("circle"), structs::NS_STYLE_LIST_STYLE_SQUARE => atom!("square"), @@ -73,10 +75,12 @@ impl Parse for ListStyleType { context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - if let Ok(style) = input.try(|i| CounterStyleOrNone::parse(context, i)) { + if let Ok(style) = input.try(|i| CounterStyle::parse(context, i)) { return Ok(ListStyleType::CounterStyle(style)); } - + if input.try(|i| i.expect_ident_matching("none")).is_ok() { + return Ok(ListStyleType::None); + } Ok(ListStyleType::String( input.expect_string()?.as_ref().to_owned(), )) diff --git a/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001-ref.xht b/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001-ref.xht deleted file mode 100644 index 699c85549f9f..000000000000 --- a/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001-ref.xht +++ /dev/null @@ -1,43 +0,0 @@ - - - -CSS Reftest Reference - - - - -

The following two lines should look the same:

-
-z -z -z -z -z -z -z -z -z -z -z -z -
-
-z -z -z -z -z -z -z -z -z -z -z -z -
- - diff --git a/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001.xht b/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001.xht index 98e9980506c0..8f06e686e124 100644 --- a/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001.xht +++ b/testing/web-platform/tests/css/CSS2/generated-content/content-counter-001.xht @@ -6,7 +6,7 @@ - +