From f0fbcf204ea1b19adab45c22666ddbd466e3f7d2 Mon Sep 17 00:00:00 2001 From: Yusuf Sermet Date: Mon, 28 May 2018 23:54:01 -0700 Subject: [PATCH 01/16] style: Create a stacking context for contain:paint. Bug: 1463599 Reviewed-by: mattwoodrow MozReview-Commit-ID: Ln72MOlHXwi --- components/style/properties/longhand/box.mako.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/properties/longhand/box.mako.rs b/components/style/properties/longhand/box.mako.rs index 026a3ca7936..f8389a19a79 100644 --- a/components/style/properties/longhand/box.mako.rs +++ b/components/style/properties/longhand/box.mako.rs @@ -593,7 +593,7 @@ ${helpers.predefined_type("contain", "specified::Contain::empty()", animation_value_type="discrete", products="gecko", - flags="FIXPOS_CB", + flags="CREATES_STACKING_CONTEXT FIXPOS_CB", gecko_pref="layout.css.contain.enabled", spec="https://drafts.csswg.org/css-contain/#contain-property")} From 378fcc2b6ab20b9b40ebfe2eaac192ad4e1bccfb Mon Sep 17 00:00:00 2001 From: Hiroyuki Ikezoe Date: Tue, 29 May 2018 12:33:16 +0900 Subject: [PATCH 02/16] style: Implement the smarter interporation for transform. Corresponding to this spec change; https://github.com/w3c/csswg-drafts/commit/32812668df0a086fc2bcc6e51de6ea5f186b82e6 The expected value in test_transitions_per_property.html can be calculated; 'start' + ('end' - 'start') * 0.25 Bug: 1464647 Reviewed-by: birtles, emilio MozReview-Commit-ID: NI9gOUuPnG --- .../helpers/animated_properties.mako.rs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 1c282e46b51..9ada8d523c3 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2564,6 +2564,31 @@ impl Animate for ComputedTransform { owned_other = other; } + // https://drafts.csswg.org/css-transforms-1/#transform-transform-neutral-extend-animation + fn match_operations_if_possible( + this: &mut Vec, + other: &mut Vec, + ) -> Result<(), ()> { + if !this.iter().zip(other.iter()).all(|(this, other)| is_matched_operation(this, other)) { + return Err(()); + } + + if this.len() == other.len() { + return Ok(()) + } + + let (shorter, longer) = if this.len() < other.len() { (this, other) } else { (other, this) }; + shorter.reserve(longer.len()); + for op in longer.iter().skip(shorter.len()) { + shorter.push(op.to_animated_zero()?); + } + Ok(()) + }; + + if match_operations_if_possible(&mut owned_this, &mut owned_other).is_ok() { + return animate_equal_lists(&owned_this, &owned_other) + } + match procedure { Procedure::Add => Err(()), Procedure::Interpolate { progress } => { From e77a28d543226ab54fcb234edd923d8ee76096f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 28 May 2018 19:32:06 +0200 Subject: [PATCH 03/16] style: trivial TopLevelRuleParser construction cleanup. MozReview-Commit-ID: 5CdOdQPZzyb --- components/style/stylesheets/stylesheet.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/style/stylesheets/stylesheet.rs b/components/style/stylesheets/stylesheet.rs index 08c2cb05f7a..5f10c1dd503 100644 --- a/components/style/stylesheets/stylesheet.rs +++ b/components/style/stylesheets/stylesheet.rs @@ -363,13 +363,13 @@ impl Stylesheet { let rule_parser = TopLevelRuleParser { stylesheet_origin: origin, - shared_lock: shared_lock, + shared_lock, loader: stylesheet_loader, - context: context, - error_context: error_context, + context, + error_context, state: State::Start, had_hierarchy_error: false, - namespaces: namespaces, + namespaces, }; { From 142c9eca4b6797af52e73da4ea265558a89d57b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 28 May 2018 23:56:20 +0200 Subject: [PATCH 04/16] style: Don't let @namespace rules that aren't going to be inserted affect the namespace map. Bug: 1464865 Reviewed-by: xidorn MozReview-Commit-ID: 9bjlEBExqsr --- components/style/stylesheets/mod.rs | 39 +++-------- components/style/stylesheets/rule_list.rs | 50 ++++++------- components/style/stylesheets/rule_parser.rs | 77 ++++++++++++++++----- components/style/stylesheets/stylesheet.rs | 3 +- 4 files changed, 94 insertions(+), 75 deletions(-) diff --git a/components/style/stylesheets/mod.rs b/components/style/stylesheets/mod.rs index b3360fa41ab..2d6539c08b4 100644 --- a/components/style/stylesheets/mod.rs +++ b/components/style/stylesheets/mod.rs @@ -46,7 +46,7 @@ pub use self::media_rule::MediaRule; pub use self::namespace_rule::NamespaceRule; pub use self::origin::{Origin, OriginSet, OriginSetIterator, PerOrigin, PerOriginIter}; pub use self::page_rule::PageRule; -pub use self::rule_parser::{State, TopLevelRuleParser}; +pub use self::rule_parser::{State, TopLevelRuleParser, InsertRuleContext}; pub use self::rule_list::{CssRules, CssRulesHelpers}; pub use self::rules_iterator::{AllRules, EffectiveRules}; pub use self::rules_iterator::{NestedRuleIterationCondition, RulesIterator}; @@ -178,12 +178,6 @@ pub enum CssRuleType { Viewport = 15, } -#[allow(missing_docs)] -pub enum SingleRuleParseError { - Syntax, - Hierarchy, -} - #[allow(missing_docs)] pub enum RulesMutateError { Syntax, @@ -192,15 +186,6 @@ pub enum RulesMutateError { InvalidState, } -impl From for RulesMutateError { - fn from(other: SingleRuleParseError) -> Self { - match other { - SingleRuleParseError::Syntax => RulesMutateError::Syntax, - SingleRuleParseError::Hierarchy => RulesMutateError::HierarchyRequest, - } - } -} - impl CssRule { /// Returns the CSSOM rule type of this rule. pub fn rule_type(&self) -> CssRuleType { @@ -236,11 +221,12 @@ impl CssRule { /// Input state is None for a nested rule pub fn parse( css: &str, + insert_rule_context: InsertRuleContext, parent_stylesheet_contents: &StylesheetContents, shared_lock: &SharedRwLock, - state: Option, + state: State, loader: Option<&StylesheetLoader>, - ) -> Result<(Self, State), SingleRuleParseError> { + ) -> Result { let url_data = parent_stylesheet_contents.url_data.read(); let error_reporter = NullReporter; let context = ParserContext::new( @@ -257,28 +243,23 @@ impl CssRule { let mut guard = parent_stylesheet_contents.namespaces.write(); // nested rules are in the body state - let state = state.unwrap_or(State::Body); let mut rule_parser = TopLevelRuleParser { stylesheet_origin: parent_stylesheet_contents.origin, - context: context, + context, error_context: ParserErrorContext { error_reporter: &error_reporter, }, shared_lock: &shared_lock, - loader: loader, - state: state, - had_hierarchy_error: false, + loader, + state, + dom_error: None, namespaces: &mut *guard, + insert_rule_context: Some(insert_rule_context), }; parse_one_rule(&mut input, &mut rule_parser) - .map(|result| (result, rule_parser.state)) .map_err(|_| { - if rule_parser.take_had_hierarchy_error() { - SingleRuleParseError::Hierarchy - } else { - SingleRuleParseError::Syntax - } + rule_parser.dom_error.unwrap_or(RulesMutateError::Syntax) }) } } diff --git a/components/style/stylesheets/rule_list.rs b/components/style/stylesheets/rule_list.rs index 18acdd583bc..35edc22b3da 100644 --- a/components/style/stylesheets/rule_list.rs +++ b/components/style/stylesheets/rule_list.rs @@ -13,7 +13,7 @@ use std::fmt::{self, Write}; use str::CssStringWriter; use stylesheets::{CssRule, RulesMutateError}; use stylesheets::loader::StylesheetLoader; -use stylesheets::rule_parser::State; +use stylesheets::rule_parser::{InsertRuleContext, State}; use stylesheets::stylesheet::StylesheetContents; /// A list of CSS rules. @@ -141,7 +141,7 @@ impl CssRulesHelpers for RawOffsetArc> { nested: bool, loader: Option<&StylesheetLoader>, ) -> Result { - let state = { + let new_rule = { let read_guard = lock.read(); let rules = self.read_with(&read_guard); @@ -151,39 +151,33 @@ impl CssRulesHelpers for RawOffsetArc> { } // Computes the parser state at the given index - if nested { - None + let state = if nested { + State::Body } else if index == 0 { - Some(State::Start) + State::Start } else { - rules.0.get(index - 1).map(CssRule::rule_state) - } - }; + rules.0.get(index - 1).map(CssRule::rule_state).unwrap_or(State::Body) + }; - // Step 3, 4 - // XXXManishearth should we also store the namespace map? - let (new_rule, new_state) = - CssRule::parse(&rule, parent_stylesheet_contents, lock, state, loader)?; + let insert_rule_context = InsertRuleContext { + rule_list: &rules.0, + index, + }; + + // Steps 3, 4, 5, 6 + CssRule::parse( + &rule, + insert_rule_context, + parent_stylesheet_contents, + lock, + state, + loader, + )? + }; { let mut write_guard = lock.write(); let rules = self.write_with(&mut write_guard); - // Step 5 - // Computes the maximum allowed parser state at a given index. - let rev_state = rules.0.get(index).map_or(State::Body, CssRule::rule_state); - if new_state > rev_state { - // We inserted a rule too early, e.g. inserting - // a regular style rule before @namespace rules - return Err(RulesMutateError::HierarchyRequest); - } - - // Step 6 - if let CssRule::Namespace(..) = new_rule { - if !rules.only_ns_or_import() { - return Err(RulesMutateError::InvalidState); - } - } - rules.0.insert(index, new_rule.clone()); } diff --git a/components/style/stylesheets/rule_parser.rs b/components/style/stylesheets/rule_parser.rs index 93f0eaab650..4964db5aae1 100644 --- a/components/style/stylesheets/rule_parser.rs +++ b/components/style/stylesheets/rule_parser.rs @@ -19,7 +19,7 @@ use servo_arc::Arc; use shared_lock::{Locked, SharedRwLock}; use str::starts_with_ignore_ascii_case; use style_traits::{ParseError, StyleParseErrorKind}; -use stylesheets::{CssRule, CssRuleType, CssRules, Origin, StylesheetLoader}; +use stylesheets::{CssRule, CssRuleType, CssRules, Origin, RulesMutateError, StylesheetLoader}; use stylesheets::{DocumentRule, FontFeatureValuesRule, KeyframesRule, MediaRule}; use stylesheets::{NamespaceRule, PageRule, StyleRule, SupportsRule, ViewportRule}; use stylesheets::document_rule::DocumentCondition; @@ -31,6 +31,14 @@ use stylesheets::viewport_rule; use values::{CssUrl, CustomIdent, KeyframesName}; use values::computed::font::FamilyName; +/// The information we need particularly to do CSSOM insertRule stuff. +pub struct InsertRuleContext<'a> { + /// The rule list we're about to insert into. + pub rule_list: &'a [CssRule], + /// The index we're about to get inserted at. + pub index: usize, +} + /// The parser for the top-level rules in a stylesheet. pub struct TopLevelRuleParser<'a, R: 'a> { /// The origin of the stylesheet we're parsing. @@ -51,11 +59,13 @@ pub struct TopLevelRuleParser<'a, R: 'a> { /// Whether we have tried to parse was invalid due to being in the wrong /// place (e.g. an @import rule was found while in the `Body` state). Reset /// to `false` when `take_had_hierarchy_error` is called. - pub had_hierarchy_error: bool, + pub dom_error: Option, /// The namespace map we use for parsing. Needs to start as `Some()`, and /// will be taken out after parsing namespace rules, and that reference will /// be moved to `ParserContext`. pub namespaces: &'a mut Namespaces, + /// The info we need insert a rule in a list. + pub insert_rule_context: Option>, } impl<'b, R> TopLevelRuleParser<'b, R> { @@ -74,14 +84,43 @@ impl<'b, R> TopLevelRuleParser<'b, R> { self.state } - /// Returns whether we previously tried to parse a rule that was invalid - /// due to being in the wrong place (e.g. an @import rule was found after - /// a regular style rule). The state of this flag is reset when this - /// function is called. - pub fn take_had_hierarchy_error(&mut self) -> bool { - let had_hierarchy_error = self.had_hierarchy_error; - self.had_hierarchy_error = false; - had_hierarchy_error + /// Checks whether we can parse a rule that would transition us to + /// `new_state`. + /// + /// This is usually a simple branch, but we may need more bookkeeping if + /// doing `insertRule` from CSSOM. + fn check_state(&mut self, new_state: State) -> bool { + if self.state > new_state { + self.dom_error = Some(RulesMutateError::HierarchyRequest); + return false; + } + + let ctx = match self.insert_rule_context { + Some(ref ctx) => ctx, + None => return true, + }; + + let next_rule_state = match ctx.rule_list.get(ctx.index) { + None => return true, + Some(rule) => rule.rule_state(), + }; + + if new_state > next_rule_state { + self.dom_error = Some(RulesMutateError::HierarchyRequest); + return false; + } + + // If there's anything that isn't a namespace rule (or import rule, but + // we checked that already at the beginning), reject with a + // StateError. + if new_state == State::Namespaces && + ctx.rule_list[ctx.index..].iter().any(|r| !matches!(*r, CssRule::Namespace(..))) + { + self.dom_error = Some(RulesMutateError::InvalidState); + return false; + } + + true } } @@ -151,9 +190,7 @@ impl<'a, 'i, R: ParseErrorReporter> AtRuleParser<'i> for TopLevelRuleParser<'a, let location = input.current_source_location(); match_ignore_ascii_case! { &*name, "import" => { - if self.state > State::Imports { - // "@import must be before any rule but @charset" - self.had_hierarchy_error = true; + if !self.check_state(State::Imports) { return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedImportRule)) } @@ -168,9 +205,7 @@ impl<'a, 'i, R: ParseErrorReporter> AtRuleParser<'i> for TopLevelRuleParser<'a, return Ok(AtRuleType::WithoutBlock(prelude)); }, "namespace" => { - if self.state > State::Namespaces { - // "@namespace must be before any rule but @charset and @import" - self.had_hierarchy_error = true; + if !self.check_state(State::Namespaces) { return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedNamespaceRule)) } @@ -190,12 +225,16 @@ impl<'a, 'i, R: ParseErrorReporter> AtRuleParser<'i> for TopLevelRuleParser<'a, // @charset is removed by rust-cssparser if it’s the first rule in the stylesheet // anything left is invalid. "charset" => { - self.had_hierarchy_error = true; + self.dom_error = Some(RulesMutateError::HierarchyRequest); return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedCharsetRule)) } _ => {} } + if !self.check_state(State::Body) { + return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)) + } + AtRuleParser::parse_prelude(&mut self.nested(), name, input) } @@ -264,6 +303,10 @@ impl<'a, 'i, R: ParseErrorReporter> QualifiedRuleParser<'i> for TopLevelRulePars &mut self, input: &mut Parser<'i, 't>, ) -> Result> { + if !self.check_state(State::Body) { + return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)) + } + QualifiedRuleParser::parse_prelude(&mut self.nested(), input) } diff --git a/components/style/stylesheets/stylesheet.rs b/components/style/stylesheets/stylesheet.rs index 5f10c1dd503..b33875a149c 100644 --- a/components/style/stylesheets/stylesheet.rs +++ b/components/style/stylesheets/stylesheet.rs @@ -368,7 +368,8 @@ impl Stylesheet { context, error_context, state: State::Start, - had_hierarchy_error: false, + dom_error: None, + insert_rule_context: None, namespaces, }; From d5e19a146e4ac52c6f243ae1a9ab7f7aa6501179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 29 May 2018 13:03:54 +0200 Subject: [PATCH 05/16] style: Some trivial cleanup. Bug: 1464865 Reviewed-by: xidorn MozReview-Commit-ID: 8ClaBR9ooGb --- components/style/stylesheets/rule_parser.rs | 64 ++++++++++----------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/components/style/stylesheets/rule_parser.rs b/components/style/stylesheets/rule_parser.rs index 4964db5aae1..a446068faac 100644 --- a/components/style/stylesheets/rule_parser.rs +++ b/components/style/stylesheets/rule_parser.rs @@ -268,8 +268,8 @@ impl<'a, 'i, R: ParseErrorReporter> AtRuleParser<'i> for TopLevelRuleParser<'a, self.state = State::Imports; CssRule::Import(import_rule) }, - AtRuleNonBlockPrelude::Namespace(prefix, url, location) => { - let opt_prefix = if let Some(prefix) = prefix { + AtRuleNonBlockPrelude::Namespace(prefix, url, source_location) => { + let prefix = if let Some(prefix) = prefix { self.namespaces.prefixes.insert(prefix.clone(), url.clone()); Some(prefix) } else { @@ -279,9 +279,9 @@ impl<'a, 'i, R: ParseErrorReporter> AtRuleParser<'i> for TopLevelRuleParser<'a, self.state = State::Namespaces; CssRule::Namespace(Arc::new(self.shared_lock.wrap(NamespaceRule { - prefix: opt_prefix, - url: url, - source_location: location, + prefix, + url, + source_location, }))) }, } @@ -506,27 +506,27 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> AtRuleParser<'i> for NestedRuleParser<'a ), ))) }, - AtRuleBlockPrelude::Media(media_queries, location) => { + AtRuleBlockPrelude::Media(media_queries, source_location) => { Ok(CssRule::Media(Arc::new(self.shared_lock.wrap(MediaRule { - media_queries: media_queries, + media_queries, rules: self.parse_nested_rules(input, CssRuleType::Media), - source_location: location, + source_location, })))) }, - AtRuleBlockPrelude::Supports(cond, location) => { + AtRuleBlockPrelude::Supports(condition, source_location) => { let eval_context = ParserContext::new_with_rule_type( self.context, CssRuleType::Style, self.namespaces, ); - let enabled = cond.eval(&eval_context); + let enabled = condition.eval(&eval_context); Ok(CssRule::Supports(Arc::new(self.shared_lock.wrap( SupportsRule { - condition: cond, + condition, rules: self.parse_nested_rules(input, CssRuleType::Supports), - enabled: enabled, - source_location: location, + enabled, + source_location, }, )))) }, @@ -541,7 +541,7 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> AtRuleParser<'i> for NestedRuleParser<'a ViewportRule::parse(&context, self.error_context, input)?, )))) }, - AtRuleBlockPrelude::Keyframes(name, prefix, location) => { + AtRuleBlockPrelude::Keyframes(name, vendor_prefix, source_location) => { let context = ParserContext::new_with_rule_type( self.context, CssRuleType::Keyframes, @@ -550,19 +550,19 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> AtRuleParser<'i> for NestedRuleParser<'a Ok(CssRule::Keyframes(Arc::new(self.shared_lock.wrap( KeyframesRule { - name: name, + name, keyframes: parse_keyframe_list( &context, self.error_context, input, self.shared_lock, ), - vendor_prefix: prefix, - source_location: location, + vendor_prefix, + source_location, }, )))) }, - AtRuleBlockPrelude::Page(location) => { + AtRuleBlockPrelude::Page(source_location) => { let context = ParserContext::new_with_rule_type( self.context, CssRuleType::Page, @@ -573,21 +573,20 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> AtRuleParser<'i> for NestedRuleParser<'a parse_property_declaration_list(&context, self.error_context, input); Ok(CssRule::Page(Arc::new(self.shared_lock.wrap(PageRule { block: Arc::new(self.shared_lock.wrap(declarations)), - source_location: location, + source_location, })))) }, - AtRuleBlockPrelude::Document(cond, location) => { - if cfg!(feature = "gecko") { - Ok(CssRule::Document(Arc::new(self.shared_lock.wrap( - DocumentRule { - condition: cond, - rules: self.parse_nested_rules(input, CssRuleType::Document), - source_location: location, - }, - )))) - } else { + AtRuleBlockPrelude::Document(condition, source_location) => { + if !cfg!(feature = "gecko") { unreachable!() } + Ok(CssRule::Document(Arc::new(self.shared_lock.wrap( + DocumentRule { + condition, + rules: self.parse_nested_rules(input, CssRuleType::Document), + source_location, + }, + )))) }, } } @@ -608,13 +607,10 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> QualifiedRuleParser<'i> for NestedRulePa url_data: Some(self.context.url_data), }; - let location = input.current_source_location(); + let source_location = input.current_source_location(); let selectors = SelectorList::parse(&selector_parser, input)?; - Ok(QualifiedRuleParserPrelude { - selectors: selectors, - source_location: location, - }) + Ok(QualifiedRuleParserPrelude { selectors, source_location, }) } fn parse_block<'t>( From 92e2adf45f293c16d9a027fa1d3b85bd57bae6e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 29 May 2018 15:09:00 +0200 Subject: [PATCH 06/16] style: Cleanup transform animation. Reviewed-by: hiro But: 1465066 MozReview-Commit-ID: D9rq8CZIgf5 --- .../helpers/animated_properties.mako.rs | 124 +++++++----------- 1 file changed, 50 insertions(+), 74 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 9ada8d523c3..f45b7ffe91e 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2501,107 +2501,83 @@ impl Animate for ComputedScale { /// impl Animate for ComputedTransform { #[inline] - fn animate( - &self, - other_: &Self, - procedure: Procedure, - ) -> Result { - - let animate_equal_lists = |this: &[ComputedTransformOperation], - other: &[ComputedTransformOperation]| - -> Result { - Ok(Transform(this.iter().zip(other) - .map(|(this, other)| this.animate(other, procedure)) - .collect::, _>>()?)) - // If we can't animate for a pair of matched transform lists - // this means we have at least one undecomposable matrix, - // so we should bubble out Err here, and let the caller do - // the fallback procedure. - }; - if self.0.is_empty() && other_.0.is_empty() { - return Ok(Transform(vec![])); - } - - - let this = &self.0; - let other = &other_.0; + fn animate(&self, other: &Self, procedure: Procedure) -> Result { + use std::borrow::Cow; if procedure == Procedure::Add { - let result = this.iter().chain(other).cloned().collect::>(); + let result = self.0.iter().chain(&other.0).cloned().collect::>(); return Ok(Transform(result)); } - - // For matched transform lists. - { - if this.len() == other.len() { - let is_matched_transforms = this.iter().zip(other).all(|(this, other)| { - is_matched_operation(this, other) - }); - - if is_matched_transforms { - return animate_equal_lists(this, other); - } - } - } - - // For mismatched transform lists. - let mut owned_this = this.clone(); - let mut owned_other = other.clone(); - - if this.is_empty() { - let this = other_.to_animated_zero()?.0; - if this.iter().zip(other).all(|(this, other)| is_matched_operation(this, other)) { - return animate_equal_lists(&this, other) - } - owned_this = this; - } - if other.is_empty() { - let other = self.to_animated_zero()?.0; - if this.iter().zip(&other).all(|(this, other)| is_matched_operation(this, other)) { - return animate_equal_lists(this, &other) - } - owned_other = other; - } - // https://drafts.csswg.org/css-transforms-1/#transform-transform-neutral-extend-animation - fn match_operations_if_possible( - this: &mut Vec, - other: &mut Vec, - ) -> Result<(), ()> { + fn match_operations_if_possible<'a>( + this: &mut Cow<'a, Vec>, + other: &mut Cow<'a, Vec>, + ) -> bool { if !this.iter().zip(other.iter()).all(|(this, other)| is_matched_operation(this, other)) { - return Err(()); + return false; } if this.len() == other.len() { - return Ok(()) + return true; } - let (shorter, longer) = if this.len() < other.len() { (this, other) } else { (other, this) }; + let (shorter, longer) = + if this.len() < other.len() { + (this.to_mut(), other) + } else { + (other.to_mut(), this) + }; + shorter.reserve(longer.len()); for op in longer.iter().skip(shorter.len()) { - shorter.push(op.to_animated_zero()?); + shorter.push(op.to_animated_zero().unwrap()); } - Ok(()) - }; - if match_operations_if_possible(&mut owned_this, &mut owned_other).is_ok() { - return animate_equal_lists(&owned_this, &owned_other) + // The resulting operations won't be matched regardless if the + // extended component is already InterpolateMatrix / + // AccumulateMatrix. + // + // Otherwise they should be matching operations all the time. + let already_mismatched = matches!( + longer[0], + TransformOperation::InterpolateMatrix { .. } | + TransformOperation::AccumulateMatrix { .. } + ); + + debug_assert_eq!( + !already_mismatched, + longer.iter().zip(shorter.iter()).all(|(this, other)| is_matched_operation(this, other)), + "ToAnimatedZero should generate matched operations" + ); + + !already_mismatched + } + + let mut this = Cow::Borrowed(&self.0); + let mut other = Cow::Borrowed(&other.0); + + if match_operations_if_possible(&mut this, &mut other) { + return Ok(Transform( + this.iter().zip(other.iter()) + .map(|(this, other)| this.animate(other, procedure)) + .collect::, _>>()? + )); } match procedure { Procedure::Add => Err(()), Procedure::Interpolate { progress } => { Ok(Transform(vec![TransformOperation::InterpolateMatrix { - from_list: Transform(owned_this), - to_list: Transform(owned_other), + from_list: Transform(this.into_owned()), + to_list: Transform(other.into_owned()), progress: Percentage(progress as f32), }])) }, Procedure::Accumulate { count } => { Ok(Transform(vec![TransformOperation::AccumulateMatrix { - from_list: Transform(owned_this), - to_list: Transform(owned_other), + from_list: Transform(this.into_owned()), + to_list: Transform(other.into_owned()), count: cmp::min(count, i32::max_value() as u64) as i32, }])) }, From 5507d53611d7eb3d7f114a863459f3a8b096b10b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 10 May 2018 18:48:08 +0200 Subject: [PATCH 07/16] style: Make element-backed pseudos inherit from NAC subtree roots and other NAC inherit from their parents. Currently, NAC always inherits from the closest non-NAC ancestor element, regardless of whether it is for an element-backed pseudo or not. This patch changes the inheritance so that for element-backed pseudos, we inherit from the closest native anonymous root's parent, and for other NAC we inherit from the parent. This prevents the following two issues and allows us to remove the NODE_IS_NATIVE_ANONYMOUS flag: * Avoiding inheriting from the non-NAC ancestor in XBL bindings bound to NAC. - This is no longer a problem since we apply the rule only if we're a pseudo-element, and all pseudo-elements are in native anonymous subtrees. - This also allows to remove the hack that propagates the NODE_IS_NATIVE_ANONYMOUS flag from the ::cue pseudo-element from BindToTree. * Inheriting from the wrong thing if we're a nested NAC subtree. - We no longer look past our NAC subtree, with the exception of ::-moz-number-text's pseudo-elements, for which we do want to propagate ::placeholder to. A few rules from forms.css have been modified because they're useless or needed to propagate stuff to the anonymous form control in input[type="number"] which previously inherited from the input itself. Bug: 1460382 Reviewed-by: heycam MozReview-Commit-ID: IDKYt3EJtSH --- components/style/dom.rs | 13 ++---- components/style/gecko/wrapper.rs | 70 +++++++++++++++++------------- components/style/matching.rs | 2 +- components/style/sharing/mod.rs | 6 +-- components/style/style_resolver.rs | 4 +- components/style/traversal.rs | 7 +-- 6 files changed, 53 insertions(+), 49 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index 0e5da1542d7..42f6e9ba0c0 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -466,12 +466,6 @@ pub trait TElement: &[] } - /// For a given NAC element, return the closest non-NAC ancestor, which is - /// guaranteed to exist. - fn closest_non_native_anonymous_ancestor(&self) -> Option { - unreachable!("Servo doesn't know about NAC"); - } - /// Get this element's style attribute. fn style_attribute(&self) -> Option>>; @@ -657,9 +651,8 @@ pub trait TElement: false } - /// Returns true if this element is native anonymous (only Gecko has native - /// anonymous content). - fn is_native_anonymous(&self) -> bool { + /// Returns true if this element is in a native anonymous subtree. + fn is_in_native_anonymous_subtree(&self) -> bool { false } @@ -794,7 +787,7 @@ pub trait TElement: /// element. fn rule_hash_target(&self) -> Self { if self.implemented_pseudo_element().is_some() { - self.closest_non_native_anonymous_ancestor() + self.pseudo_element_originating_element() .expect("Trying to collect rules for a detached pseudo-element") } else { *self diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 4b8148bb701..562d6458aa3 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -588,6 +588,20 @@ impl<'le> fmt::Debug for GeckoElement<'le> { } impl<'le> GeckoElement<'le> { + #[inline] + fn closest_anon_subtree_root_parent(&self) -> Option { + debug_assert!(self.is_in_native_anonymous_subtree()); + let mut current = *self; + + loop { + if current.is_root_of_native_anonymous_subtree() { + return current.traversal_parent(); + } + + current = current.traversal_parent()?; + } + } + #[inline] fn may_have_anonymous_children(&self) -> bool { self.as_node() @@ -813,13 +827,6 @@ impl<'le> GeckoElement<'le> { return self.flags() & (NODE_IS_NATIVE_ANONYMOUS_ROOT as u32) != 0; } - /// This logic is duplicated in Gecko's nsINode::IsInNativeAnonymousSubtree. - #[inline] - fn is_in_native_anonymous_subtree(&self) -> bool { - use gecko_bindings::structs::NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE; - self.flags() & (NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE as u32) != 0 - } - /// This logic is duplicated in Gecko's nsIContent::IsInAnonymousSubtree. #[inline] fn is_in_anonymous_subtree(&self) -> bool { @@ -1038,13 +1045,11 @@ impl<'le> TElement for GeckoElement<'le> { type TraversalChildrenIterator = GeckoChildrenIterator<'le>; fn inheritance_parent(&self) -> Option { - if self.is_native_anonymous() { - self.closest_non_native_anonymous_ancestor() - } else { - self.as_node() - .flattened_tree_parent() - .and_then(|n| n.as_element()) + if self.implemented_pseudo_element().is_some() { + return self.pseudo_element_originating_element() } + + self.as_node().flattened_tree_parent().and_then(|n| n.as_element()) } fn traversal_children(&self) -> LayoutIterator> { @@ -1174,19 +1179,6 @@ impl<'le> TElement for GeckoElement<'le> { unsafe { bindings::Gecko_DestroyAnonymousContentList(array) }; } - fn closest_non_native_anonymous_ancestor(&self) -> Option { - debug_assert!(self.is_native_anonymous()); - let mut parent = self.traversal_parent()?; - - loop { - if !parent.is_native_anonymous() { - return Some(parent); - } - - parent = parent.traversal_parent()?; - } - } - #[inline] fn as_node(&self) -> Self::ConcreteNode { unsafe { GeckoNode(&*(self.0 as *const _ as *const RawGeckoNode)) } @@ -1354,10 +1346,11 @@ impl<'le> TElement for GeckoElement<'le> { self.state().intersects(ElementState::IN_VISITED_STATE) } + /// This logic is duplicated in Gecko's nsINode::IsInNativeAnonymousSubtree. #[inline] - fn is_native_anonymous(&self) -> bool { - use gecko_bindings::structs::NODE_IS_NATIVE_ANONYMOUS; - self.flags() & (NODE_IS_NATIVE_ANONYMOUS as u32) != 0 + fn is_in_native_anonymous_subtree(&self) -> bool { + use gecko_bindings::structs::NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE; + self.flags() & (NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE as u32) != 0 } #[inline] @@ -1366,7 +1359,7 @@ impl<'le> TElement for GeckoElement<'le> { } fn implemented_pseudo_element(&self) -> Option { - if !self.is_native_anonymous() { + if !self.is_in_native_anonymous_subtree() { return None; } @@ -1915,7 +1908,22 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { #[inline] fn pseudo_element_originating_element(&self) -> Option { debug_assert!(self.implemented_pseudo_element().is_some()); - self.closest_non_native_anonymous_ancestor() + let parent = self.closest_anon_subtree_root_parent()?; + + // FIXME(emilio): Special-case for s + // pseudo-elements, which are nested NAC. Probably nsNumberControlFrame + // should instead inherit from nsTextControlFrame, and then this could + // go away. + if let Some(PseudoElement::MozNumberText) = parent.implemented_pseudo_element() { + debug_assert_eq!( + self.implemented_pseudo_element().unwrap(), + PseudoElement::Placeholder, + "You added a new pseudo, do you really want this?" + ); + return parent.closest_anon_subtree_root_parent(); + } + + Some(parent) } #[inline] diff --git a/components/style/matching.rs b/components/style/matching.rs index 1d5cbf8d375..05630b07fb7 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -678,7 +678,7 @@ pub trait MatchMethods: TElement { let new_primary_style = data.styles.primary.as_ref().unwrap(); let mut cascade_requirement = ChildCascadeRequirement::CanSkipCascade; - if self.is_root() && !self.is_native_anonymous() { + if self.is_root() && !self.is_in_native_anonymous_subtree() { let device = context.shared.stylist.device(); let new_font_size = new_primary_style.get_font().clone_font_size(); diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 4a9c6d5408c..314e9877083 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -565,7 +565,7 @@ impl StyleSharingCache { }, }; - if element.is_native_anonymous() { + if element.is_in_native_anonymous_subtree() { debug!("Failing to insert into the cache: NAC"); return; } @@ -656,7 +656,7 @@ impl StyleSharingCache { return None; } - if target.is_native_anonymous() { + if target.is_in_native_anonymous_subtree() { debug!("{:?} Cannot share style: NAC", target.element); return None; } @@ -681,7 +681,7 @@ impl StyleSharingCache { nth_index_cache: &mut NthIndexCache, selector_flags_map: &mut SelectorFlagsMap, ) -> Option { - debug_assert!(!target.is_native_anonymous()); + debug_assert!(!target.is_in_native_anonymous_subtree()); // Check that we have the same parent, or at least that the parents // share styles and permit sharing across their children. The latter diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index ab71cdb9bc0..b31beff7460 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -191,7 +191,9 @@ where // Before doing the cascade, check the sharing cache and see if we can // reuse the style via rule node identity. let may_reuse = - !self.element.is_native_anonymous() && parent_style.is_some() && inputs.rules.is_some(); + !self.element.is_in_native_anonymous_subtree() && + parent_style.is_some() && + inputs.rules.is_some(); if may_reuse { let cached = self.context.thread_local.sharing_cache.lookup_by_rules( diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 7092101fff7..0bffd39819c 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -433,9 +433,10 @@ pub fn recalc_style_at( if compute_self { child_cascade_requirement = compute_style(traversal_data, context, element, data); - if element.is_native_anonymous() { - // We must always cascade native anonymous subtrees, since they inherit - // styles from their first non-NAC ancestor. + if element.is_in_native_anonymous_subtree() { + // We must always cascade native anonymous subtrees, since they + // may have pseudo-elements underneath that would inherit from the + // closest non-NAC ancestor instead of us. child_cascade_requirement = cmp::max( child_cascade_requirement, ChildCascadeRequirement::MustCascadeChildren, From 0bfd1dc5c07d5639b6086bbb271ff7186b761fee Mon Sep 17 00:00:00 2001 From: Morgan Rae Reschenberg Date: Wed, 30 May 2018 07:49:31 -0700 Subject: [PATCH 08/16] style: Add contain:size and contain:content parsing functionality. Bug: 1463589 Reviewed-by: emilio MozReview-Commit-ID: 4fOqln3oOpC --- components/style/properties/gecko.mako.rs | 36 ++++++++++++++++++++--- components/style/values/specified/box.rs | 24 +++++++++++---- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index baa1ed015ee..741aa7b86b4 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -3626,10 +3626,13 @@ fn static_assert() { pub fn set_contain(&mut self, v: longhands::contain::computed_value::T) { use gecko_bindings::structs::NS_STYLE_CONTAIN_NONE; use gecko_bindings::structs::NS_STYLE_CONTAIN_STRICT; + use gecko_bindings::structs::NS_STYLE_CONTAIN_CONTENT; + use gecko_bindings::structs::NS_STYLE_CONTAIN_SIZE; use gecko_bindings::structs::NS_STYLE_CONTAIN_LAYOUT; use gecko_bindings::structs::NS_STYLE_CONTAIN_STYLE; use gecko_bindings::structs::NS_STYLE_CONTAIN_PAINT; use gecko_bindings::structs::NS_STYLE_CONTAIN_ALL_BITS; + use gecko_bindings::structs::NS_STYLE_CONTAIN_CONTENT_BITS; use properties::longhands::contain::SpecifiedValue; if v.is_empty() { @@ -3641,6 +3644,10 @@ fn static_assert() { self.gecko.mContain = (NS_STYLE_CONTAIN_STRICT | NS_STYLE_CONTAIN_ALL_BITS) as u8; return; } + if v.contains(SpecifiedValue::CONTENT) { + self.gecko.mContain = (NS_STYLE_CONTAIN_CONTENT | NS_STYLE_CONTAIN_CONTENT_BITS) as u8; + return; + } let mut bitfield = 0; if v.contains(SpecifiedValue::LAYOUT) { @@ -3652,36 +3659,57 @@ fn static_assert() { if v.contains(SpecifiedValue::PAINT) { bitfield |= NS_STYLE_CONTAIN_PAINT; } + if v.contains(SpecifiedValue::SIZE) { + bitfield |= NS_STYLE_CONTAIN_SIZE; + } self.gecko.mContain = bitfield as u8; } pub fn clone_contain(&self) -> longhands::contain::computed_value::T { use gecko_bindings::structs::NS_STYLE_CONTAIN_STRICT; + use gecko_bindings::structs::NS_STYLE_CONTAIN_CONTENT; + use gecko_bindings::structs::NS_STYLE_CONTAIN_SIZE; use gecko_bindings::structs::NS_STYLE_CONTAIN_LAYOUT; use gecko_bindings::structs::NS_STYLE_CONTAIN_STYLE; use gecko_bindings::structs::NS_STYLE_CONTAIN_PAINT; use gecko_bindings::structs::NS_STYLE_CONTAIN_ALL_BITS; + use gecko_bindings::structs::NS_STYLE_CONTAIN_CONTENT_BITS; use properties::longhands::contain::{self, SpecifiedValue}; let mut servo_flags = contain::computed_value::T::empty(); let gecko_flags = self.gecko.mContain; - if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0 && - gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8) != 0 { + if gecko_flags & (NS_STYLE_CONTAIN_STRICT as u8) != 0 { + debug_assert_eq!( + gecko_flags & (NS_STYLE_CONTAIN_ALL_BITS as u8), + NS_STYLE_CONTAIN_ALL_BITS as u8, + "When strict is specified, ALL_BITS should be specified as well" + ); servo_flags.insert(SpecifiedValue::STRICT | SpecifiedValue::STRICT_BITS); return servo_flags; } - + if gecko_flags & (NS_STYLE_CONTAIN_CONTENT as u8) != 0 { + debug_assert_eq!( + gecko_flags & (NS_STYLE_CONTAIN_CONTENT_BITS as u8), + NS_STYLE_CONTAIN_CONTENT_BITS as u8, + "When content is specified, CONTENT_BITS should be specified as well" + ); + servo_flags.insert(SpecifiedValue::CONTENT | SpecifiedValue::CONTENT_BITS); + return servo_flags; + } if gecko_flags & (NS_STYLE_CONTAIN_LAYOUT as u8) != 0 { servo_flags.insert(SpecifiedValue::LAYOUT); } - if gecko_flags & (NS_STYLE_CONTAIN_STYLE as u8) != 0{ + if gecko_flags & (NS_STYLE_CONTAIN_STYLE as u8) != 0 { servo_flags.insert(SpecifiedValue::STYLE); } if gecko_flags & (NS_STYLE_CONTAIN_PAINT as u8) != 0 { servo_flags.insert(SpecifiedValue::PAINT); } + if gecko_flags & (NS_STYLE_CONTAIN_SIZE as u8) != 0 { + servo_flags.insert(SpecifiedValue::SIZE); + } return servo_flags; } diff --git a/components/style/values/specified/box.rs b/components/style/values/specified/box.rs index d716e9e6798..f418b39ef7d 100644 --- a/components/style/values/specified/box.rs +++ b/components/style/values/specified/box.rs @@ -525,19 +525,25 @@ pub fn assert_touch_action_matches() { bitflags! { #[derive(MallocSizeOf, SpecifiedValueInfo, ToComputedValue)] - #[value_info(other_values = "none,strict,layout,style,paint")] + #[value_info(other_values = "none,strict,content,size,layout,style,paint")] /// Constants for contain: https://drafts.csswg.org/css-contain/#contain-property pub struct Contain: u8 { + /// 'size' variant, turns on size containment + const SIZE = 0x01; /// `layout` variant, turns on layout containment - const LAYOUT = 0x01; + const LAYOUT = 0x02; /// `style` variant, turns on style containment - const STYLE = 0x02; + const STYLE = 0x04; /// `paint` variant, turns on paint containment - const PAINT = 0x04; + const PAINT = 0x08; /// `strict` variant, turns on all types of containment - const STRICT = 0x8; + const STRICT = 0x10; + /// 'content' variant, turns on style, layout, and paint containment + const CONTENT = 0x20; /// variant with all the bits that contain: strict turns on - const STRICT_BITS = Contain::LAYOUT.bits | Contain::STYLE.bits | Contain::PAINT.bits; + const STRICT_BITS = Contain::LAYOUT.bits | Contain::STYLE.bits | Contain::PAINT.bits | Contain::SIZE.bits; + /// variant with all the bits that contain: content turns on + const CONTENT_BITS = Contain::STYLE.bits | Contain::LAYOUT.bits | Contain::PAINT.bits; } } @@ -552,6 +558,9 @@ impl ToCss for Contain { if self.contains(Contain::STRICT) { return dest.write_str("strict"); } + if self.contains(Contain::CONTENT) { + return dest.write_str("content"); + } let mut has_any = false; macro_rules! maybe_write_value { @@ -565,6 +574,7 @@ impl ToCss for Contain { } }; } + maybe_write_value!(Contain::SIZE => "size"); maybe_write_value!(Contain::LAYOUT => "layout"); maybe_write_value!(Contain::STYLE => "style"); maybe_write_value!(Contain::PAINT => "paint"); @@ -583,10 +593,12 @@ impl Parse for Contain { let mut result = Contain::empty(); while let Ok(name) = input.try(|i| i.expect_ident_cloned()) { let flag = match_ignore_ascii_case! { &name, + "size" => Some(Contain::SIZE), "layout" => Some(Contain::LAYOUT), "style" => Some(Contain::STYLE), "paint" => Some(Contain::PAINT), "strict" if result.is_empty() => return Ok(Contain::STRICT | Contain::STRICT_BITS), + "content" if result.is_empty() => return Ok(Contain::CONTENT | Contain::CONTENT_BITS), "none" if result.is_empty() => return Ok(result), _ => None }; From 4108b1b2783c89b5d522e88b72a1d8bd4192fb02 Mon Sep 17 00:00:00 2001 From: Dan Glastonbury Date: Fri, 27 Apr 2018 12:07:20 +1000 Subject: [PATCH 09/16] style: Change nscolor to StyleComplexColor in SVG properties. Change mStopColor, mFloodColor, and mLightingColor in nsStyleSVGReset. Bug: 1457353 Reviewed-by: xidorn MozReview-Commit-ID: KMRMtHk1jNK --- components/style/properties/longhand/svg.mako.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/components/style/properties/longhand/svg.mako.rs b/components/style/properties/longhand/svg.mako.rs index 2ec7bcc59a1..6d7bf803f1e 100644 --- a/components/style/properties/longhand/svg.mako.rs +++ b/components/style/properties/longhand/svg.mako.rs @@ -21,8 +21,8 @@ ${helpers.single_keyword("vector-effect", "none non-scaling-stroke", ${helpers.predefined_type( "stop-color", - "RGBAColor", - "RGBA::new(0, 0, 0, 255)", + "Color", + "RGBA::new(0, 0, 0, 255).into()", products="gecko", animation_value_type="AnimatedRGBA", spec="https://www.w3.org/TR/SVGTiny12/painting.html#StopColorProperty", @@ -37,10 +37,10 @@ ${helpers.predefined_type("stop-opacity", "Opacity", "1.0", ${helpers.predefined_type( "flood-color", - "RGBAColor", - "RGBA::new(0, 0, 0, 255)", + "Color", + "RGBA::new(0, 0, 0, 255).into()", products="gecko", - animation_value_type="AnimatedRGBA", + animation_value_type="AnimatedColor", spec="https://www.w3.org/TR/SVG/filters.html#FloodColorProperty", )} @@ -50,10 +50,10 @@ ${helpers.predefined_type("flood-opacity", "Opacity", ${helpers.predefined_type( "lighting-color", - "RGBAColor", - "RGBA::new(255, 255, 255, 255)", + "Color", + "RGBA::new(255, 255, 255, 255).into()", products="gecko", - animation_value_type="AnimatedRGBA", + animation_value_type="AnimatedColor", spec="https://www.w3.org/TR/SVG/filters.html#LightingColorProperty", )} From 93472bcdeaa632fe58e91591984b22309175e269 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 30 May 2018 18:15:25 +0200 Subject: [PATCH 10/16] style: Merge ServoDeclarationBlock and DeclarationBlock. Bug: 1464496 Reviewed-by: emilio MozReview-Commit-ID: By9fV70Oq0K --- components/style/gecko/wrapper.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 562d6458aa3..9ff92a19e16 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1215,16 +1215,9 @@ impl<'le> TElement for GeckoElement<'le> { unsafe { let slots = self.extended_slots()?; - let base_declaration: &structs::DeclarationBlock = + let declaration: &structs::DeclarationBlock = slots.mSMILOverrideStyleDeclaration.mRawPtr.as_ref()?; - let declaration: &structs::ServoDeclarationBlock = mem::transmute(base_declaration); - - debug_assert_eq!( - &declaration._base as *const structs::DeclarationBlock, - base_declaration as *const structs::DeclarationBlock - ); - let raw: &structs::RawServoDeclarationBlock = declaration.mRaw.mRawPtr.as_ref()?; Some( From 5db1387f39676f1a5ae902789a24095307b8464d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 1 Jun 2018 11:35:57 +0200 Subject: [PATCH 11/16] style: Make will-change honor prefs properly, and clean it up while at it. Bug: 1466008 Reviewed-by: xidorn MozReview-Commit-ID: JyzwaRgf5Ct --- components/style/properties/gecko.mako.rs | 66 ++----------- .../style/properties/properties.mako.rs | 6 +- components/style/values/specified/box.rs | 97 +++++++++++++++++-- 3 files changed, 103 insertions(+), 66 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 741aa7b86b4..4b902358866 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -3515,77 +3515,27 @@ fn static_assert() { pub fn set_will_change(&mut self, v: longhands::will_change::computed_value::T) { use gecko_bindings::bindings::{Gecko_AppendWillChange, Gecko_ClearWillChange}; - use gecko_bindings::structs::NS_STYLE_WILL_CHANGE_OPACITY; - use gecko_bindings::structs::NS_STYLE_WILL_CHANGE_SCROLL; - use gecko_bindings::structs::NS_STYLE_WILL_CHANGE_TRANSFORM; - use properties::PropertyId; use properties::longhands::will_change::computed_value::T; - fn will_change_bitfield_from_prop_flags(prop: LonghandId) -> u8 { - use properties::PropertyFlags; - use gecko_bindings::structs::NS_STYLE_WILL_CHANGE_ABSPOS_CB; - use gecko_bindings::structs::NS_STYLE_WILL_CHANGE_FIXPOS_CB; - use gecko_bindings::structs::NS_STYLE_WILL_CHANGE_STACKING_CONTEXT; - let servo_flags = prop.flags(); - let mut bitfield = 0; - - if servo_flags.contains(PropertyFlags::CREATES_STACKING_CONTEXT) { - bitfield |= NS_STYLE_WILL_CHANGE_STACKING_CONTEXT; - } - if servo_flags.contains(PropertyFlags::FIXPOS_CB) { - bitfield |= NS_STYLE_WILL_CHANGE_FIXPOS_CB; - } - if servo_flags.contains(PropertyFlags::ABSPOS_CB) { - bitfield |= NS_STYLE_WILL_CHANGE_ABSPOS_CB; - } - - bitfield as u8 - } - - self.gecko.mWillChangeBitField = 0; - match v { - T::AnimateableFeatures(features) => { + T::AnimateableFeatures { features, bits } => { unsafe { Gecko_ClearWillChange(&mut self.gecko, features.len()); } for feature in features.iter() { - if feature.0 == atom!("scroll-position") { - self.gecko.mWillChangeBitField |= NS_STYLE_WILL_CHANGE_SCROLL as u8; - } else if feature.0 == atom!("opacity") { - self.gecko.mWillChangeBitField |= NS_STYLE_WILL_CHANGE_OPACITY as u8; - } else if feature.0 == atom!("transform") { - self.gecko.mWillChangeBitField |= NS_STYLE_WILL_CHANGE_TRANSFORM as u8; - } - unsafe { - Gecko_AppendWillChange(&mut self.gecko, feature.0.as_ptr()); - } - - if let Ok(prop_id) = PropertyId::parse(&feature.0.to_string()) { - match prop_id.as_shorthand() { - Ok(shorthand) => { - for longhand in shorthand.longhands() { - self.gecko.mWillChangeBitField |= - will_change_bitfield_from_prop_flags(longhand); - } - }, - Err(longhand_or_custom) => { - if let PropertyDeclarationId::Longhand(longhand) - = longhand_or_custom { - self.gecko.mWillChangeBitField |= - will_change_bitfield_from_prop_flags(longhand); - } - }, - } + Gecko_AppendWillChange(&mut self.gecko, feature.0.as_ptr()) } } + + self.gecko.mWillChangeBitField = bits.bits(); }, T::Auto => { unsafe { Gecko_ClearWillChange(&mut self.gecko, 0); } + self.gecko.mWillChangeBitField = 0; }, }; } @@ -3607,6 +3557,7 @@ fn static_assert() { use properties::longhands::will_change::computed_value::T; use gecko_bindings::structs::nsAtom; use values::CustomIdent; + use values::specified::box_::WillChangeBits; if self.gecko.mWillChange.len() == 0 { return T::Auto @@ -3618,7 +3569,10 @@ fn static_assert() { } }).collect(); - T::AnimateableFeatures(custom_idents.into_boxed_slice()) + T::AnimateableFeatures { + features: custom_idents.into_boxed_slice(), + bits: WillChangeBits::from_bits_truncate(self.gecko.mWillChangeBitField), + } } <% impl_shape_source("shape_outside", "mShapeOutside") %> diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 81e9a09cbe8..7696f15c8b8 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1715,7 +1715,8 @@ impl PropertyId { } } - fn non_custom_id(&self) -> Option { + /// Returns the non-custom property id for this property. + pub fn non_custom_id(&self) -> Option { Some(match *self { PropertyId::Custom(_) => return None, PropertyId::Shorthand(shorthand_id) => shorthand_id.into(), @@ -1750,7 +1751,8 @@ impl PropertyId { id.enabled_for_all_content() } - fn allowed_in(&self, context: &ParserContext) -> bool { + /// Returns whether the property is allowed in a given context. + pub fn allowed_in(&self, context: &ParserContext) -> bool { let id = match self.non_custom_id() { // Custom properties are allowed everywhere None => return true, diff --git a/components/style/values/specified/box.rs b/components/style/values/specified/box.rs index f418b39ef7d..ed118acc74b 100644 --- a/components/style/values/specified/box.rs +++ b/components/style/values/specified/box.rs @@ -7,6 +7,7 @@ use Atom; use cssparser::Parser; use parser::{Parse, ParserContext}; +use properties::{LonghandId, PropertyId, PropertyFlags, PropertyDeclarationId}; use selectors::parser::SelectorParseErrorKind; use std::fmt::{self, Write}; use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss}; @@ -382,7 +383,15 @@ pub enum WillChange { Auto, /// #[css(comma)] - AnimateableFeatures(#[css(iterable)] Box<[CustomIdent]>), + AnimateableFeatures { + /// The features that are supposed to change. + #[css(iterable)] + features: Box<[CustomIdent]>, + /// A bitfield with the kind of change that the value will create, based + /// on the above field. + #[css(skip)] + bits: WillChangeBits, + }, } impl WillChange { @@ -393,10 +402,72 @@ impl WillChange { } } +bitflags! { + /// The change bits that we care about. + /// + /// These need to be in sync with NS_STYLE_WILL_CHANGE_*. + #[derive(MallocSizeOf, SpecifiedValueInfo, ToComputedValue)] + pub struct WillChangeBits: u8 { + /// Whether the stacking context will change. + const STACKING_CONTEXT = 1 << 0; + /// Whether `transform` will change. + const TRANSFORM = 1 << 1; + /// Whether `scroll-position` will change. + const SCROLL = 1 << 2; + /// Whether `opacity` will change. + const OPACITY = 1 << 3; + /// Fixed pos containing block. + const FIXPOS_CB = 1 << 4; + /// Abs pos containing block. + const ABSPOS_CB = 1 << 5; + } +} + +fn change_bits_for_longhand(longhand: LonghandId) -> WillChangeBits { + let mut flags = match longhand { + LonghandId::Opacity => WillChangeBits::OPACITY, + LonghandId::Transform => WillChangeBits::TRANSFORM, + _ => WillChangeBits::empty(), + }; + + let property_flags = longhand.flags(); + if property_flags.contains(PropertyFlags::CREATES_STACKING_CONTEXT) { + flags |= WillChangeBits::STACKING_CONTEXT; + } + if property_flags.contains(PropertyFlags::FIXPOS_CB) { + flags |= WillChangeBits::FIXPOS_CB; + } + if property_flags.contains(PropertyFlags::ABSPOS_CB) { + flags |= WillChangeBits::ABSPOS_CB; + } + flags +} + +fn change_bits_for_maybe_property(ident: &str, context: &ParserContext) -> WillChangeBits { + let id = match PropertyId::parse(ident) { + Ok(id) => id, + Err(..) => return WillChangeBits::empty(), + }; + + if !id.allowed_in(context) { + return WillChangeBits::empty(); + } + + match id.as_shorthand() { + Ok(shorthand) => { + shorthand.longhands().fold(WillChangeBits::empty(), |flags, p| { + flags | change_bits_for_longhand(p) + }) + } + Err(PropertyDeclarationId::Longhand(longhand)) => change_bits_for_longhand(longhand), + Err(PropertyDeclarationId::Custom(..)) => WillChangeBits::empty(), + } +} + impl Parse for WillChange { /// auto | # fn parse<'i, 't>( - _context: &ParserContext, + context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { if input @@ -406,18 +477,28 @@ impl Parse for WillChange { return Ok(WillChange::Auto); } + let mut bits = WillChangeBits::empty(); let custom_idents = input.parse_comma_separated(|i| { let location = i.current_source_location(); - CustomIdent::from_ident( + let parser_ident = i.expect_ident()?; + let ident = CustomIdent::from_ident( location, - i.expect_ident()?, + parser_ident, &["will-change", "none", "all", "auto"], - ) + )?; + + if ident.0 == atom!("scroll-position") { + bits |= WillChangeBits::SCROLL; + } else { + bits |= change_bits_for_maybe_property(&parser_ident, context); + } + Ok(ident) })?; - Ok(WillChange::AnimateableFeatures( - custom_idents.into_boxed_slice(), - )) + Ok(WillChange::AnimateableFeatures { + features: custom_idents.into_boxed_slice(), + bits, + }) } } From 600f19540ee9adde68033ba0060442b484ab4892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 1 Jun 2018 14:00:57 +0200 Subject: [PATCH 12/16] style: Make PropertyId::parse less of a footgun. Bug: 1466095 Reviewed-by: xidorn MozReview-Commit-ID: 2BmtSDPmHj9 --- .../style/properties/declaration_block.rs | 2 +- .../style/properties/properties.mako.rs | 47 +++++++++++++------ .../style/stylesheets/keyframes_rule.rs | 9 ++-- components/style/stylesheets/supports_rule.rs | 6 +-- components/style/values/specified/box.rs | 6 +-- 5 files changed, 44 insertions(+), 26 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 0263de83fef..a66058caf1e 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -1158,7 +1158,7 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> { name: CowRcStr<'i>, input: &mut Parser<'i, 't>, ) -> Result> { - let id = match PropertyId::parse(&name) { + let id = match PropertyId::parse(&name, self.context) { Ok(id) => id, Err(..) => { return Err(input.new_custom_error(if is_non_mozilla_vendor_identifier(&name) { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 7696f15c8b8..9ce7d3dfc38 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1595,7 +1595,7 @@ impl PropertyId { /// Returns a given property from the string `s`. /// /// Returns Err(()) for unknown non-custom properties. - pub fn parse(property_name: &str) -> Result { + fn parse_unchecked(property_name: &str) -> Result { // FIXME(https://github.com/rust-lang/rust/issues/33156): remove this // enum and use PropertyId when stable Rust allows destructors in // statics. @@ -1639,13 +1639,39 @@ impl PropertyId { PropertyId::ShorthandAlias(id, alias) }, None => { - return ::custom_properties::parse_name(property_name).map(|name| { - PropertyId::Custom(::custom_properties::Name::from(name)) - }) + let name = ::custom_properties::parse_name(property_name)?; + PropertyId::Custom(::custom_properties::Name::from(name)) }, }) } + /// Parses a property name, and returns an error if it's unknown or isn't + /// enabled for all content. + #[inline] + pub fn parse_enabled_for_all_content(name: &str) -> Result { + let id = Self::parse_unchecked(name)?; + + if !id.enabled_for_all_content() { + return Err(()); + } + + Ok(id) + } + + + /// Parses a property name, and returns an error if it's unknown or isn't + /// allowed in this context. + #[inline] + pub fn parse(name: &str, context: &ParserContext) -> Result { + let id = Self::parse_unchecked(name)?; + + if !id.allowed_in(context) { + return Err(()); + } + + Ok(id) + } + /// Returns a property id from Gecko's nsCSSPropertyID. #[cfg(feature = "gecko")] #[allow(non_upper_case_globals)] @@ -1715,8 +1741,7 @@ impl PropertyId { } } - /// Returns the non-custom property id for this property. - pub fn non_custom_id(&self) -> Option { + fn non_custom_id(&self) -> Option { Some(match *self { PropertyId::Custom(_) => return None, PropertyId::Shorthand(shorthand_id) => shorthand_id.into(), @@ -1751,8 +1776,7 @@ impl PropertyId { id.enabled_for_all_content() } - /// Returns whether the property is allowed in a given context. - pub fn allowed_in(&self, context: &ParserContext) -> bool { + fn allowed_in(&self, context: &ParserContext) -> bool { let id = match self.non_custom_id() { // Custom properties are allowed everywhere None => return true, @@ -1974,12 +1998,7 @@ impl PropertyDeclaration { input: &mut Parser<'i, 't>, ) -> Result<(), ParseError<'i>> { assert!(declarations.is_empty()); - - if !id.allowed_in(context) { - return Err(input.new_custom_error( - StyleParseErrorKind::UnknownProperty(name) - )); - } + debug_assert!(id.allowed_in(context), "{:?}", id); let start = input.state(); match id { diff --git a/components/style/stylesheets/keyframes_rule.rs b/components/style/stylesheets/keyframes_rule.rs index 4de3acbaff5..afe797c12ff 100644 --- a/components/style/stylesheets/keyframes_rule.rs +++ b/components/style/stylesheets/keyframes_rule.rs @@ -620,9 +620,12 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for KeyframeDeclarationParser<'a, 'b> { name: CowRcStr<'i>, input: &mut Parser<'i, 't>, ) -> Result<(), ParseError<'i>> { - let id = PropertyId::parse(&name).map_err(|()| { - input.new_custom_error(StyleParseErrorKind::UnknownProperty(name.clone())) - })?; + let id = match PropertyId::parse(&name, self.context) { + Ok(id) => id, + Err(()) => return Err(input.new_custom_error( + StyleParseErrorKind::UnknownProperty(name.clone()) + )), + }; // TODO(emilio): Shouldn't this use parse_entirely? PropertyDeclaration::parse_into(self.declarations, id, name, self.context, input)?; diff --git a/components/style/stylesheets/supports_rule.rs b/components/style/stylesheets/supports_rule.rs index d94a9a0202a..5e2739d153d 100644 --- a/components/style/stylesheets/supports_rule.rs +++ b/components/style/stylesheets/supports_rule.rs @@ -315,12 +315,12 @@ impl Declaration { let mut input = ParserInput::new(&self.0); let mut input = Parser::new(&mut input); - input - .parse_entirely(|input| -> Result<(), CssParseError<()>> { + input.parse_entirely(|input| -> Result<(), CssParseError<()>> { let prop = input.expect_ident_cloned().unwrap(); input.expect_colon().unwrap(); - let id = PropertyId::parse(&prop).map_err(|_| input.new_custom_error(()))?; + let id = PropertyId::parse(&prop, context) + .map_err(|_| input.new_custom_error(()))?; let mut declarations = SourcePropertyDeclaration::new(); input.parse_until_before(Delimiter::Bang, |input| { diff --git a/components/style/values/specified/box.rs b/components/style/values/specified/box.rs index ed118acc74b..dcce3d592ab 100644 --- a/components/style/values/specified/box.rs +++ b/components/style/values/specified/box.rs @@ -444,15 +444,11 @@ fn change_bits_for_longhand(longhand: LonghandId) -> WillChangeBits { } fn change_bits_for_maybe_property(ident: &str, context: &ParserContext) -> WillChangeBits { - let id = match PropertyId::parse(ident) { + let id = match PropertyId::parse(ident, context) { Ok(id) => id, Err(..) => return WillChangeBits::empty(), }; - if !id.allowed_in(context) { - return WillChangeBits::empty(); - } - match id.as_shorthand() { Ok(shorthand) => { shorthand.longhands().fold(WillChangeBits::empty(), |flags, p| { From 8777c4ee638406eee0d109715080dd1dc135a070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 1 Jun 2018 16:13:15 +0200 Subject: [PATCH 13/16] style: followup: set the rule type in the custom properties code, since we use it. Though I think it may be slightly fishy if used in, e.g., a @keyframes block. For our purposes right now it doesn't make a difference, I think. Bug: 1466008 MozReview-Commit-ID: A7VCTOqaIuB --- components/style/properties/properties.mako.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 9ce7d3dfc38..b7bc96973b3 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1422,10 +1422,12 @@ impl UnparsedValue { .and_then(|css| { // As of this writing, only the base URL is used for property // values. + // + // FIXME(emilio): These bits are slightly fishy. let context = ParserContext::new( Origin::Author, &self.url_data, - None, + Some(CssRuleType::Style), ParsingMode::DEFAULT, quirks_mode, ); From 9bb72139e40571126f9b2872c77a4f38bb0b568b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 2 Jun 2018 12:17:44 +0200 Subject: [PATCH 14/16] atoms: Add scroll-position as a static atom. That way we avoid cfg churn in WillChange stuff. --- components/atoms/static_atoms.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/components/atoms/static_atoms.txt b/components/atoms/static_atoms.txt index 8b168ae2fd5..cf8eb460383 100644 --- a/components/atoms/static_atoms.txt +++ b/components/atoms/static_atoms.txt @@ -65,6 +65,7 @@ reset right sans-serif screen +scroll-position search select serif From 274bc4df3e78eb7adb02be439c2720d383211b79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 2 Jun 2018 12:24:44 +0200 Subject: [PATCH 15/16] dom: don't parse internal properties in CSSStyleDeclaration. --- components/script/dom/cssstyledeclaration.rs | 34 ++++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs index 1e95cd9d27d..e63d5833909 100644 --- a/components/script/dom/cssstyledeclaration.rs +++ b/components/script/dom/cssstyledeclaration.rs @@ -312,22 +312,18 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration { // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue fn GetPropertyValue(&self, property: DOMString) -> DOMString { - let id = if let Ok(id) = PropertyId::parse(&property) { - id - } else { - // Unkwown property - return DOMString::new() + let id = match PropertyId::parse_enabled_for_all_content(&property) { + Ok(id) => id, + Err(..) => return DOMString::new(), }; self.get_property_value(id) } // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertypriority fn GetPropertyPriority(&self, property: DOMString) -> DOMString { - let id = if let Ok(id) = PropertyId::parse(&property) { - id - } else { - // Unkwown property - return DOMString::new() + let id = match PropertyId::parse_enabled_for_all_content(&property) { + Ok(id) => id, + Err(..) => return DOMString::new(), }; self.owner.with_block(|pdb| { @@ -347,11 +343,9 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration { priority: DOMString) -> ErrorResult { // Step 3 - let id = if let Ok(id) = PropertyId::parse(&property) { - id - } else { - // Unknown property - return Ok(()) + let id = match PropertyId::parse_enabled_for_all_content(&property) { + Ok(id) => id, + Err(..) => return Ok(()), }; self.set_property(id, value, priority) } @@ -364,7 +358,7 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration { } // Step 2 & 3 - let id = match PropertyId::parse(&property) { + let id = match PropertyId::parse_enabled_for_all_content(&property) { Ok(id) => id, Err(..) => return Ok(()), // Unkwown property }; @@ -396,11 +390,9 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration { return Err(Error::NoModificationAllowed); } - let id = if let Ok(id) = PropertyId::parse(&property) { - id - } else { - // Unkwown property, cannot be there to remove. - return Ok(DOMString::new()) + let id = match PropertyId::parse_enabled_for_all_content(&property) { + Ok(id) => id, + Err(..) => return Ok(DOMString::new()), }; let mut string = String::new(); From 97d8f6b09ea882d36294f5a8b414202f40be8818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 2 Jun 2018 12:27:42 +0200 Subject: [PATCH 16/16] layout_thread: Don't parse internal properties in paint registration code. --- components/layout_thread/lib.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index d393f23c2c4..f0383e871fa 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -757,14 +757,16 @@ impl LayoutThread { Msg::RegisterPaint(name, mut properties, painter) => { debug!("Registering the painter"); let properties = properties.drain(..) - .filter_map(|name| PropertyId::parse(&*name) - .ok().map(|id| (name.clone(), id))) - .filter(|&(_, ref id)| id.as_shorthand().is_err()) + .filter_map(|name| { + let id = PropertyId::parse_enabled_for_all_content(&*name).ok()?; + Some((name.clone(), id)) + }) + .filter(|&(_, ref id)| !id.is_shorthand()) .collect(); let registered_painter = RegisteredPainterImpl { name: name.clone(), - properties: properties, - painter: painter, + properties, + painter, }; self.registered_painters.0.insert(name, registered_painter); },