From cd63d7b2721a5e3e84a083a90e316d0340e6ecd9 Mon Sep 17 00:00:00 2001 From: Daniel Holbert Date: Thu, 23 Apr 2020 05:01:07 +0000 Subject: [PATCH 01/42] style: Remove pref for CSS Containment (layout.css.contain.enabled) r=AlaskanEmily (Since we've been shipping with it default-enabled for a while now.) See https://bugzilla.mozilla.org/show_bug.cgi?id=1466008#c9 through #c13 for notes on the reftest.list change. Differential Revision: https://phabricator.services.mozilla.com/D71861 --- components/style/properties/longhands/box.mako.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/components/style/properties/longhands/box.mako.rs b/components/style/properties/longhands/box.mako.rs index 4a9b85a0883..53aaf453d92 100644 --- a/components/style/properties/longhands/box.mako.rs +++ b/components/style/properties/longhands/box.mako.rs @@ -612,9 +612,7 @@ ${helpers.predefined_type( engines="gecko", animation_value_type="none", flags="CREATES_STACKING_CONTEXT FIXPOS_CB", - gecko_pref="layout.css.contain.enabled", spec="https://drafts.csswg.org/css-contain/#contain-property", - enabled_in="chrome", )} // Non-standard From c1bc588c932bbae5a46a67f571b818f5afa92baf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 23 Apr 2020 19:20:03 +0000 Subject: [PATCH 02/42] style: Keep track of nested dependencies for :where() and :is(). The tricky part of :is() and :where() is that they can have combinators inside, so something like this is valid: foo:is(#bar > .baz) ~ taz The current invalidation logic is based on the assumption that you can represent a combinator as a (selector, offset) tuple, which are stored in the Dependency struct. This assumption breaks with :is() and :where(), so we need to make them be able to represent a combinator in an "inner" selector. For this purpose, we add a `parent` dependency. With it, when invalidating inside the `:is()` we can represent combinators inside as a stack. The basic idea is that, for the example above, when an id of "bar" is added or removed, we'd find a dependency like: Dependency { selector: #bar > .baz, offset: 1, // pointing to the `>` combinator parent: Some(Dependency { selector: foo:is(#bar > .baz) > taz, offset: 1, // Pointing to the `~` combinator. parent: None, }) } That way, we'd start matching at the element that changed, towards the right, and if we find an element that matches .baz, instead of invalidating that element, we'd look at the parent dependency, then double-check that the whole left-hand-side of the selector (foo:is(#bar > .baz)) actually changed, and then keep invalidating to the right using the parent dependency as usual. This patch only builds the data structure and keeps the code compiling, the actual invalidation work will come in a following patch. Differential Revision: https://phabricator.services.mozilla.com/D71421 --- .../invalidation/element/invalidation_map.rs | 335 +++++++++++------- .../element/state_and_attributes.rs | 64 ++-- 2 files changed, 249 insertions(+), 150 deletions(-) diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index dca418703be..3dd5134ec34 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -47,6 +47,23 @@ pub struct Dependency { /// The offset into the selector that we should match on. pub selector_offset: usize, + + /// The parent dependency for an ancestor selector. For example, consider + /// the following: + /// + /// .foo .bar:where(.baz span) .qux + /// ^ ^ ^ + /// A B C + /// + /// We'd generate: + /// + /// * One dependency for .quz (offset: 0, parent: None) + /// * One dependency for .baz pointing to B with parent being a + /// dependency pointing to C. + /// * One dependency from .bar pointing to C (parent: None) + /// * One dependency from .foo pointing to A (parent: None) + /// + pub parent: Option>, } /// The kind of elements down the tree this dependency may affect. @@ -227,78 +244,120 @@ impl InvalidationMap { ) -> Result<(), FailedAllocationError> { debug!("InvalidationMap::note_selector({:?})", selector); - let mut iter = selector.iter(); + let mut document_state = DocumentState::empty(); + + { + let mut parent_stack = SmallVec::new(); + let mut alloc_error = None; + let mut collector = SelectorDependencyCollector { + map: self, + document_state: &mut document_state, + selector, + parent_selectors: &mut parent_stack, + quirks_mode, + compound_state: PerCompoundState::new(0), + alloc_error: &mut alloc_error, + }; + + let visit_result = collector.visit_whole_selector(); + debug_assert_eq!(!visit_result, alloc_error.is_some()); + if let Some(alloc_error) = alloc_error { + return Err(alloc_error); + } + } + + if !document_state.is_empty() { + let dep = DocumentStateDependency { + state: document_state, + selector: selector.clone(), + }; + self.document_state_selectors.try_push(dep)?; + } + + Ok(()) + } +} + +struct PerCompoundState { + /// The offset at which our compound starts. + offset: usize, + + /// The state this compound selector is affected by. + element_state: ElementState, + + /// Whether we've added a generic attribute dependency for this selector. + added_attr_dependency: bool, +} + +impl PerCompoundState { + fn new(offset: usize) -> Self { + Self { + offset, + element_state: ElementState::empty(), + added_attr_dependency: false, + } + } +} + +/// A struct that collects invalidations for a given compound selector. +struct SelectorDependencyCollector<'a> { + map: &'a mut InvalidationMap, + + /// The document this _complex_ selector is affected by. + /// + /// We don't need to track state per compound selector, since it's global + /// state and it changes for everything. + document_state: &'a mut DocumentState, + + /// The current selector and offset we're iterating. + selector: &'a Selector, + + /// The stack of parent selectors that we have, and at which offset of the + /// sequence. + /// + /// This starts empty. It grows when we find nested :is and :where selector + /// lists. + parent_selectors: &'a mut SmallVec<[(Selector, usize); 5]>, + + /// The quirks mode of the document where we're inserting dependencies. + quirks_mode: QuirksMode, + + /// State relevant to a given compound selector. + compound_state: PerCompoundState, + + /// The allocation error, if we OOM. + alloc_error: &'a mut Option, +} + +impl<'a> SelectorDependencyCollector<'a> { + fn visit_whole_selector(&mut self) -> bool { + let mut iter = self.selector.iter(); let mut combinator; let mut index = 0; - let mut document_state = DocumentState::empty(); - loop { - let sequence_start = index; - - let mut compound_visitor = CompoundSelectorDependencyCollector { - classes: SmallVec::new(), - ids: SmallVec::new(), - state: ElementState::empty(), - document_state: &mut document_state, - other_attributes: false, - flags: &mut self.flags, - }; + // Reset the compound state. + self.compound_state = PerCompoundState::new(index); // Visit all the simple selectors in this sequence. - // - // Note that this works because we can't have combinators nested - // inside simple selectors (i.e. in :not() or :-moz-any()). - // - // If we ever support that we'll need to visit nested complex - // selectors as well, in order to mark them as affecting descendants - // at least. for ss in &mut iter { - ss.visit(&mut compound_visitor); + ss.visit(self); index += 1; // Account for the simple selector. } - for class in compound_visitor.classes { - self.class_to_selector - .try_entry(class, quirks_mode)? - .or_insert_with(SmallVec::new) - .try_push(Dependency { - selector: selector.clone(), - selector_offset: sequence_start, - })?; - } - - for id in compound_visitor.ids { - self.id_to_selector - .try_entry(id, quirks_mode)? - .or_insert_with(SmallVec::new) - .try_push(Dependency { - selector: selector.clone(), - selector_offset: sequence_start, - })?; - } - - if !compound_visitor.state.is_empty() { - self.state_affecting_selectors.insert( + if !self.compound_state.element_state.is_empty() { + let dependency = self.dependency(); + let result = self.map.state_affecting_selectors.insert( StateDependency { - dep: Dependency { - selector: selector.clone(), - selector_offset: sequence_start, - }, - state: compound_visitor.state, + dep: dependency, + state: self.compound_state.element_state, }, - quirks_mode, - )?; - } - - if compound_visitor.other_attributes { - self.other_attribute_affecting_selectors.insert( - Dependency { - selector: selector.clone(), - selector_offset: sequence_start, - }, - quirks_mode, - )?; + self.quirks_mode, + ); + if let Err(alloc_error) = result { + *self.alloc_error = Some(alloc_error); + return false; + } } combinator = iter.next_sequence(); @@ -309,88 +368,107 @@ impl InvalidationMap { index += 1; // Account for the combinator. } - if !document_state.is_empty() { - self.document_state_selectors - .try_push(DocumentStateDependency { - state: document_state, - selector: selector.clone(), - })?; + true + } + + fn add_attr_dependency(&mut self) -> bool { + debug_assert!(!self.compound_state.added_attr_dependency); + self.compound_state.added_attr_dependency = true; + + let dependency = self.dependency(); + let result = self.map.other_attribute_affecting_selectors.insert( + dependency, + self.quirks_mode, + ); + if let Err(alloc_error) = result { + *self.alloc_error = Some(alloc_error); + return false; + } + true + } + + fn dependency(&self) -> Dependency { + let mut parent = None; + + // TODO(emilio): Maybe we should refcount the parent dependencies, or + // cache them or something. + for &(ref selector, ref selector_offset) in self.parent_selectors.iter() { + let new_parent = Dependency { + selector: selector.clone(), + selector_offset: *selector_offset, + parent, + }; + parent = Some(Box::new(new_parent)); } - Ok(()) + Dependency { + selector: self.selector.clone(), + selector_offset: self.compound_state.offset, + parent, + } } } -/// A struct that collects invalidations for a given compound selector. -/// -/// FIXME(emilio, bug 1509418): :where is mishandled, figure out the right way -/// to do invalidation for :where when combinators are inside. -/// -/// Simplest feasible idea seems to be: For each :where branch, if there are -/// combinators in the branch, treat as its own selector (basically, call -/// `.note_selector` with the nested selector). That may over-invalidate, but -/// not too much. If there are no combinators, then behave like we do today and -/// use the outer selector as a whole. If we care a lot about that potential -/// over-invalidation where combinators are present then we need more complex -/// data-structures in `Dependency`. -struct CompoundSelectorDependencyCollector<'a> { - /// The state this compound selector is affected by. - state: ElementState, - - /// The document this _complex_ selector is affected by. - /// - /// We don't need to track state per compound selector, since it's global - /// state and it changes for everything. - document_state: &'a mut DocumentState, - - /// The classes this compound selector is affected by. - /// - /// NB: This will be often a single class, but could be multiple in - /// presence of :not, :-moz-any, .foo.bar.baz, etc. - classes: SmallVec<[Atom; 5]>, - - /// The IDs this compound selector is affected by. - /// - /// NB: This will be almost always a single id, but could be multiple in - /// presence of :not, :-moz-any, #foo#bar, etc. - ids: SmallVec<[Atom; 5]>, - - /// Whether it affects other attribute-dependent selectors that aren't ID or - /// class selectors (NB: We still set this to true in presence of [class] or - /// [id] attribute selectors). - other_attributes: bool, - - /// The invalidation map flags, that we set when some attribute selectors are present. - flags: &'a mut InvalidationMapFlags, -} - -impl<'a> SelectorVisitor for CompoundSelectorDependencyCollector<'a> { +impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { type Impl = SelectorImpl; + fn visit_selector_list(&mut self, list: &[Selector]) -> bool { + self.parent_selectors.push((self.selector.clone(), self.compound_state.offset)); + for selector in list { + let mut nested = SelectorDependencyCollector { + map: &mut *self.map, + document_state: &mut *self.document_state, + selector, + parent_selectors: &mut *self.parent_selectors, + quirks_mode: self.quirks_mode, + compound_state: PerCompoundState::new(0), + alloc_error: &mut *self.alloc_error, + }; + if !nested.visit_whole_selector() { + return false; + } + } + self.parent_selectors.pop(); + true + } + fn visit_simple_selector(&mut self, s: &Component) -> bool { #[cfg(feature = "gecko")] use crate::selector_parser::NonTSPseudoClass; match *s { - Component::ID(ref id) => { - self.ids.push(id.clone()); - }, - Component::Class(ref class) => { - self.classes.push(class.clone()); + Component::ID(ref atom) | Component::Class(ref atom) => { + let dependency = self.dependency(); + let map = match *s { + Component::ID(..) => &mut self.map.id_to_selector, + Component::Class(..) => &mut self.map.class_to_selector, + _ => unreachable!(), + }; + let entry = match map.try_entry(atom.clone(), self.quirks_mode) { + Ok(entry) => entry, + Err(err) => { + *self.alloc_error = Some(err); + return false; + }, + }; + entry.or_insert_with(SmallVec::new).try_push(dependency).is_ok() }, Component::NonTSPseudoClass(ref pc) => { - self.other_attributes |= pc.is_attr_based(); - self.state |= match *pc { + self.compound_state.element_state |= match *pc { #[cfg(feature = "gecko")] NonTSPseudoClass::Dir(ref dir) => dir.element_state(), _ => pc.state_flag(), }; *self.document_state |= pc.document_state_flag(); - }, - _ => {}, - } - true + if !self.compound_state.added_attr_dependency && pc.is_attr_based() { + self.add_attr_dependency() + } else { + true + } + }, + _ => true, + } } fn visit_attribute_selector( @@ -399,7 +477,6 @@ impl<'a> SelectorVisitor for CompoundSelectorDependencyCollector<'a> { _local_name: &LocalName, local_name_lower: &LocalName, ) -> bool { - self.other_attributes = true; let may_match_in_no_namespace = match *constraint { NamespaceConstraint::Any => true, NamespaceConstraint::Specific(ref ns) => ns.is_empty(), @@ -407,14 +484,16 @@ impl<'a> SelectorVisitor for CompoundSelectorDependencyCollector<'a> { if may_match_in_no_namespace { if *local_name_lower == local_name!("id") { - self.flags - .insert(InvalidationMapFlags::HAS_ID_ATTR_SELECTOR) + self.map.flags.insert(InvalidationMapFlags::HAS_ID_ATTR_SELECTOR) } else if *local_name_lower == local_name!("class") { - self.flags - .insert(InvalidationMapFlags::HAS_CLASS_ATTR_SELECTOR) + self.map.flags.insert(InvalidationMapFlags::HAS_CLASS_ATTR_SELECTOR) } } - true + if !self.compound_state.added_attr_dependency { + self.add_attr_dependency() + } else { + true + } } } diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index e906f8d8405..f6fa933e44e 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -79,6 +79,39 @@ impl<'a, 'b: 'a, E: TElement + 'b> StateAndAttrInvalidationProcessor<'a, 'b, E> } } +/// Checks a dependency against a given element and wrapper, to see if something +/// changed. +pub fn check_dependency( + dependency: &Dependency, + element: &E, + wrapper: &W, + context: &mut MatchingContext<'_, SelectorImpl>, +) +where + E: TElement, + W: selectors::Element, +{ + let matches_now = matches_selector( + &dependency.selector, + dependency.selector_offset, + None, + element, + &mut context, + &mut |_, _| {}, + ); + + let matched_then = matches_selector( + &dependency.selector, + dependency.selector_offset, + None, + wrapper, + &mut context, + &mut |_, _| {}, + ); + + matched_then != matches_now +} + /// Whether we should process the descendants of a given element for style /// invalidation. pub fn should_process_descendants(data: &ElementData) -> bool { @@ -412,28 +445,9 @@ where } /// Check whether a dependency should be taken into account. + #[inline] fn check_dependency(&mut self, dependency: &Dependency) -> bool { - let element = &self.element; - let wrapper = &self.wrapper; - let matches_now = matches_selector( - &dependency.selector, - dependency.selector_offset, - None, - element, - &mut self.matching_context, - &mut |_, _| {}, - ); - - let matched_then = matches_selector( - &dependency.selector, - dependency.selector_offset, - None, - wrapper, - &mut self.matching_context, - &mut |_, _| {}, - ); - - matched_then != matches_now + check_dependency(&self.element, &self.wrapper, &mut self.matching_context) } fn scan_dependency(&mut self, dependency: &'selectors Dependency) { @@ -456,7 +470,13 @@ where let invalidation_kind = dependency.invalidation_kind(); if matches!(invalidation_kind, DependencyInvalidationKind::Element) { - self.invalidates_self = true; + if let Some(ref parent) = dependency.parent { + // We know something changed in the inner selector, go outwards + // now. + self.scan_dependency(parent); + } else { + self.invalidates_self = true; + } return; } From 4b5de772c64fee498347962611d4f28a746263a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 23 Apr 2020 19:20:10 +0000 Subject: [PATCH 03/42] style: Make Invalidation work in terms of a dependency, not a selector. That way we can look at the parent dependency as described in the previous patch. An alternative would be to add a: parent_dependency: Option<&'a Dependency> on construction to `Invalidation`, but this way seems slightly better to avoid growing the struct. It's not even one more indirection because the selector is contained directly in the Dependency struct. Differential Revision: https://phabricator.services.mozilla.com/D71422 --- components/style/dom_apis.rs | 18 +- .../invalidation/element/document_state.rs | 14 +- .../invalidation/element/invalidation_map.rs | 26 +- .../style/invalidation/element/invalidator.rs | 442 ++++++++++-------- .../element/state_and_attributes.rs | 17 +- 5 files changed, 315 insertions(+), 202 deletions(-) diff --git a/components/style/dom_apis.rs b/components/style/dom_apis.rs index 8f764dc8958..685b5b4e8a0 100644 --- a/components/style/dom_apis.rs +++ b/components/style/dom_apis.rs @@ -9,6 +9,7 @@ use crate::context::QuirksMode; use crate::dom::{TDocument, TElement, TNode, TShadowRoot}; use crate::invalidation::element::invalidator::{DescendantInvalidationLists, Invalidation}; use crate::invalidation::element::invalidator::{InvalidationProcessor, InvalidationVector}; +use crate::invalidation::element::invalidation_map::Dependency; use crate::Atom; use selectors::attr::CaseSensitivity; use selectors::matching::{self, MatchingContext, MatchingMode}; @@ -130,7 +131,7 @@ where { results: &'a mut Q::Output, matching_context: MatchingContext<'a, E::Impl>, - selector_list: &'a SelectorList, + dependencies: &'a [Dependency], } impl<'a, E, Q> InvalidationProcessor<'a, E> for QuerySelectorProcessor<'a, E, Q> @@ -143,6 +144,11 @@ where true } + fn check_outer_dependency(&mut self, _: &Dependency, _: E) -> bool { + debug_assert!(false, "How? We should only have parent-less dependencies here!"); + true + } + fn collect_invalidations( &mut self, element: E, @@ -171,11 +177,10 @@ where self_invalidations }; - for selector in self.selector_list.0.iter() { + for dependency in self.dependencies.iter() { target_vector.push(Invalidation::new( - selector, + dependency, self.matching_context.current_host.clone(), - 0, )) } @@ -642,10 +647,13 @@ pub fn query_selector( if root_element.is_some() || !invalidation_may_be_useful { query_selector_slow::(root, selector_list, results, &mut matching_context); } else { + let dependencies = selector_list.0.iter().map(|selector| { + Dependency::for_full_selector_invalidation(selector.clone()) + }).collect::>(); let mut processor = QuerySelectorProcessor:: { results, matching_context, - selector_list, + dependencies: &dependencies, }; for node in root.dom_children() { diff --git a/components/style/invalidation/element/document_state.rs b/components/style/invalidation/element/document_state.rs index 7fc51338b1e..4659eb42f5e 100644 --- a/components/style/invalidation/element/document_state.rs +++ b/components/style/invalidation/element/document_state.rs @@ -6,6 +6,7 @@ use crate::dom::TElement; use crate::element_state::DocumentState; +use crate::invalidation::element::invalidation_map::Dependency; use crate::invalidation::element::invalidator::{DescendantInvalidationLists, InvalidationVector}; use crate::invalidation::element::invalidator::{Invalidation, InvalidationProcessor}; use crate::invalidation::element::state_and_attributes; @@ -65,6 +66,11 @@ where E: TElement, I: Iterator, { + fn check_outer_dependency(&mut self, _: &Dependency, _: E) -> bool { + debug_assert!(false, "how, we should only have parent-less dependencies here!"); + true + } + fn collect_invalidations( &mut self, _element: E, @@ -81,10 +87,14 @@ where // We pass `None` as a scope, as document state selectors aren't // affected by the current scope. + // + // FIXME(emilio): We should really pass the relevant host for + // self.rules, so that we invalidate correctly if the selector + // happens to have something like :host(:-moz-window-inactive) + // for example. self_invalidations.push(Invalidation::new( - &dependency.selector, + &dependency.dependency, /* scope = */ None, - 0, )); } } diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index 3dd5134ec34..031407adce1 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -57,7 +57,7 @@ pub struct Dependency { /// /// We'd generate: /// - /// * One dependency for .quz (offset: 0, parent: None) + /// * One dependency for .qux (offset: 0, parent: None) /// * One dependency for .baz pointing to B with parent being a /// dependency pointing to C. /// * One dependency from .bar pointing to C (parent: None) @@ -88,6 +88,22 @@ pub enum DependencyInvalidationKind { } impl Dependency { + /// Creates a dummy dependency to invalidate the whole selector. + /// + /// This is necessary because document state invalidation wants to + /// invalidate all elements in the document. + /// + /// The offset is such as that Invalidation::new(self) returns a zero + /// offset. That is, it points to a virtual "combinator" outside of the + /// selector, so calling combinator() on such a dependency will panic. + pub fn for_full_selector_invalidation(selector: Selector) -> Self { + Self { + selector_offset: selector.len() + 1, + selector, + parent: None, + } + } + /// Returns the combinator to the right of the partial selector this /// dependency represents. /// @@ -147,14 +163,14 @@ impl SelectorMapEntry for StateDependency { /// The same, but for document state selectors. #[derive(Clone, Debug, MallocSizeOf)] pub struct DocumentStateDependency { - /// The selector that is affected. We don't need to track an offset, since - /// when it changes it changes for the whole document anyway. + /// We track `Dependency` even though we don't need to track an offset, + /// since when it changes it changes for the whole document anyway. #[cfg_attr( feature = "gecko", ignore_malloc_size_of = "CssRules have primary refs, we measure there" )] #[cfg_attr(feature = "servo", ignore_malloc_size_of = "Arc")] - pub selector: Selector, + pub dependency: Dependency, /// The state this dependency is affected by. pub state: DocumentState, } @@ -269,7 +285,7 @@ impl InvalidationMap { if !document_state.is_empty() { let dep = DocumentStateDependency { state: document_state, - selector: selector.clone(), + dependency: Dependency::for_full_selector_invalidation(selector.clone()), }; self.document_state_selectors.try_push(dep)?; } diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index f0f55595dff..4f5d005c120 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -7,10 +7,10 @@ use crate::context::StackLimitChecker; use crate::dom::{TElement, TNode, TShadowRoot}; -use crate::selector_parser::SelectorImpl; +use crate::invalidation::element::invalidation_map::{Dependency, DependencyInvalidationKind}; use selectors::matching::matches_compound_selector_from; use selectors::matching::{CompoundSelectorMatchingResult, MatchingContext}; -use selectors::parser::{Combinator, Component, Selector}; +use selectors::parser::{Combinator, Component}; use selectors::OpaqueElement; use smallvec::SmallVec; use std::fmt; @@ -34,6 +34,27 @@ where false } + /// When a dependency from a :where or :is selector matches, it may still be + /// the case that we don't need to invalidate the full style. Consider the + /// case of: + /// + /// div .foo:where(.bar *, .baz) .qux + /// + /// We can get to the `*` part after a .bar class change, but you only need + /// to restyle the element if it also matches .foo. + /// + /// Similarly, you only need to restyle .baz if the whole result of matching + /// the selector changes. + /// + /// This function is called to check the result of matching the "outer" + /// dependency that we generate for the parent of the `:where` selector, + /// that is, in the case above it should match + /// `div .foo:where(.bar *, .baz)`. + /// + /// Returning true unconditionally here is over-optimistic and may + /// over-invalidate. + fn check_outer_dependency(&mut self, dependency: &Dependency, element: E) -> bool; + /// The matching context that should be used to process invalidations. fn matching_context(&mut self) -> &mut MatchingContext<'a, E::Impl>; @@ -127,7 +148,11 @@ enum InvalidationKind { /// relative to a current element we are processing, must be restyled. #[derive(Clone)] pub struct Invalidation<'a> { - selector: &'a Selector, + /// The dependency that generated this invalidation. + /// + /// Note that the offset inside the dependency is not really useful after + /// construction. + dependency: &'a Dependency, /// The right shadow host from where the rule came from, if any. /// /// This is needed to ensure that we match the selector with the right @@ -138,6 +163,8 @@ pub struct Invalidation<'a> { /// /// This order is a "parse order" offset, that is, zero is the leftmost part /// of the selector written as parsed / serialized. + /// + /// It is initialized from the offset from `dependency`. offset: usize, /// Whether the invalidation was already matched by any previous sibling or /// ancestor. @@ -149,16 +176,21 @@ pub struct Invalidation<'a> { } impl<'a> Invalidation<'a> { - /// Create a new invalidation for a given selector and offset. + /// Create a new invalidation for matching a dependency. pub fn new( - selector: &'a Selector, + dependency: &'a Dependency, scope: Option, - offset: usize, ) -> Self { + debug_assert!( + dependency.selector_offset == dependency.selector.len() + 1 || + dependency.invalidation_kind() != DependencyInvalidationKind::Element, + "No point to this, if the dependency matched the element we should just invalidate it" + ); Self { - selector, + dependency, scope, - offset, + // + 1 to go past the combinator. + offset: dependency.selector.len() + 1 - dependency.selector_offset, matched_by_any_previous: false, } } @@ -174,7 +206,7 @@ impl<'a> Invalidation<'a> { // for the weird pseudos in . // // We should be able to do better here! - match self.selector.combinator_at_parse_order(self.offset - 1) { + match self.dependency.selector.combinator_at_parse_order(self.offset - 1) { Combinator::Descendant | Combinator::LaterSibling | Combinator::PseudoElement => true, Combinator::Part | Combinator::SlotAssignment | @@ -188,7 +220,7 @@ impl<'a> Invalidation<'a> { return InvalidationKind::Descendant(DescendantInvalidationKind::Dom); } - match self.selector.combinator_at_parse_order(self.offset - 1) { + match self.dependency.selector.combinator_at_parse_order(self.offset - 1) { Combinator::Child | Combinator::Descendant | Combinator::PseudoElement => { InvalidationKind::Descendant(DescendantInvalidationKind::Dom) }, @@ -206,7 +238,7 @@ impl<'a> fmt::Debug for Invalidation<'a> { use cssparser::ToCss; f.write_str("Invalidation(")?; - for component in self.selector.iter_raw_parse_order_from(self.offset) { + for component in self.dependency.selector.iter_raw_parse_order_from(self.offset) { if matches!(*component, Component::Combinator(..)) { break; } @@ -763,201 +795,241 @@ where context.current_host = invalidation.scope; matches_compound_selector_from( - &invalidation.selector, + &invalidation.dependency.selector, invalidation.offset, context, &self.element, ) }; - let mut invalidated_self = false; - let mut matched = false; - match matching_result { + let next_invalidation = match matching_result { + CompoundSelectorMatchingResult::NotMatched => { + return SingleInvalidationResult { + invalidated_self: false, + matched: false, + } + }, CompoundSelectorMatchingResult::FullyMatched => { debug!(" > Invalidation matched completely"); - matched = true; - invalidated_self = true; + // We matched completely. If we're an inner selector now we need + // to go outside our selector and carry on invalidating. + let mut cur_dependency = invalidation.dependency; + loop { + cur_dependency = match cur_dependency.parent { + None => return SingleInvalidationResult { + invalidated_self: true, + matched: true, + }, + Some(ref p) => &**p, + }; + + debug!(" > Checking outer dependency {:?}", cur_dependency); + + // The inner selector changed, now check if the full + // previous part of the selector did, before keeping + // checking for descendants. + if !self.processor.check_outer_dependency(cur_dependency, self.element) { + return SingleInvalidationResult { + invalidated_self: false, + matched: false, + } + } + + if cur_dependency.invalidation_kind() == DependencyInvalidationKind::Element { + continue; + } + + debug!(" > Generating invalidation"); + break Invalidation::new(cur_dependency, invalidation.scope) + } }, CompoundSelectorMatchingResult::Matched { next_combinator_offset, } => { - let next_combinator = invalidation - .selector - .combinator_at_parse_order(next_combinator_offset); - matched = true; - - if matches!(next_combinator, Combinator::PseudoElement) { - // This will usually be the very next component, except for - // the fact that we store compound selectors the other way - // around, so there could also be state pseudo-classes. - let pseudo_selector = invalidation - .selector - .iter_raw_parse_order_from(next_combinator_offset + 1) - .skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..))) - .next() - .unwrap(); - - let pseudo = match *pseudo_selector { - Component::PseudoElement(ref pseudo) => pseudo, - _ => unreachable!( - "Someone seriously messed up selector parsing: \ - {:?} at offset {:?}: {:?}", - invalidation.selector, next_combinator_offset, pseudo_selector, - ), - }; - - // FIXME(emilio): This is not ideal, and could not be - // accurate if we ever have stateful element-backed eager - // pseudos. - // - // Ideally, we'd just remove element-backed eager pseudos - // altogether, given they work fine without it. Only gotcha - // is that we wouldn't style them in parallel, which may or - // may not be an issue. - // - // Also, this could be more fine grained now (perhaps a - // RESTYLE_PSEUDOS hint?). - // - // Note that we'll also restyle the pseudo-element because - // it would match this invalidation. - if self.processor.invalidates_on_eager_pseudo_element() { - if pseudo.is_eager() { - invalidated_self = true; - } - // If we start or stop matching some marker rules, and - // don't have a marker, then we need to restyle the - // element to potentially create one. - // - // Same caveats as for other eager pseudos apply, this - // could be more fine-grained. - if pseudo.is_marker() && self.element.marker_pseudo_element().is_none() { - invalidated_self = true; - } - - // FIXME: ::selection doesn't generate elements, so the - // regular invalidation doesn't work for it. We store - // the cached selection style holding off the originating - // element, so we need to restyle it in order to invalidate - // it. This is still not quite correct, since nothing - // triggers a repaint necessarily, but matches old Gecko - // behavior, and the ::selection implementation needs to - // change significantly anyway to implement - // https://github.com/w3c/csswg-drafts/issues/2474. - if pseudo.is_selection() { - invalidated_self = true; - } - } - } - - let next_invalidation = Invalidation { - selector: invalidation.selector, + Invalidation { + dependency: invalidation.dependency, scope: invalidation.scope, offset: next_combinator_offset + 1, matched_by_any_previous: false, - }; - - debug!( - " > Invalidation matched, next: {:?}, ({:?})", - next_invalidation, next_combinator - ); - - let next_invalidation_kind = next_invalidation.kind(); - - // We can skip pushing under some circumstances, and we should - // because otherwise the invalidation list could grow - // exponentially. - // - // * First of all, both invalidations need to be of the same - // kind. This is because of how we propagate them going to - // the right of the tree for sibling invalidations and going - // down the tree for children invalidations. A sibling - // invalidation that ends up generating a children - // invalidation ends up (correctly) in five different lists, - // not in the same list five different times. - // - // * Then, the invalidation needs to be matched by a previous - // ancestor/sibling, in order to know that this invalidation - // has been generated already. - // - // * Finally, the new invalidation needs to be - // `effective_for_next()`, in order for us to know that it is - // still in the list, since we remove the dependencies that - // aren't from the lists for our children / siblings. - // - // To go through an example, let's imagine we are processing a - // dom subtree like: - // - //
- // - // And an invalidation list with a single invalidation like: - // - // [div div div] - // - // When we process the invalidation list for the outer div, we - // match it, and generate a `div div` invalidation, so for the - //
child we have: - // - // [div div div, div div] - // - // With the first of them marked as `matched`. - // - // When we process the
child, we don't match any of - // them, so both invalidations go untouched to our children. - // - // When we process the second
, we match _both_ - // invalidations. - // - // However, when matching the first, we can tell it's been - // matched, and not push the corresponding `div div` - // invalidation, since we know it's necessarily already on the - // list. - // - // Thus, without skipping the push, we'll arrive to the - // innermost
with: - // - // [div div div, div div, div div, div] - // - // While skipping it, we won't arrive here with duplicating - // dependencies: - // - // [div div div, div div, div] - // - let can_skip_pushing = next_invalidation_kind == invalidation_kind && - invalidation.matched_by_any_previous && - next_invalidation.effective_for_next(); - - if can_skip_pushing { - debug!( - " > Can avoid push, since the invalidation had \ - already been matched before" - ); - } else { - match next_invalidation_kind { - InvalidationKind::Descendant(DescendantInvalidationKind::Dom) => { - descendant_invalidations - .dom_descendants - .push(next_invalidation); - }, - InvalidationKind::Descendant(DescendantInvalidationKind::Part) => { - descendant_invalidations.parts.push(next_invalidation); - }, - InvalidationKind::Descendant(DescendantInvalidationKind::Slotted) => { - descendant_invalidations - .slotted_descendants - .push(next_invalidation); - }, - InvalidationKind::Sibling => { - sibling_invalidations.push(next_invalidation); - }, - } } }, - CompoundSelectorMatchingResult::NotMatched => {}, + }; + + debug_assert_ne!( + next_invalidation.offset, + 0, + "Rightmost selectors shouldn't generate more invalidations", + ); + + let mut invalidated_self = false; + let next_combinator = next_invalidation + .dependency + .selector + .combinator_at_parse_order(next_invalidation.offset - 1); + + if matches!(next_combinator, Combinator::PseudoElement) { + // This will usually be the very next component, except for + // the fact that we store compound selectors the other way + // around, so there could also be state pseudo-classes. + let pseudo_selector = next_invalidation + .dependency + .selector + .iter_raw_parse_order_from(next_invalidation.offset) + .skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..))) + .next() + .unwrap(); + + let pseudo = match *pseudo_selector { + Component::PseudoElement(ref pseudo) => pseudo, + _ => unreachable!( + "Someone seriously messed up selector parsing: \ + {:?} at offset {:?}: {:?}", + next_invalidation.dependency, next_invalidation.offset, pseudo_selector, + ), + }; + + // FIXME(emilio): This is not ideal, and could not be + // accurate if we ever have stateful element-backed eager + // pseudos. + // + // Ideally, we'd just remove element-backed eager pseudos + // altogether, given they work fine without it. Only gotcha + // is that we wouldn't style them in parallel, which may or + // may not be an issue. + // + // Also, this could be more fine grained now (perhaps a + // RESTYLE_PSEUDOS hint?). + // + // Note that we'll also restyle the pseudo-element because + // it would match this invalidation. + if self.processor.invalidates_on_eager_pseudo_element() { + if pseudo.is_eager() { + invalidated_self = true; + } + // If we start or stop matching some marker rules, and + // don't have a marker, then we need to restyle the + // element to potentially create one. + // + // Same caveats as for other eager pseudos apply, this + // could be more fine-grained. + if pseudo.is_marker() && self.element.marker_pseudo_element().is_none() { + invalidated_self = true; + } + + // FIXME: ::selection doesn't generate elements, so the + // regular invalidation doesn't work for it. We store + // the cached selection style holding off the originating + // element, so we need to restyle it in order to invalidate + // it. This is still not quite correct, since nothing + // triggers a repaint necessarily, but matches old Gecko + // behavior, and the ::selection implementation needs to + // change significantly anyway to implement + // https://github.com/w3c/csswg-drafts/issues/2474. + if pseudo.is_selection() { + invalidated_self = true; + } + } + } + + debug!( + " > Invalidation matched, next: {:?}, ({:?})", + next_invalidation, next_combinator + ); + + let next_invalidation_kind = next_invalidation.kind(); + + // We can skip pushing under some circumstances, and we should + // because otherwise the invalidation list could grow + // exponentially. + // + // * First of all, both invalidations need to be of the same + // kind. This is because of how we propagate them going to + // the right of the tree for sibling invalidations and going + // down the tree for children invalidations. A sibling + // invalidation that ends up generating a children + // invalidation ends up (correctly) in five different lists, + // not in the same list five different times. + // + // * Then, the invalidation needs to be matched by a previous + // ancestor/sibling, in order to know that this invalidation + // has been generated already. + // + // * Finally, the new invalidation needs to be + // `effective_for_next()`, in order for us to know that it is + // still in the list, since we remove the dependencies that + // aren't from the lists for our children / siblings. + // + // To go through an example, let's imagine we are processing a + // dom subtree like: + // + //
+ // + // And an invalidation list with a single invalidation like: + // + // [div div div] + // + // When we process the invalidation list for the outer div, we + // match it, and generate a `div div` invalidation, so for the + //
child we have: + // + // [div div div, div div] + // + // With the first of them marked as `matched`. + // + // When we process the
child, we don't match any of + // them, so both invalidations go untouched to our children. + // + // When we process the second
, we match _both_ + // invalidations. + // + // However, when matching the first, we can tell it's been + // matched, and not push the corresponding `div div` + // invalidation, since we know it's necessarily already on the + // list. + // + // Thus, without skipping the push, we'll arrive to the + // innermost
with: + // + // [div div div, div div, div div, div] + // + // While skipping it, we won't arrive here with duplicating + // dependencies: + // + // [div div div, div div, div] + // + let can_skip_pushing = next_invalidation_kind == invalidation_kind && + invalidation.matched_by_any_previous && + next_invalidation.effective_for_next(); + + if can_skip_pushing { + debug!( + " > Can avoid push, since the invalidation had \ + already been matched before" + ); + } else { + match next_invalidation_kind { + InvalidationKind::Descendant(DescendantInvalidationKind::Dom) => { + descendant_invalidations + .dom_descendants + .push(next_invalidation); + }, + InvalidationKind::Descendant(DescendantInvalidationKind::Part) => { + descendant_invalidations.parts.push(next_invalidation); + }, + InvalidationKind::Descendant(DescendantInvalidationKind::Slotted) => { + descendant_invalidations + .slotted_descendants + .push(next_invalidation); + }, + InvalidationKind::Sibling => { + sibling_invalidations.push(next_invalidation); + }, + } } SingleInvalidationResult { invalidated_self, - matched, + matched: true, } } } diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index f6fa933e44e..828bc99ed20 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -85,8 +85,8 @@ pub fn check_dependency( dependency: &Dependency, element: &E, wrapper: &W, - context: &mut MatchingContext<'_, SelectorImpl>, -) + mut context: &mut MatchingContext<'_, E::Impl>, +) -> bool where E: TElement, W: selectors::Element, @@ -166,6 +166,14 @@ where true } + fn check_outer_dependency(&mut self, dependency: &Dependency, element: E) -> bool { + // We cannot assert about `element` having a snapshot here (in fact it + // most likely won't), because it may be an arbitrary descendant or + // later-sibling of the element we started invalidating with. + let wrapper = ElementWrapper::new(element, &*self.shared_context.snapshot_map); + check_dependency(dependency, &element, &wrapper, &mut self.matching_context) + } + fn matching_context(&mut self) -> &mut MatchingContext<'a, E::Impl> { &mut self.matching_context } @@ -447,7 +455,7 @@ where /// Check whether a dependency should be taken into account. #[inline] fn check_dependency(&mut self, dependency: &Dependency) -> bool { - check_dependency(&self.element, &self.wrapper, &mut self.matching_context) + check_dependency(dependency, &self.element, &self.wrapper, &mut self.matching_context) } fn scan_dependency(&mut self, dependency: &'selectors Dependency) { @@ -484,9 +492,8 @@ where debug_assert_ne!(dependency.selector_offset, dependency.selector.len()); let invalidation = Invalidation::new( - &dependency.selector, + &dependency, self.matching_context.current_host.clone(), - dependency.selector.len() - dependency.selector_offset + 1, ); match invalidation_kind { From 2b7fb519ba200ebc471aa5ab3dad3c6c48304386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 23 Apr 2020 19:20:17 +0000 Subject: [PATCH 04/42] style: Optimize invalidation by scanning the rightmost compound inside :where() and :is() with the outer visitor. See the comment about why this is valuable. For a selector like: .foo:is(.bar) > .baz Before this patch we'd generate an Dependency for .bar like this: Dependency { selector: .bar, offset: 0, parent: Some(Dependency { selector: .foo:is(.bar) > .baz, offset: 1, // Pointing to the `>` combinator. parent: None, }), } After this patch we'd generate just: Dependency { selector: .foo:is(.bar) > .baz, offset: 1, // Pointing to the `>` combinator. parent: None, } This is not only less memory but also less work. The reason for that is that, before this patch, when .bar changes, we'd look the dependency, and see there's a parent, and then scan that, so we'd match `.bar` two times, one for the initial dependency, and one for .foo:is(.bar). Instead, with this we'd only check `.foo:is(.bar)` once. Differential Revision: https://phabricator.services.mozilla.com/D71423 --- .../invalidation/element/invalidation_map.rs | 55 ++++++++++++++----- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index 031407adce1..b760d865aa2 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -347,17 +347,20 @@ struct SelectorDependencyCollector<'a> { impl<'a> SelectorDependencyCollector<'a> { fn visit_whole_selector(&mut self) -> bool { - let mut iter = self.selector.iter(); - let mut combinator; - let mut index = 0; + let iter = self.selector.iter(); + self.visit_whole_selector_from(iter, 0) + } + fn visit_whole_selector_from(&mut self, mut iter: SelectorIter, mut index: usize) -> bool { loop { // Reset the compound state. self.compound_state = PerCompoundState::new(index); // Visit all the simple selectors in this sequence. for ss in &mut iter { - ss.visit(self); + if !ss.visit(self) { + return false; + } index += 1; // Account for the simple selector. } @@ -376,15 +379,12 @@ impl<'a> SelectorDependencyCollector<'a> { } } - combinator = iter.next_sequence(); + let combinator = iter.next_sequence(); if combinator.is_none() { - break; + return true; } - - index += 1; // Account for the combinator. + index += 1; // account for the combinator } - - true } fn add_attr_dependency(&mut self) -> bool { @@ -409,6 +409,11 @@ impl<'a> SelectorDependencyCollector<'a> { // TODO(emilio): Maybe we should refcount the parent dependencies, or // cache them or something. for &(ref selector, ref selector_offset) in self.parent_selectors.iter() { + debug_assert_ne!( + self.compound_state.offset, + 0, + "Shouldn't bother creating nested dependencies for the rightmost compound", + ); let new_parent = Dependency { selector: selector.clone(), selector_offset: *selector_offset, @@ -429,22 +434,44 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { type Impl = SelectorImpl; fn visit_selector_list(&mut self, list: &[Selector]) -> bool { - self.parent_selectors.push((self.selector.clone(), self.compound_state.offset)); for selector in list { + // Here we cheat a bit: We can visit the rightmost compound with + // the "outer" visitor, and it'd be fine. This reduces the amount of + // state and attribute invalidations, and we need to check the outer + // selector to the left anyway to avoid over-invalidation, so it + // avoids matching it twice uselessly. + let mut iter = selector.iter(); + let mut index = 0; + + for ss in &mut iter { + if !ss.visit(self) { + return false; + } + index += 1; + } + + let combinator = iter.next_sequence(); + if combinator.is_none() { + continue; + } + + index += 1; // account for the combinator. + + self.parent_selectors.push((self.selector.clone(), self.compound_state.offset)); let mut nested = SelectorDependencyCollector { map: &mut *self.map, document_state: &mut *self.document_state, selector, parent_selectors: &mut *self.parent_selectors, quirks_mode: self.quirks_mode, - compound_state: PerCompoundState::new(0), + compound_state: PerCompoundState::new(index), alloc_error: &mut *self.alloc_error, }; - if !nested.visit_whole_selector() { + if !nested.visit_whole_selector_from(iter, index) { return false; } + self.parent_selectors.pop(); } - self.parent_selectors.pop(); true } From 554fc4c6feaf636b231accbd8cc6fc66dbb88711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 23 Apr 2020 19:20:27 +0000 Subject: [PATCH 05/42] style: Handle disjoint selectors in the selector map. This way, something like: *:where(.foo, .bar) Will end up twice on the selector map, just as if you would've written .foo, .bar. But we're a bit careful to not be wasteful, so: .foo:where(div, span) Will still end up using the .foo bucket. It needs a bit of borrow-checker gymnastics to avoid cloning the entry in the common path. It's a bit gross but not too terrible I think. Differential Revision: https://phabricator.services.mozilla.com/D71457 --- components/style/selector_map.rs | 209 +++++++++++++++++++------------ 1 file changed, 127 insertions(+), 82 deletions(-) diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index a53aad65fb3..3c811e2e458 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -280,53 +280,89 @@ impl SelectorMap { impl SelectorMap { /// Inserts into the correct hash, trying id, class, localname and /// namespace. - pub fn insert( - &mut self, - entry: T, - quirks_mode: QuirksMode, - ) -> Result<(), FailedAllocationError> { + pub fn insert(&mut self, entry: T, quirks_mode: QuirksMode) -> Result<(), FailedAllocationError> { self.count += 1; - let vector = match find_bucket(entry.selector()) { - Bucket::Root => &mut self.root, - Bucket::ID(id) => self - .id_hash - .try_entry(id.clone(), quirks_mode)? - .or_insert_with(SmallVec::new), - Bucket::Class(class) => self - .class_hash - .try_entry(class.clone(), quirks_mode)? - .or_insert_with(SmallVec::new), - Bucket::LocalName { name, lower_name } => { - // If the local name in the selector isn't lowercase, insert it - // into the rule hash twice. This means that, during lookup, we - // can always find the rules based on the local name of the - // element, regardless of whether it's an html element in an - // html document (in which case we match against lower_name) or - // not (in which case we match against name). - // - // In the case of a non-html-element-in-html-document with a - // lowercase localname and a non-lowercase selector, the - // rulehash lookup may produce superfluous selectors, but the - // subsequent selector matching work will filter them out. - if name != lower_name { - self.local_name_hash - .try_entry(lower_name.clone())? - .or_insert_with(SmallVec::new) - .try_push(entry.clone())?; + // NOTE(emilio): It'd be nice for this to be a separate function, but + // then the compiler can't reason about the lifetime dependency between + // `entry` and `bucket`, and would force us to clone the rule in the + // common path. + macro_rules! insert_into_bucket { + ($entry:ident, $bucket:expr) => {{ + match $bucket { + Bucket::Root => &mut self.root, + Bucket::ID(id) => self + .id_hash + .try_entry(id.clone(), quirks_mode)? + .or_insert_with(SmallVec::new), + Bucket::Class(class) => self + .class_hash + .try_entry(class.clone(), quirks_mode)? + .or_insert_with(SmallVec::new), + Bucket::LocalName { name, lower_name } => { + // If the local name in the selector isn't lowercase, + // insert it into the rule hash twice. This means that, + // during lookup, we can always find the rules based on + // the local name of the element, regardless of whether + // it's an html element in an html document (in which + // case we match against lower_name) or not (in which + // case we match against name). + // + // In the case of a non-html-element-in-html-document + // with a lowercase localname and a non-lowercase + // selector, the rulehash lookup may produce superfluous + // selectors, but the subsequent selector matching work + // will filter them out. + if name != lower_name { + self.local_name_hash + .try_entry(lower_name.clone())? + .or_insert_with(SmallVec::new) + .try_push($entry.clone())?; + } + self.local_name_hash + .try_entry(name.clone())? + .or_insert_with(SmallVec::new) + }, + Bucket::Namespace(url) => self + .namespace_hash + .try_entry(url.clone())? + .or_insert_with(SmallVec::new), + Bucket::Universal => &mut self.other, + }.try_push($entry)?; + }} + } + + let bucket = { + let mut disjoint_buckets = SmallVec::new(); + let bucket = find_bucket(entry.selector(), &mut disjoint_buckets); + + // See if inserting this selector in multiple entries in the + // selector map would be worth it. Consider a case like: + // + // .foo:where(div, #bar) + // + // There, `bucket` would be `Class(foo)`, and disjoint_buckets would + // be `[LocalName { div }, ID(bar)]`. + // + // Here we choose to insert the selector in the `.foo` bucket in + // such a case, as it's likely more worth it than inserting it in + // both `div` and `#bar`. + // + // This is specially true if there's any universal selector in the + // `disjoint_selectors` set, at which point we'd just be doing + // wasted work. + if !disjoint_buckets.is_empty() && disjoint_buckets.iter().all(|b| b.more_specific_than(&bucket)) { + for bucket in &disjoint_buckets { + let entry = entry.clone(); + insert_into_bucket!(entry, *bucket); } - self.local_name_hash - .try_entry(name.clone())? - .or_insert_with(SmallVec::new) - }, - Bucket::Namespace(url) => self - .namespace_hash - .try_entry(url.clone())? - .or_insert_with(SmallVec::new), - Bucket::Universal => &mut self.other, + return Ok(()); + } + bucket }; - vector.try_push(entry) + insert_into_bucket!(entry, bucket); + Ok(()) } /// Looks up entries by id, class, local name, namespace, and other (in @@ -457,18 +493,43 @@ impl SelectorMap { } enum Bucket<'a> { - Root, - ID(&'a Atom), - Class(&'a Atom), + Universal, + Namespace(&'a Namespace), LocalName { name: &'a LocalName, lower_name: &'a LocalName, }, - Namespace(&'a Namespace), - Universal, + Class(&'a Atom), + ID(&'a Atom), + Root, } -fn specific_bucket_for<'a>(component: &'a Component) -> Bucket<'a> { +impl<'a> Bucket<'a> { + /// root > id > class > local name > namespace > universal. + #[inline] + fn specificity(&self) -> usize { + match *self { + Bucket::Universal => 0, + Bucket::Namespace(..) => 1, + Bucket::LocalName { .. } => 2, + Bucket::Class(..) => 3, + Bucket::ID(..) => 4, + Bucket::Root => 5, + } + } + + #[inline] + fn more_specific_than(&self, other: &Self) -> bool { + self.specificity() > other.specificity() + } +} + +type DisjointBuckets<'a> = SmallVec<[Bucket<'a>; 5]>; + +fn specific_bucket_for<'a>( + component: &'a Component, + disjoint_buckets: &mut DisjointBuckets<'a>, +) -> Bucket<'a> { match *component { Component::Root => Bucket::Root, Component::ID(ref id) => Bucket::ID(id), @@ -498,14 +559,16 @@ fn specific_bucket_for<'a>(component: &'a Component) -> Bucket<'a> // // So inserting `span` in the rule hash makes sense since we want to // match the slotted . - Component::Slotted(ref selector) => find_bucket(selector.iter()), - Component::Host(Some(ref selector)) => find_bucket(selector.iter()), + Component::Slotted(ref selector) => find_bucket(selector.iter(), disjoint_buckets), + Component::Host(Some(ref selector)) => find_bucket(selector.iter(), disjoint_buckets), Component::Is(ref list) | Component::Where(ref list) => { if list.len() == 1 { - find_bucket(list[0].iter()) + find_bucket(list[0].iter(), disjoint_buckets) } else { - // TODO(emilio): We should handle the multiple element case by - // inserting multiple entries in the map. + for selector in &**list { + let bucket = find_bucket(selector.iter(), disjoint_buckets); + disjoint_buckets.push(bucket); + } Bucket::Universal } }, @@ -515,39 +578,21 @@ fn specific_bucket_for<'a>(component: &'a Component) -> Bucket<'a> /// Searches a compound selector from left to right, and returns the appropriate /// bucket for it. +/// +/// It also populates disjoint_buckets with dependencies from nested selectors +/// with any semantics like :is() and :where(). #[inline(always)] -fn find_bucket<'a>(mut iter: SelectorIter<'a, SelectorImpl>) -> Bucket<'a> { +fn find_bucket<'a>( + mut iter: SelectorIter<'a, SelectorImpl>, + disjoint_buckets: &mut DisjointBuckets<'a>, +) -> Bucket<'a> { let mut current_bucket = Bucket::Universal; loop { - // We basically want to find the most specific bucket, - // where: - // - // root > id > class > local name > namespace > universal. - // for ss in &mut iter { - let new_bucket = specific_bucket_for(ss); - match new_bucket { - Bucket::Root => return new_bucket, - Bucket::ID(..) => { - current_bucket = new_bucket; - }, - Bucket::Class(..) => { - if !matches!(current_bucket, Bucket::ID(..)) { - current_bucket = new_bucket; - } - }, - Bucket::LocalName { .. } => { - if matches!(current_bucket, Bucket::Universal | Bucket::Namespace(..)) { - current_bucket = new_bucket; - } - }, - Bucket::Namespace(..) => { - if matches!(current_bucket, Bucket::Universal) { - current_bucket = new_bucket; - } - }, - Bucket::Universal => {}, + let new_bucket = specific_bucket_for(ss, disjoint_buckets); + if new_bucket.more_specific_than(¤t_bucket) { + current_bucket = new_bucket; } } @@ -558,7 +603,7 @@ fn find_bucket<'a>(mut iter: SelectorIter<'a, SelectorImpl>) -> Bucket<'a> { } } - return current_bucket; + current_bucket } /// Wrapper for PrecomputedHashMap that does ASCII-case-insensitive lookup in quirks mode. From 21d48e00cc9c268ded420991e14a98a53f2ced90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 23 Apr 2020 19:20:35 +0000 Subject: [PATCH 06/42] style: Collect ancestor hashes from single-length :is and :where selector lists. We can only collect hashes from single-length selectors, as described in the comment. Differential Revision: https://phabricator.services.mozilla.com/D71458 --- components/selectors/parser.rs | 121 ++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 56 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 0a3dd43ac3c..15440703033 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -404,18 +404,68 @@ pub struct AncestorHashes { pub packed_hashes: [u32; 3], } +fn collect_ancestor_hashes( + iter: SelectorIter, + quirks_mode: QuirksMode, + hashes: &mut [u32; 4], + len: &mut usize, +) -> bool +where + Impl::Identifier: PrecomputedHash, + Impl::ClassName: PrecomputedHash, + Impl::LocalName: PrecomputedHash, + Impl::NamespaceUrl: PrecomputedHash, +{ + for component in AncestorIter::new(iter) { + let hash = match *component { + Component::LocalName(LocalName { + ref name, + ref lower_name, + }) => { + // Only insert the local-name into the filter if it's all + // lowercase. Otherwise we would need to test both hashes, and + // our data structures aren't really set up for that. + if name != lower_name { + continue; + } + name.precomputed_hash() + }, + Component::DefaultNamespace(ref url) | Component::Namespace(_, ref url) => { + url.precomputed_hash() + }, + // In quirks mode, class and id selectors should match + // case-insensitively, so just avoid inserting them into the filter. + Component::ID(ref id) if quirks_mode != QuirksMode::Quirks => { + id.precomputed_hash() + }, + Component::Class(ref class) if quirks_mode != QuirksMode::Quirks => { + class.precomputed_hash() + }, + Component::Is(ref list) | Component::Where(ref list) => { + // :where and :is OR their selectors, so we can't put any hash + // in the filter if there's more than one selector, as that'd + // exclude elements that may match one of the other selectors. + if list.len() == 1 { + if !collect_ancestor_hashes(list[0].iter(), quirks_mode, hashes, len) { + return false; + } + } + continue; + } + _ => continue, + }; + + hashes[*len] = hash & BLOOM_HASH_MASK; + *len += 1; + if *len == hashes.len() { + return false; + } + } + true +} + impl AncestorHashes { pub fn new(selector: &Selector, quirks_mode: QuirksMode) -> Self - where - Impl::Identifier: PrecomputedHash, - Impl::ClassName: PrecomputedHash, - Impl::LocalName: PrecomputedHash, - Impl::NamespaceUrl: PrecomputedHash, - { - Self::from_iter(selector.iter(), quirks_mode) - } - - fn from_iter(iter: SelectorIter, quirks_mode: QuirksMode) -> Self where Impl::Identifier: PrecomputedHash, Impl::ClassName: PrecomputedHash, @@ -424,18 +474,14 @@ impl AncestorHashes { { // Compute ancestor hashes for the bloom filter. let mut hashes = [0u32; 4]; - let mut hash_iter = AncestorIter::new(iter).filter_map(|x| x.ancestor_hash(quirks_mode)); - for i in 0..4 { - hashes[i] = match hash_iter.next() { - Some(x) => x & BLOOM_HASH_MASK, - None => break, - } - } + let mut len = 0; + collect_ancestor_hashes(selector.iter(), quirks_mode, &mut hashes, &mut len); + debug_assert!(len <= 4); // Now, pack the fourth hash (if it exists) into the upper byte of each of // the other three hashes. - let fourth = hashes[3]; - if fourth != 0 { + if len == 4 { + let fourth = hashes[3]; hashes[0] |= (fourth & 0x000000ff) << 24; hashes[1] |= (fourth & 0x0000ff00) << 16; hashes[2] |= (fourth & 0x00ff0000) << 8; @@ -980,43 +1026,6 @@ pub enum Component { } impl Component { - /// Compute the ancestor hash to check against the bloom filter. - fn ancestor_hash(&self, quirks_mode: QuirksMode) -> Option - where - Impl::Identifier: PrecomputedHash, - Impl::ClassName: PrecomputedHash, - Impl::LocalName: PrecomputedHash, - Impl::NamespaceUrl: PrecomputedHash, - { - match *self { - Component::LocalName(LocalName { - ref name, - ref lower_name, - }) => { - // Only insert the local-name into the filter if it's all - // lowercase. Otherwise we would need to test both hashes, and - // our data structures aren't really set up for that. - if name == lower_name { - Some(name.precomputed_hash()) - } else { - None - } - }, - Component::DefaultNamespace(ref url) | Component::Namespace(_, ref url) => { - Some(url.precomputed_hash()) - }, - // In quirks mode, class and id selectors should match - // case-insensitively, so just avoid inserting them into the filter. - Component::ID(ref id) if quirks_mode != QuirksMode::Quirks => { - Some(id.precomputed_hash()) - }, - Component::Class(ref class) if quirks_mode != QuirksMode::Quirks => { - Some(class.precomputed_hash()) - }, - _ => None, - } - } - /// Returns true if this is a combinator. pub fn is_combinator(&self) -> bool { matches!(*self, Component::Combinator(_)) From 6f58c665899c23fe8b3965ec687cecfd2ee67aa0 Mon Sep 17 00:00:00 2001 From: Mats Palmgren Date: Tue, 28 Apr 2020 01:18:44 +0000 Subject: [PATCH 07/42] style: Implement style system support for Masonry layout. This implements support for this CSS Masonry layout proposal: https://github.com/w3c/csswg-drafts/issues/4650 I've intentionally left out a shorthand (place-tracks?) for now until we have a draft CSS spec for this. Differential Revision: https://phabricator.services.mozilla.com/D67061 --- components/style/properties/data.py | 1 + components/style/properties/gecko.mako.rs | 4 +- .../properties/longhands/position.mako.rs | 32 ++++ components/style/values/computed/align.rs | 2 +- components/style/values/computed/mod.rs | 4 +- components/style/values/computed/position.rs | 2 +- components/style/values/generics/grid.rs | 3 + components/style/values/specified/align.rs | 87 ++++++--- components/style/values/specified/grid.rs | 18 +- components/style/values/specified/mod.rs | 6 +- components/style/values/specified/position.rs | 166 ++++++++++++++++++ 11 files changed, 295 insertions(+), 30 deletions(-) diff --git a/components/style/properties/data.py b/components/style/properties/data.py index 472d1eb34dd..56ac30dd458 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -356,6 +356,7 @@ class Longhand(object): "JustifyItems", "JustifySelf", "LineBreak", + "MasonryAutoFlow", "MozForceBrokenImageIcon", "MozListReversed", "MozScriptLevel", diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index f3db987d8f9..545e8cc77b5 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -802,7 +802,8 @@ fn static_assert() { <% skip_position_longhands = " ".join(x.ident for x in SIDES) %> <%self:impl_trait style_struct_name="Position" - skip_longhands="${skip_position_longhands}"> + skip_longhands="${skip_position_longhands} + masonry-auto-flow"> % for side in SIDES: <% impl_split_style_coord(side.ident, "mOffset", side.index) %> % endfor @@ -811,6 +812,7 @@ fn static_assert() { self.gecko.mJustifyItems.computed = v; } + ${impl_simple_type_with_conversion("masonry_auto_flow", "mMasonryAutoFlow")} <% skip_outline_longhands = " ".join("outline-style outline-width".split() + diff --git a/components/style/properties/longhands/position.mako.rs b/components/style/properties/longhands/position.mako.rs index d9403be5864..04d735201d4 100644 --- a/components/style/properties/longhands/position.mako.rs +++ b/components/style/properties/longhands/position.mako.rs @@ -114,6 +114,17 @@ ${helpers.single_keyword( animation_value_type="discrete", servo_restyle_damage="reflow", )} + + ${helpers.predefined_type( + "justify-tracks", + "JustifyTracks", + "specified::JustifyTracks::default()", + engines="gecko", + gecko_pref="layout.css.grid-template-masonry-value.enabled", + animation_value_type="discrete", + servo_restyle_damage="reflow", + spec="https://github.com/w3c/csswg-drafts/issues/4650", + )} % endif % if engine in ["servo-2013", "servo-2020"]: @@ -151,6 +162,17 @@ ${helpers.single_keyword( servo_restyle_damage="reflow", )} + ${helpers.predefined_type( + "align-tracks", + "AlignTracks", + "specified::AlignTracks::default()", + engines="gecko", + gecko_pref="layout.css.grid-template-masonry-value.enabled", + animation_value_type="discrete", + servo_restyle_damage="reflow", + spec="https://github.com/w3c/csswg-drafts/issues/4650", + )} + ${helpers.predefined_type( "align-items", "AlignItems", @@ -372,6 +394,16 @@ ${helpers.predefined_type( % endfor +${helpers.predefined_type( + "masonry-auto-flow", + "MasonryAutoFlow", + "computed::MasonryAutoFlow::initial()", + engines="gecko", + gecko_pref="layout.css.grid-template-masonry-value.enabled", + animation_value_type="discrete", + spec="https://github.com/w3c/csswg-drafts/issues/4650", +)} + ${helpers.predefined_type( "grid-auto-flow", "GridAutoFlow", diff --git a/components/style/values/computed/align.rs b/components/style/values/computed/align.rs index 45d6cfac323..893f6fcce51 100644 --- a/components/style/values/computed/align.rs +++ b/components/style/values/computed/align.rs @@ -10,7 +10,7 @@ use crate::values::computed::{Context, ToComputedValue}; use crate::values::specified; pub use super::specified::{ - AlignContent, AlignItems, ContentDistribution, JustifyContent, SelfAlignment, + AlignContent, AlignItems, AlignTracks, ContentDistribution, JustifyContent, JustifyTracks, SelfAlignment, }; pub use super::specified::{AlignSelf, JustifySelf}; diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index 841cc3df22a..468deac4592 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -29,7 +29,7 @@ use std::cmp; use std::f32; #[cfg(feature = "gecko")] -pub use self::align::{AlignContent, AlignItems, JustifyContent, JustifyItems, SelfAlignment}; +pub use self::align::{AlignContent, AlignItems, AlignTracks, JustifyContent, JustifyItems, JustifyTracks, SelfAlignment}; #[cfg(feature = "gecko")] pub use self::align::{AlignSelf, JustifySelf}; pub use self::angle::Angle; @@ -68,7 +68,7 @@ pub use self::list::Quotes; pub use self::motion::{OffsetPath, OffsetRotate}; pub use self::outline::OutlineStyle; pub use self::percentage::{NonNegativePercentage, Percentage}; -pub use self::position::{GridAutoFlow, GridTemplateAreas, Position, PositionOrAuto, ZIndex}; +pub use self::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto, ZIndex}; pub use self::rect::NonNegativeLengthOrNumberRect; pub use self::resolution::Resolution; pub use self::svg::MozContextProperties; diff --git a/components/style/values/computed/position.rs b/components/style/values/computed/position.rs index 3eff231de88..554c36295c7 100644 --- a/components/style/values/computed/position.rs +++ b/components/style/values/computed/position.rs @@ -12,7 +12,7 @@ use crate::values::generics::position::Position as GenericPosition; use crate::values::generics::position::PositionComponent as GenericPositionComponent; use crate::values::generics::position::PositionOrAuto as GenericPositionOrAuto; use crate::values::generics::position::ZIndex as GenericZIndex; -pub use crate::values::specified::position::{GridAutoFlow, GridTemplateAreas}; +pub use crate::values::specified::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow}; use crate::Zero; use std::fmt::{self, Write}; use style_traits::{CssWriter, ToCss}; diff --git a/components/style/values/generics/grid.rs b/components/style/values/generics/grid.rs index 6f0f155912d..0fbeebea1fb 100644 --- a/components/style/values/generics/grid.rs +++ b/components/style/values/generics/grid.rs @@ -786,6 +786,9 @@ pub enum GenericGridTemplateComponent { /// TODO: Support animations for this after subgrid is addressed in [grid-2] spec. #[animation(error)] Subgrid(Box), + /// `masonry` value. + /// https://github.com/w3c/csswg-drafts/issues/4650 + Masonry, } pub use self::GenericGridTemplateComponent as GridTemplateComponent; diff --git a/components/style/values/specified/align.rs b/components/style/values/specified/align.rs index 10f7f3efbfc..4b483962f2f 100644 --- a/components/style/values/specified/align.rs +++ b/components/style/values/specified/align.rs @@ -171,16 +171,6 @@ impl ContentDistribution { Self { primary } } - fn from_bits(bits: u16) -> Self { - Self { - primary: AlignFlags::from_bits_truncate(bits as u8), - } - } - - fn as_bits(&self) -> u16 { - self.primary.bits() as u16 - } - /// Returns whether this value is a . pub fn is_baseline_position(&self) -> bool { matches!( @@ -292,6 +282,41 @@ impl SpecifiedValueInfo for AlignContent { } } +/// Value for the `align-tracks` property. +/// +/// +#[derive( + Clone, + Debug, + Default, + Eq, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +#[repr(transparent)] +#[css(comma)] +pub struct AlignTracks( + #[css(iterable, if_empty = "normal")] + pub crate::OwnedSlice +); + +impl Parse for AlignTracks { + fn parse<'i, 't>( + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { + let values = input.parse_comma_separated(|input| { + AlignContent::parse(context, input) + })?; + Ok(AlignTracks(values.into())) + } +} + /// Value for the `justify-content` property. /// /// @@ -329,18 +354,38 @@ impl SpecifiedValueInfo for JustifyContent { ContentDistribution::list_keywords(f, AxisDirection::Inline); } } +/// Value for the `justify-tracks` property. +/// +/// +#[derive( + Clone, + Debug, + Default, + Eq, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +#[repr(transparent)] +#[css(comma)] +pub struct JustifyTracks( + #[css(iterable, if_empty = "normal")] + pub crate::OwnedSlice +); -#[cfg(feature = "gecko")] -impl From for JustifyContent { - fn from(bits: u16) -> Self { - JustifyContent(ContentDistribution::from_bits(bits)) - } -} - -#[cfg(feature = "gecko")] -impl From for u16 { - fn from(v: JustifyContent) -> u16 { - v.0.as_bits() +impl Parse for JustifyTracks { + fn parse<'i, 't>( + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { + let values = input.parse_comma_separated(|input| { + JustifyContent::parse(context, input) + })?; + Ok(JustifyTracks(values.into())) } } diff --git a/components/style/values/specified/grid.rs b/components/style/values/specified/grid.rs index 54d31d5826d..9c6bf385d31 100644 --- a/components/style/values/specified/grid.rs +++ b/components/style/values/specified/grid.rs @@ -295,6 +295,18 @@ fn allow_grid_template_subgrids() -> bool { false } +#[cfg(feature = "gecko")] +#[inline] +fn allow_grid_template_masonry() -> bool { + static_prefs::pref!("layout.css.grid-template-masonry-value.enabled") +} + +#[cfg(feature = "servo")] +#[inline] +fn allow_grid_template_masonry() -> bool { + false +} + impl Parse for GridTemplateComponent { fn parse<'i, 't>( context: &ParserContext, @@ -319,7 +331,11 @@ impl GridTemplateComponent { return Ok(GridTemplateComponent::Subgrid(Box::new(t))); } } - + if allow_grid_template_masonry() { + if input.try(|i| i.expect_ident_matching("masonry")).is_ok() { + return Ok(GridTemplateComponent::Masonry); + } + } let track_list = TrackList::parse(context, input)?; Ok(GridTemplateComponent::TrackList(Box::new(track_list))) } diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index 63af3015386..c1c11d5f816 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -28,9 +28,9 @@ use style_traits::values::specified::AllowedNumericType; use style_traits::{CssWriter, ParseError, SpecifiedValueInfo, StyleParseErrorKind, ToCss}; #[cfg(feature = "gecko")] -pub use self::align::{AlignContent, AlignItems, AlignSelf, ContentDistribution}; +pub use self::align::{AlignContent, AlignItems, AlignSelf, AlignTracks, ContentDistribution}; #[cfg(feature = "gecko")] -pub use self::align::{JustifyContent, JustifyItems, JustifySelf, SelfAlignment}; +pub use self::align::{JustifyContent, JustifyItems, JustifySelf, JustifyTracks, SelfAlignment}; pub use self::angle::{AllowUnitlessZeroAngle, Angle}; pub use self::background::{BackgroundRepeat, BackgroundSize}; pub use self::basic_shape::FillRule; @@ -72,7 +72,7 @@ pub use self::list::Quotes; pub use self::motion::{OffsetPath, OffsetRotate}; pub use self::outline::OutlineStyle; pub use self::percentage::Percentage; -pub use self::position::{GridAutoFlow, GridTemplateAreas, Position, PositionOrAuto}; +pub use self::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto}; pub use self::position::{PositionComponent, ZIndex}; pub use self::rect::NonNegativeLengthOrNumberRect; pub use self::resolution::Resolution; diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index b843de29a41..33b9ddcf5b5 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -383,6 +383,172 @@ bitflags! { } } +#[derive( + Clone, + Copy, + Debug, + Eq, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +/// Masonry auto-placement algorithm packing. +pub enum MasonryPlacement { + /// Place the item in the track(s) with the smallest extent so far. + Pack, + /// Place the item after the last item, from start to end. + Next, +} + +#[derive( + Clone, + Copy, + Debug, + Eq, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +/// Masonry auto-placement algorithm item sorting option. +pub enum MasonryItemOrder { + /// Place all items with a definite placement before auto-placed items. + DefiniteFirst, + /// Place items in `order-modified document order`. + Ordered, +} + +#[derive( + Clone, + Copy, + Debug, + Eq, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +/// Controls how the Masonry layout algorithm works +/// specifying exactly how auto-placed items get flowed in the masonry axis. +pub struct MasonryAutoFlow { + /// Specify how to pick a auto-placement track. + #[css(contextual_skip_if = "is_pack_with_non_default_order")] + pub placement: MasonryPlacement, + /// Specify how to pick an item to place. + #[css(skip_if = "is_item_order_definite_first")] + pub order: MasonryItemOrder, +} + +#[inline] +fn is_pack_with_non_default_order(placement: &MasonryPlacement, order: &MasonryItemOrder) -> bool { + *placement == MasonryPlacement::Pack && + *order != MasonryItemOrder::DefiniteFirst +} + +#[inline] +fn is_item_order_definite_first(order: &MasonryItemOrder) -> bool { + *order == MasonryItemOrder::DefiniteFirst +} + +impl MasonryAutoFlow { + #[inline] + /// Get initial `masonry-auto-flow` value. + pub fn initial() -> MasonryAutoFlow { + MasonryAutoFlow { + placement: MasonryPlacement::Pack, + order: MasonryItemOrder::DefiniteFirst, + } + } +} + +impl Parse for MasonryAutoFlow { + /// [ definite-first | ordered ] || [ pack | next ] + fn parse<'i, 't>( + _context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { + let mut value = MasonryAutoFlow::initial(); + let mut got_placement = false; + let mut got_order = false; + while !input.is_exhausted() { + let location = input.current_source_location(); + let ident = input.expect_ident()?; + let success = match_ignore_ascii_case! { &ident, + "pack" if !got_placement => { + got_placement = true; + true + }, + "next" if !got_placement => { + value.placement = MasonryPlacement::Next; + got_placement = true; + true + }, + "definite-first" if !got_order => { + got_order = true; + true + }, + "ordered" if !got_order => { + value.order = MasonryItemOrder::Ordered; + got_order = true; + true + }, + _ => false + }; + if !success { + return Err(location + .new_custom_error(SelectorParseErrorKind::UnexpectedIdent(ident.clone()))); + } + } + + if got_placement || got_order { + Ok(value) + } else { + Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)) + } + } +} + +#[cfg(feature = "gecko")] +impl From for MasonryAutoFlow { + fn from(bits: u8) -> MasonryAutoFlow { + use crate::gecko_bindings::structs; + let mut value = MasonryAutoFlow::initial(); + if bits & structs::NS_STYLE_MASONRY_PLACEMENT_PACK as u8 == 0 { + value.placement = MasonryPlacement::Next; + } + if bits & structs::NS_STYLE_MASONRY_ORDER_DEFINITE_FIRST as u8 == 0 { + value.order = MasonryItemOrder::Ordered; + } + value + } +} + +#[cfg(feature = "gecko")] +impl From for u8 { + fn from(v: MasonryAutoFlow) -> u8 { + use crate::gecko_bindings::structs; + + let mut result: u8 = 0; + if v.placement == MasonryPlacement::Pack { + result |= structs::NS_STYLE_MASONRY_PLACEMENT_PACK as u8; + } + if v.order == MasonryItemOrder::DefiniteFirst { + result |= structs::NS_STYLE_MASONRY_ORDER_DEFINITE_FIRST as u8; + } + result + } +} + impl Parse for GridAutoFlow { /// [ row | column ] || dense fn parse<'i, 't>( From c67c9fdcee5859ac29a04355d120a010066983e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 30 Apr 2020 00:09:19 +0000 Subject: [PATCH 08/42] style: Only override to default color in high-contrast / forced-colors mode if inheriting from transparent. That way elements inside links, form controls, etc have the right contrast, even if the page overrides the color. We can't do it when inheriting from transparent because we've already forgotten about the "right" color to inherit, so the default color makes sense. But that is a pretty unlikely edge case. Differential Revision: https://phabricator.services.mozilla.com/D73069 --- components/style/properties/cascade.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index e9a34d6bf34..515239c4461 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -384,10 +384,15 @@ fn tweak_when_ignoring_colors( if color.0.is_transparent() { return; } - let color = builder.device.default_color(); - declarations_to_apply_unless_overriden.push( - PropertyDeclaration::Color(specified::ColorPropertyValue(color.into())) - ) + // If the inherited color would be transparent, but we would + // override this with a non-transparent color, then override it with + // the default color. Otherwise just let it inherit through. + if builder.get_parent_inherited_text().clone_color().alpha == 0 { + let color = builder.device.default_color(); + declarations_to_apply_unless_overriden.push( + PropertyDeclaration::Color(specified::ColorPropertyValue(color.into())) + ) + } }, // We honor url background-images if backplating. #[cfg(feature = "gecko")] From 6a7c0b7e9c075e34dd89f0b789b88ef205a445c8 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Mon, 4 May 2020 19:15:43 +0000 Subject: [PATCH 09/42] style: Add ::marker when checking may_have_animations() during traversal. When unhidding a ::marker element, we construct its generated item, and then call StyleNewSubtree() on this generated item. During traversal, we may update any animation related values in Gecko_UpdateAnimations(), which may update the base styles for animation properties. The test case is an animation segment from "null" to "inital" value. We replace the "null" value with the base style value for the specific animation property, so we can do interpolation properly. (e.g. opacity: "null => initial" becomes "1.0 => initial") If we don't update the animation related values in Gecko_UpdateAnimations after generating ::marker, we may do interpolation from "null" to "initial", which causes a panic. Differential Revision: https://phabricator.services.mozilla.com/D73408 --- components/style/gecko/pseudo_element.rs | 6 ++++++ components/style/gecko/wrapper.rs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/components/style/gecko/pseudo_element.rs b/components/style/gecko/pseudo_element.rs index d989380c02e..3ca06eea37e 100644 --- a/components/style/gecko/pseudo_element.rs +++ b/components/style/gecko/pseudo_element.rs @@ -92,6 +92,12 @@ impl PseudoElement { EAGER_PSEUDOS[i].clone() } + /// Whether the current pseudo element is animatable. + #[inline] + pub fn is_animatable(&self) -> bool { + matches!(*self, Self::Before | Self::After | Self::Marker) + } + /// Whether the current pseudo element is ::before or ::after. #[inline] pub fn is_before_or_after(&self) -> bool { diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index a5472fadd66..af3173f92d1 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1446,7 +1446,7 @@ impl<'le> TElement for GeckoElement<'le> { #[inline] fn may_have_animations(&self) -> bool { if let Some(pseudo) = self.implemented_pseudo_element() { - if !pseudo.is_before_or_after() { + if !pseudo.is_animatable() { return false; } // FIXME(emilio): When would the parent of a ::before / ::after From 735fdedbe9928f9cbd7863b5d4f171957b3c743e Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Tue, 5 May 2020 09:09:01 +0000 Subject: [PATCH 10/42] style: Make NotNull move its member pointer where possible. Differential Revision: https://phabricator.services.mozilla.com/D72827 --- components/style/gecko_bindings/sugar/ownership.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/components/style/gecko_bindings/sugar/ownership.rs b/components/style/gecko_bindings/sugar/ownership.rs index 5170b7dbc5a..0ece222a1c4 100644 --- a/components/style/gecko_bindings/sugar/ownership.rs +++ b/components/style/gecko_bindings/sugar/ownership.rs @@ -9,6 +9,7 @@ use std::marker::PhantomData; use std::mem::{forget, transmute}; use std::ops::{Deref, DerefMut}; use std::ptr; +use gecko_bindings::structs::root::mozilla::detail::CopyablePtr; /// Indicates that a given Servo type has a corresponding Gecko FFI type. pub unsafe trait HasFFI: Sized + 'static { @@ -332,3 +333,16 @@ impl OwnedOrNull { unsafe { transmute(self) } } } + +impl Deref for CopyablePtr { + type Target = T; + fn deref(&self) -> &Self::Target { + &self.mPtr + } +} + +impl DerefMut for CopyablePtr { + fn deref_mut<'a>(&'a mut self) -> &'a mut T { + &mut self.mPtr + } +} \ No newline at end of file From e6687c5fa8337a9cb78cc811960de520d1dbe5cf Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Tue, 5 May 2020 10:42:23 +0000 Subject: [PATCH 11/42] style: Improve handling of copying arrays in layout/style/ and servo/. Differential Revision: https://phabricator.services.mozilla.com/D72351 --- .../style/gecko_bindings/sugar/ns_t_array.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/components/style/gecko_bindings/sugar/ns_t_array.rs b/components/style/gecko_bindings/sugar/ns_t_array.rs index e3d4e5b86bd..04ba326c615 100644 --- a/components/style/gecko_bindings/sugar/ns_t_array.rs +++ b/components/style/gecko_bindings/sugar/ns_t_array.rs @@ -5,7 +5,7 @@ //! Rust helpers for Gecko's nsTArray. use crate::gecko_bindings::bindings; -use crate::gecko_bindings::structs::{nsTArray, nsTArrayHeader}; +use crate::gecko_bindings::structs::{nsTArray, nsTArrayHeader, CopyableTArray}; use std::mem; use std::ops::{Deref, DerefMut}; use std::slice; @@ -128,3 +128,16 @@ impl nsTArray { self.iter_mut().zip(iter).for_each(|(r, v)| *r = v); } } + +impl Deref for CopyableTArray { + type Target = nsTArray; + fn deref(&self) -> &Self::Target { + &self._base + } +} + +impl DerefMut for CopyableTArray { + fn deref_mut(&mut self) -> &mut nsTArray { + &mut self._base + } +} From a8c318a0684ec5ff5a40585b657369a46bfb3559 Mon Sep 17 00:00:00 2001 From: Emily McDonough Date: Wed, 6 May 2020 19:35:03 +0000 Subject: [PATCH 12/42] style: Use fill length rather than index to indicate a repeat(auto) in subgrid from Servo. Differential Revision: https://phabricator.services.mozilla.com/D70066 --- components/style/values/generics/grid.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/style/values/generics/grid.rs b/components/style/values/generics/grid.rs index 0fbeebea1fb..a0970a6fbc0 100644 --- a/components/style/values/generics/grid.rs +++ b/components/style/values/generics/grid.rs @@ -714,7 +714,7 @@ impl Parse for LineNameList { line_names.truncate(MAX_GRID_LINE as usize); } - let (fill_start, fill_len) = fill_data.unwrap_or((usize::MAX, 0)); + let (fill_start, fill_len) = fill_data.unwrap_or((0, 0)); Ok(LineNameList { names: line_names.into(), @@ -733,7 +733,7 @@ impl ToCss for LineNameList { let fill_start = self.fill_start; let fill_len = self.fill_len; for (i, names) in self.names.iter().enumerate() { - if i == fill_start { + if fill_len > 0 && i == fill_start { dest.write_str(" repeat(auto-fill,")?; } @@ -748,7 +748,7 @@ impl ToCss for LineNameList { } dest.write_str("]")?; - if i == fill_start + fill_len - 1 { + if fill_len > 0 && i == fill_start + fill_len - 1 { dest.write_str(")")?; } } From 709c52fefb0a514c9c59fd3ad3ed950392254cd9 Mon Sep 17 00:00:00 2001 From: Philipp Zech Date: Thu, 7 May 2020 08:32:27 +0000 Subject: [PATCH 13/42] style: Convert style-font #defines to an enum class. Differential Revision: https://phabricator.services.mozilla.com/D73728 --- components/style/properties/gecko.mako.rs | 40 +++++++++++------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 545e8cc77b5..8b10d71c146 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -953,19 +953,19 @@ fn static_assert() { if let Some(info) = v.keyword_info { self.gecko.mFontSizeKeyword = match info.kw { - KeywordSize::XXSmall => structs::NS_STYLE_FONT_SIZE_XXSMALL, - KeywordSize::XSmall => structs::NS_STYLE_FONT_SIZE_XSMALL, - KeywordSize::Small => structs::NS_STYLE_FONT_SIZE_SMALL, - KeywordSize::Medium => structs::NS_STYLE_FONT_SIZE_MEDIUM, - KeywordSize::Large => structs::NS_STYLE_FONT_SIZE_LARGE, - KeywordSize::XLarge => structs::NS_STYLE_FONT_SIZE_XLARGE, - KeywordSize::XXLarge => structs::NS_STYLE_FONT_SIZE_XXLARGE, - KeywordSize::XXXLarge => structs::NS_STYLE_FONT_SIZE_XXXLARGE, - } as u8; + KeywordSize::XXSmall => structs::StyleFontSize::Xxsmall, + KeywordSize::XSmall => structs::StyleFontSize::Xsmall, + KeywordSize::Small => structs::StyleFontSize::Small, + KeywordSize::Medium => structs::StyleFontSize::Medium, + KeywordSize::Large => structs::StyleFontSize::Large, + KeywordSize::XLarge => structs::StyleFontSize::Xxlarge, + KeywordSize::XXLarge => structs::StyleFontSize::Xxlarge, + KeywordSize::XXXLarge => structs::StyleFontSize::Xxxlarge, + }; self.gecko.mFontSizeFactor = info.factor; self.gecko.mFontSizeOffset = info.offset.to_i32_au(); } else { - self.gecko.mFontSizeKeyword = structs::NS_STYLE_FONT_SIZE_NO_KEYWORD as u8; + self.gecko.mFontSizeKeyword = structs::StyleFontSize::NoKeyword; self.gecko.mFontSizeFactor = 1.; self.gecko.mFontSizeOffset = 0; } @@ -974,16 +974,16 @@ fn static_assert() { pub fn clone_font_size(&self) -> FontSize { use crate::values::specified::font::{KeywordInfo, KeywordSize}; let size = Au(self.gecko.mSize).into(); - let kw = match self.gecko.mFontSizeKeyword as u32 { - structs::NS_STYLE_FONT_SIZE_XXSMALL => KeywordSize::XXSmall, - structs::NS_STYLE_FONT_SIZE_XSMALL => KeywordSize::XSmall, - structs::NS_STYLE_FONT_SIZE_SMALL => KeywordSize::Small, - structs::NS_STYLE_FONT_SIZE_MEDIUM => KeywordSize::Medium, - structs::NS_STYLE_FONT_SIZE_LARGE => KeywordSize::Large, - structs::NS_STYLE_FONT_SIZE_XLARGE => KeywordSize::XLarge, - structs::NS_STYLE_FONT_SIZE_XXLARGE => KeywordSize::XXLarge, - structs::NS_STYLE_FONT_SIZE_XXXLARGE => KeywordSize::XXXLarge, - structs::NS_STYLE_FONT_SIZE_NO_KEYWORD => { + let kw = match self.gecko.mFontSizeKeyword { + structs::StyleFontSize::Xxsmall => KeywordSize::XXSmall, + structs::StyleFontSize::Xsmall => KeywordSize::XSmall, + structs::StyleFontSize::Small => KeywordSize::Small, + structs::StyleFontSize::Medium => KeywordSize::Medium, + structs::StyleFontSize::Large => KeywordSize::Large, + structs::StyleFontSize::Xlarge => KeywordSize::XLarge, + structs::StyleFontSize::Xxlarge => KeywordSize::XXLarge, + structs::StyleFontSize::Xxxlarge => KeywordSize::XXXLarge, + structs::StyleFontSize::NoKeyword => { return FontSize { size, keyword_info: None, From 6bae0b99ca8881bf13b71ee088d9966fe21f8de4 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Mon, 11 May 2020 00:11:45 +0000 Subject: [PATCH 14/42] style: Gracefully handle errors creating shared memory UA style sheets. We still panic in a debug build, so that developers can notice when they need to add a new static atom after modifying UA sheets. We also add telemetry to note when this happens, add an app note to a crash report, in case any crash later on occurs, and re-up the existing, expired shared memory sheet telemetry probes so we can look at them again. Differential Revision: https://phabricator.services.mozilla.com/D73188 --- components/derive_common/cg.rs | 29 +++- components/style/gecko/url.rs | 8 +- components/style/gecko_string_cache/mod.rs | 18 +-- components/style/shared_lock.rs | 10 +- components/style/stylesheets/import_rule.rs | 9 +- components/style/stylesheets/mod.rs | 23 +-- components/style/values/computed/font.rs | 17 ++- components/style_traits/owned_slice.rs | 8 +- components/to_shmem/lib.rs | 154 +++++++++++--------- components/to_shmem_derive/to_shmem.rs | 30 ++-- 10 files changed, 184 insertions(+), 122 deletions(-) diff --git a/components/derive_common/cg.rs b/components/derive_common/cg.rs index c1b6588c749..c51c0d7750b 100644 --- a/components/derive_common/cg.rs +++ b/components/derive_common/cg.rs @@ -85,9 +85,22 @@ pub fn add_predicate(where_clause: &mut Option, pred: WherePre .push(pred); } -pub fn fmap_match(input: &DeriveInput, bind_style: BindStyle, mut f: F) -> TokenStream +pub fn fmap_match(input: &DeriveInput, bind_style: BindStyle, f: F) -> TokenStream where - F: FnMut(BindingInfo) -> TokenStream, + F: FnMut(&BindingInfo) -> TokenStream, +{ + fmap2_match(input, bind_style, f, |_| None) +} + +pub fn fmap2_match( + input: &DeriveInput, + bind_style: BindStyle, + mut f: F, + mut g: G, +) -> TokenStream +where + F: FnMut(&BindingInfo) -> TokenStream, + G: FnMut(&BindingInfo) -> Option, { let mut s = synstructure::Structure::new(input); s.variants_mut().iter_mut().for_each(|v| { @@ -95,12 +108,20 @@ where }); s.each_variant(|variant| { let (mapped, mapped_fields) = value(variant, "mapped"); - let fields_pairs = variant.bindings().iter().zip(mapped_fields); + let fields_pairs = variant.bindings().iter().zip(mapped_fields.iter()); let mut computations = quote!(); computations.append_all(fields_pairs.map(|(field, mapped_field)| { - let expr = f(field.clone()); + let expr = f(field); quote! { let #mapped_field = #expr; } })); + computations.append_all( + mapped_fields + .iter() + .map(|mapped_field| match g(mapped_field) { + Some(expr) => quote! { let #mapped_field = #expr; }, + None => quote!(), + }), + ); computations.append_all(mapped); Some(computations) }) diff --git a/components/style/gecko/url.rs b/components/style/gecko/url.rs index 27f3c60884c..678adb39b09 100644 --- a/components/style/gecko/url.rs +++ b/components/style/gecko/url.rs @@ -18,7 +18,7 @@ use std::fmt::{self, Write}; use std::mem::ManuallyDrop; use std::sync::RwLock; use style_traits::{CssWriter, ParseError, ToCss}; -use to_shmem::{SharedMemoryBuilder, ToShmem}; +use to_shmem::{self, SharedMemoryBuilder, ToShmem}; /// A CSS url() value for gecko. #[css(function = "url")] @@ -241,11 +241,11 @@ impl LoadDataSource { } impl ToShmem for LoadDataSource { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new(match self { + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { + Ok(ManuallyDrop::new(match self { LoadDataSource::Owned(..) => LoadDataSource::Lazy, LoadDataSource::Lazy => LoadDataSource::Lazy, - }) + })) } } diff --git a/components/style/gecko_string_cache/mod.rs b/components/style/gecko_string_cache/mod.rs index b9e3a0c608a..e9208a5e32e 100644 --- a/components/style/gecko_string_cache/mod.rs +++ b/components/style/gecko_string_cache/mod.rs @@ -3,7 +3,6 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ #![allow(unsafe_code)] - // This is needed for the constants in atom_macro.rs, because we have some // atoms whose names differ only by case, e.g. datetime and dateTime. #![allow(non_upper_case_globals)] @@ -30,7 +29,7 @@ use std::num::NonZeroUsize; use std::ops::Deref; use std::{slice, str}; use style_traits::SpecifiedValueInfo; -use to_shmem::{SharedMemoryBuilder, ToShmem}; +use to_shmem::{self, SharedMemoryBuilder, ToShmem}; #[macro_use] #[allow(improper_ctypes, non_camel_case_types, missing_docs)] @@ -132,14 +131,15 @@ impl Borrow for Atom { } impl ToShmem for Atom { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - assert!( - self.is_static(), - "ToShmem failed for Atom: must be a static atom: {}", - self - ); + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { + if !self.is_static() { + return Err(format!( + "ToShmem failed for Atom: must be a static atom: {}", + self + )); + } - ManuallyDrop::new(Atom(self.0)) + Ok(ManuallyDrop::new(Atom(self.0))) } } diff --git a/components/style/shared_lock.rs b/components/style/shared_lock.rs index d46370952d3..107fd6d7103 100644 --- a/components/style/shared_lock.rs +++ b/components/style/shared_lock.rs @@ -258,20 +258,20 @@ impl Locked { #[cfg(feature = "gecko")] impl ToShmem for Locked { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> to_shmem::Result { let guard = self.shared_lock.read(); - ManuallyDrop::new(Locked { + Ok(ManuallyDrop::new(Locked { shared_lock: SharedRwLock::read_only(), data: UnsafeCell::new(ManuallyDrop::into_inner( - self.read_with(&guard).to_shmem(builder), + self.read_with(&guard).to_shmem(builder)?, )), - }) + })) } } #[cfg(feature = "servo")] impl ToShmem for Locked { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { panic!("ToShmem not supported in Servo currently") } } diff --git a/components/style/stylesheets/import_rule.rs b/components/style/stylesheets/import_rule.rs index 982572673a9..5efae0a3ee5 100644 --- a/components/style/stylesheets/import_rule.rs +++ b/components/style/stylesheets/import_rule.rs @@ -15,9 +15,8 @@ use crate::stylesheets::{CssRule, Origin, StylesheetInDocument}; use crate::values::CssUrl; use cssparser::SourceLocation; use std::fmt::{self, Write}; -use std::mem::ManuallyDrop; use style_traits::{CssWriter, ToCss}; -use to_shmem::{SharedMemoryBuilder, ToShmem}; +use to_shmem::{self, SharedMemoryBuilder, ToShmem}; /// With asynchronous stylesheet parsing, we can't synchronously create a /// GeckoStyleSheet. So we use this placeholder instead. @@ -184,8 +183,10 @@ pub struct ImportRule { } impl ToShmem for ImportRule { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - panic!("ToShmem failed for ImportRule: cannot handle imported style sheets") + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { + Err(String::from( + "ToShmem failed for ImportRule: cannot handle imported style sheets", + )) } } diff --git a/components/style/stylesheets/mod.rs b/components/style/stylesheets/mod.rs index daf97f20e3c..12ba46b3e06 100644 --- a/components/style/stylesheets/mod.rs +++ b/components/style/stylesheets/mod.rs @@ -40,7 +40,7 @@ use std::fmt; use std::mem::{self, ManuallyDrop}; use style_traits::ParsingMode; #[cfg(feature = "gecko")] -use to_shmem::{SharedMemoryBuilder, ToShmem}; +use to_shmem::{self, SharedMemoryBuilder, ToShmem}; pub use self::counter_style_rule::CounterStyleRule; pub use self::document_rule::DocumentRule; @@ -117,20 +117,25 @@ impl Drop for UrlExtraData { #[cfg(feature = "gecko")] impl ToShmem for UrlExtraData { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { if self.0 & 1 == 0 { let shared_extra_datas = unsafe { &structs::URLExtraData_sShared }; let self_ptr = self.as_ref() as *const _ as *mut _; let sheet_id = shared_extra_datas .iter() - .position(|r| r.mRawPtr == self_ptr) - .expect( - "ToShmem failed for UrlExtraData: expected sheet's URLExtraData to be in \ - URLExtraData::sShared", - ); - ManuallyDrop::new(UrlExtraData((sheet_id << 1) | 1)) + .position(|r| r.mRawPtr == self_ptr); + let sheet_id = match sheet_id { + Some(id) => id, + None => { + return Err(String::from( + "ToShmem failed for UrlExtraData: expected sheet's URLExtraData to be in \ + URLExtraData::sShared", + )); + }, + }; + Ok(ManuallyDrop::new(UrlExtraData((sheet_id << 1) | 1))) } else { - ManuallyDrop::new(UrlExtraData(self.0)) + Ok(ManuallyDrop::new(UrlExtraData(self.0))) } } } diff --git a/components/style/values/computed/font.rs b/components/style/values/computed/font.rs index 0f821123c20..ed00f42d678 100644 --- a/components/style/values/computed/font.rs +++ b/components/style/values/computed/font.rs @@ -32,7 +32,7 @@ use std::mem::{self, ManuallyDrop}; use std::slice; use style_traits::{CssWriter, ParseError, ToCss}; #[cfg(feature = "gecko")] -use to_shmem::{SharedMemoryBuilder, ToShmem}; +use to_shmem::{self, SharedMemoryBuilder, ToShmem}; pub use crate::values::computed::Length as MozScriptMinSize; pub use crate::values::specified::font::{FontSynthesis, MozScriptSizeMultiplier}; @@ -466,19 +466,20 @@ pub enum FontFamilyList { #[cfg(feature = "gecko")] impl ToShmem for FontFamilyList { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { // In practice, the only SharedFontList objects we create from shared // style sheets are ones with a single generic entry. - ManuallyDrop::new(match *self { + Ok(ManuallyDrop::new(match *self { FontFamilyList::SharedFontList(ref r) => { - assert!( - r.mNames.len() == 1 && r.mNames[0].mName.mRawPtr.is_null(), - "ToShmem failed for FontFamilyList: cannot handle non-generic families", - ); + if !(r.mNames.len() == 1 && r.mNames[0].mName.mRawPtr.is_null()) { + return Err(String::from( + "ToShmem failed for FontFamilyList: cannot handle non-generic families", + )); + } FontFamilyList::Generic(r.mNames[0].mGeneric) }, FontFamilyList::Generic(t) => FontFamilyList::Generic(t), - }) + })) } } diff --git a/components/style_traits/owned_slice.rs b/components/style_traits/owned_slice.rs index bbce7065196..f6068365360 100644 --- a/components/style_traits/owned_slice.rs +++ b/components/style_traits/owned_slice.rs @@ -13,7 +13,7 @@ use std::marker::PhantomData; use std::ops::{Deref, DerefMut}; use std::ptr::NonNull; use std::{fmt, iter, mem, slice}; -use to_shmem::{SharedMemoryBuilder, ToShmem}; +use to_shmem::{self, SharedMemoryBuilder, ToShmem}; /// A struct that basically replaces a `Box<[T]>`, but which cbindgen can /// understand. @@ -159,10 +159,10 @@ impl MallocSizeOf for OwnedSlice { } impl ToShmem for OwnedSlice { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> mem::ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> to_shmem::Result { unsafe { - let dest = to_shmem::to_shmem_slice(self.iter(), builder); - mem::ManuallyDrop::new(Self::from(Box::from_raw(dest))) + let dest = to_shmem::to_shmem_slice(self.iter(), builder)?; + Ok(mem::ManuallyDrop::new(Self::from(Box::from_raw(dest)))) } } } diff --git a/components/to_shmem/lib.rs b/components/to_shmem/lib.rs index 22b86087bfd..47dc8d17eff 100644 --- a/components/to_shmem/lib.rs +++ b/components/to_shmem/lib.rs @@ -42,6 +42,11 @@ use std::slice; use std::str; use thin_slice::ThinBoxedSlice; +/// Result type for ToShmem::to_shmem. +/// +/// The String is an error message describing why the call failed. +pub type Result = std::result::Result, String>; + // Various pointer arithmetic functions in this file can be replaced with // functions on `Layout` once they have stabilized: // @@ -114,13 +119,13 @@ impl SharedMemoryBuilder { /// a shared memory buffer by calling ToShmem::to_shmem on it. /// /// Panics if there is insufficient space in the buffer. - pub fn write(&mut self, value: &T) -> *mut T { + pub fn write(&mut self, value: &T) -> std::result::Result<*mut T, String> { // Reserve space for the value. let dest: *mut T = self.alloc_value(); // Make a clone of the value with all of its heap allocations // placed in the shared memory buffer. - let value = value.to_shmem(self); + let value = value.to_shmem(self)?; unsafe { // Copy the value into the buffer. @@ -128,7 +133,7 @@ impl SharedMemoryBuilder { } // Return a pointer to the shared value. - dest + Ok(dest) } /// Reserves space in the shared memory buffer to fit a value of type T, @@ -190,7 +195,10 @@ pub trait ToShmem: Sized { /// /// The return type is wrapped in ManuallyDrop to make it harder to /// accidentally invoke the destructor of the value that is produced. - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop; + /// + /// Returns a Result so that we can gracefully recover from unexpected + /// content. + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result; } #[macro_export] @@ -201,8 +209,8 @@ macro_rules! impl_trivial_to_shmem { fn to_shmem( &self, _builder: &mut $crate::SharedMemoryBuilder, - ) -> ::std::mem::ManuallyDrop { - ::std::mem::ManuallyDrop::new(*self) + ) -> $crate::Result { + $crate::Result::Ok(::std::mem::ManuallyDrop::new(*self)) } } )* @@ -231,58 +239,60 @@ impl_trivial_to_shmem!(cssparser::SourceLocation); impl_trivial_to_shmem!(cssparser::TokenSerializationType); impl ToShmem for PhantomData { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new(*self) + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> Result { + Ok(ManuallyDrop::new(*self)) } } impl ToShmem for Range { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new(Range { - start: ManuallyDrop::into_inner(self.start.to_shmem(builder)), - end: ManuallyDrop::into_inner(self.end.to_shmem(builder)), - }) + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { + Ok(ManuallyDrop::new(Range { + start: ManuallyDrop::into_inner(self.start.to_shmem(builder)?), + end: ManuallyDrop::into_inner(self.end.to_shmem(builder)?), + })) } } impl ToShmem for cssparser::UnicodeRange { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new(cssparser::UnicodeRange { + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> Result { + Ok(ManuallyDrop::new(cssparser::UnicodeRange { start: self.start, end: self.end, - }) + })) } } impl ToShmem for (T, U) { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new(( - ManuallyDrop::into_inner(self.0.to_shmem(builder)), - ManuallyDrop::into_inner(self.1.to_shmem(builder)), - )) + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { + Ok(ManuallyDrop::new(( + ManuallyDrop::into_inner(self.0.to_shmem(builder)?), + ManuallyDrop::into_inner(self.1.to_shmem(builder)?), + ))) } } impl ToShmem for Wrapping { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new(Wrapping(ManuallyDrop::into_inner(self.0.to_shmem(builder)))) + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { + Ok(ManuallyDrop::new(Wrapping(ManuallyDrop::into_inner( + self.0.to_shmem(builder)?, + )))) } } impl ToShmem for Box { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { // Reserve space for the boxed value. let dest: *mut T = builder.alloc_value(); // Make a clone of the boxed value with all of its heap allocations // placed in the shared memory buffer. - let value = (**self).to_shmem(builder); + let value = (**self).to_shmem(builder)?; unsafe { // Copy the value into the buffer. ptr::write(dest, ManuallyDrop::into_inner(value)); - ManuallyDrop::new(Box::from_raw(dest)) + Ok(ManuallyDrop::new(Box::from_raw(dest))) } } } @@ -293,7 +303,7 @@ unsafe fn to_shmem_slice_ptr<'a, T, I>( src: I, dest: *mut T, builder: &mut SharedMemoryBuilder, -) -> *mut [T] +) -> std::result::Result<*mut [T], String> where T: 'a + ToShmem, I: ExactSizeIterator, @@ -303,15 +313,18 @@ where // Make a clone of each element from the iterator with its own heap // allocations placed in the buffer, and copy that clone into the buffer. for (src, dest) in src.zip(dest.iter_mut()) { - ptr::write(dest, ManuallyDrop::into_inner(src.to_shmem(builder))); + ptr::write(dest, ManuallyDrop::into_inner(src.to_shmem(builder)?)); } - dest + Ok(dest) } /// Writes all the items in `src` into a slice in the shared memory buffer and /// returns a pointer to the slice. -pub unsafe fn to_shmem_slice<'a, T, I>(src: I, builder: &mut SharedMemoryBuilder) -> *mut [T] +pub unsafe fn to_shmem_slice<'a, T, I>( + src: I, + builder: &mut SharedMemoryBuilder, +) -> std::result::Result<*mut [T], String> where T: 'a + ToShmem, I: ExactSizeIterator, @@ -321,16 +334,16 @@ where } impl ToShmem for Box<[T]> { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { unsafe { - let dest = to_shmem_slice(self.iter(), builder); - ManuallyDrop::new(Box::from_raw(dest)) + let dest = to_shmem_slice(self.iter(), builder)?; + Ok(ManuallyDrop::new(Box::from_raw(dest))) } } } impl ToShmem for ThinBoxedSlice { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { // We could support this if we needed but in practice we will never // need to handle such big ThinBoxedSlices. assert!( @@ -340,14 +353,14 @@ impl ToShmem for ThinBoxedSlice { ); unsafe { - let dest = to_shmem_slice(self.iter(), builder); - ManuallyDrop::new(ThinBoxedSlice::from_raw(dest)) + let dest = to_shmem_slice(self.iter(), builder)?; + Ok(ManuallyDrop::new(ThinBoxedSlice::from_raw(dest))) } } } impl ToShmem for Box { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { // Reserve space for the string bytes. let dest: *mut u8 = builder.alloc_array(self.len()); @@ -355,15 +368,15 @@ impl ToShmem for Box { // Copy the value into the buffer. ptr::copy(self.as_ptr(), dest, self.len()); - ManuallyDrop::new(Box::from_raw(str::from_utf8_unchecked_mut( - slice::from_raw_parts_mut(dest, self.len()), + Ok(ManuallyDrop::new(Box::from_raw( + str::from_utf8_unchecked_mut(slice::from_raw_parts_mut(dest, self.len())), ))) } } } impl ToShmem for String { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { // Reserve space for the string bytes. let dest: *mut u8 = builder.alloc_array(self.len()); @@ -371,13 +384,17 @@ impl ToShmem for String { // Copy the value into the buffer. ptr::copy(self.as_ptr(), dest, self.len()); - ManuallyDrop::new(String::from_raw_parts(dest, self.len(), self.len())) + Ok(ManuallyDrop::new(String::from_raw_parts( + dest, + self.len(), + self.len(), + ))) } } } impl ToShmem for CString { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { let len = self.as_bytes_with_nul().len(); // Reserve space for the string bytes. @@ -387,53 +404,55 @@ impl ToShmem for CString { // Copy the value into the buffer. ptr::copy(self.as_ptr(), dest, len); - ManuallyDrop::new(CString::from_raw(dest)) + Ok(ManuallyDrop::new(CString::from_raw(dest))) } } } impl ToShmem for Vec { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { unsafe { - let dest = to_shmem_slice(self.iter(), builder) as *mut T; + let dest = to_shmem_slice(self.iter(), builder)? as *mut T; let dest_vec = Vec::from_raw_parts(dest, self.len(), self.len()); - ManuallyDrop::new(dest_vec) + Ok(ManuallyDrop::new(dest_vec)) } } } impl> ToShmem for SmallVec { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { let dest_vec = unsafe { if self.spilled() { // Place the items in a separate allocation in the shared memory // buffer. - let dest = to_shmem_slice(self.iter(), builder) as *mut T; + let dest = to_shmem_slice(self.iter(), builder)? as *mut T; SmallVec::from_raw_parts(dest, self.len(), self.len()) } else { // Place the items inline. let mut s = SmallVec::new(); - to_shmem_slice_ptr(self.iter(), s.as_mut_ptr(), builder); + to_shmem_slice_ptr(self.iter(), s.as_mut_ptr(), builder)?; s.set_len(self.len()); s } }; - ManuallyDrop::new(dest_vec) + Ok(ManuallyDrop::new(dest_vec)) } } impl ToShmem for Option { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { - ManuallyDrop::new( - self.as_ref() - .map(|v| ManuallyDrop::into_inner(v.to_shmem(builder))), - ) + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { + let v = match self { + Some(v) => Some(ManuallyDrop::into_inner(v.to_shmem(builder)?)), + None => None, + }; + + Ok(ManuallyDrop::new(v)) } } impl ToShmem for Arc { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { // Assert that we don't encounter any shared references to values we // don't expect. Those we expect are those noted by calling // add_allowed_duplication_type, and should be types where we're fine @@ -453,7 +472,7 @@ impl ToShmem for Arc { // Make a clone of the Arc-owned value with all of its heap allocations // placed in the shared memory buffer. - let value = (**self).to_shmem(builder); + let value = (**self).to_shmem(builder)?; // Create a new Arc with the shared value and have it place its // ArcInner in the shared memory buffer. @@ -466,13 +485,13 @@ impl ToShmem for Arc { #[cfg(debug_assertions)] builder.shared_values.insert(self.heap_ptr()); - ManuallyDrop::new(static_arc) + Ok(ManuallyDrop::new(static_arc)) } } } impl ToShmem for ThinArc { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { // We don't currently have any shared ThinArc values in stylesheets, // so don't support them for now. #[cfg(debug_assertions)] @@ -484,8 +503,11 @@ impl ToShmem for ThinArc { // Make a clone of the Arc-owned header and slice values with all of // their heap allocations placed in the shared memory buffer. - let header = self.header.header.to_shmem(builder); - let values: Vec> = self.slice.iter().map(|v| v.to_shmem(builder)).collect(); + let header = self.header.header.to_shmem(builder)?; + let mut values = Vec::with_capacity(self.slice.len()); + for v in self.slice.iter() { + values.push(v.to_shmem(builder)?); + } // Create a new ThinArc with the shared value and have it place // its ArcInner in the shared memory buffer. @@ -499,13 +521,13 @@ impl ToShmem for ThinArc { #[cfg(debug_assertions)] builder.shared_values.insert(self.heap_ptr()); - ManuallyDrop::new(static_arc) + Ok(ManuallyDrop::new(static_arc)) } } } impl ToShmem for SmallBitVec { - fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> Result { let storage = match self.clone().into_storage() { InternalStorage::Spilled(vs) => { // Reserve space for the boxed slice values. @@ -524,13 +546,15 @@ impl ToShmem for SmallBitVec { }, InternalStorage::Inline(x) => InternalStorage::Inline(x), }; - ManuallyDrop::new(unsafe { SmallBitVec::from_storage(storage) }) + Ok(ManuallyDrop::new(unsafe { + SmallBitVec::from_storage(storage) + })) } } #[cfg(feature = "string_cache")] impl ToShmem for string_cache::Atom { - fn to_shmem(&self, _: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, _: &mut SharedMemoryBuilder) -> Result { // NOTE(emilio): In practice, this can be implemented trivially if // string_cache could expose the implementation detail of static atoms // being an index into the static table (and panicking in the diff --git a/components/to_shmem_derive/to_shmem.rs b/components/to_shmem_derive/to_shmem.rs index 01ba308e308..c3410bcc7e0 100644 --- a/components/to_shmem_derive/to_shmem.rs +++ b/components/to_shmem_derive/to_shmem.rs @@ -27,13 +27,23 @@ pub fn derive(mut input: syn::DeriveInput) -> TokenStream { input.generics.where_clause = where_clause; - let match_body = cg::fmap_match(&input, BindStyle::Ref, |binding| { - quote! { - ::std::mem::ManuallyDrop::into_inner( - ::to_shmem::ToShmem::to_shmem(#binding, builder) - ) - } - }); + // Do all of the `to_shmem()?` calls before the `ManuallyDrop::into_inner()` + // calls, so that we don't drop a value in the shared memory buffer if one + // of the `to_shmem`s fails. + let match_body = cg::fmap2_match( + &input, + BindStyle::Ref, + |binding| { + quote! { + ::to_shmem::ToShmem::to_shmem(#binding, builder)? + } + }, + |binding| { + Some(quote! { + ::std::mem::ManuallyDrop::into_inner(#binding) + }) + }, + ); let name = &input.ident; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); @@ -44,12 +54,12 @@ pub fn derive(mut input: syn::DeriveInput) -> TokenStream { fn to_shmem( &self, builder: &mut ::to_shmem::SharedMemoryBuilder, - ) -> ::std::mem::ManuallyDrop { - ::std::mem::ManuallyDrop::new( + ) -> ::to_shmem::Result { + Ok(::std::mem::ManuallyDrop::new( match *self { #match_body } - ) + )) } } } From ec6ecf7d2127b2adf3d059802a2446508a7475f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 11 May 2020 21:33:31 +0000 Subject: [PATCH 15/42] style: Clean up cascade rule iteration. r=nordzilla The current API was pretty awkward as a result of two things: * Not being able to create empty iterators for smallbitvec. * We used to call the `F` function multiple times, but turns out that collecting the declarations in a SmallVec was a perf win. So clean this up so that it looks more similar to other APIs, taking an iterator directly. This is a bit more code, but hopefully easier to understand (and also hopefully easier to optimize). The motivation for this work is that I plan to investigate rebasing / landing https://github.com/servo/servo/pull/20151, and I don't want more instantiations of apply_declarations and such. Differential Revision: https://phabricator.services.mozilla.com/D74369 --- components/style/animation.rs | 20 ++- components/style/properties/cascade.rs | 134 +++++++++++------- .../style/properties/declaration_block.rs | 10 +- components/style/stylist.rs | 12 +- 4 files changed, 105 insertions(+), 71 deletions(-) diff --git a/components/style/animation.rs b/components/style/animation.rs index 152692a0b28..a0d684f1a4b 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -978,7 +978,13 @@ where } => { let guard = declarations.read_with(context.guards.author); - let iter = || { + // This currently ignores visited styles, which seems acceptable, + // as existing browsers don't appear to animate visited styles. + let computed = properties::apply_declarations::( + context.stylist.device(), + /* pseudo = */ None, + previous_style.rules(), + &context.guards, // It's possible to have !important properties in keyframes // so we have to filter them out. // See the spec issue https://github.com/w3c/csswg-drafts/issues/1824 @@ -986,17 +992,7 @@ where guard .normal_declaration_iter() .filter(|declaration| declaration.is_animatable()) - .map(|decl| (decl, Origin::Author)) - }; - - // This currently ignores visited styles, which seems acceptable, - // as existing browsers don't appear to animate visited styles. - let computed = properties::apply_declarations::( - context.stylist.device(), - /* pseudo = */ None, - previous_style.rules(), - &context.guards, - iter, + .map(|decl| (decl, Origin::Author)), Some(previous_style), Some(previous_style), Some(previous_style), diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index 515239c4461..807e1d70ff5 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -10,8 +10,8 @@ use crate::dom::TElement; use crate::font_metrics::FontMetricsProvider; use crate::logical_geometry::WritingMode; use crate::media_queries::Device; -use crate::properties::{ComputedValues, StyleBuilder}; -use crate::properties::{LonghandId, LonghandIdSet, CSSWideKeyword}; +use crate::properties::{ComputedValues, StyleBuilder, Importance}; +use crate::properties::{LonghandId, LonghandIdSet, CSSWideKeyword, PropertyFlags}; use crate::properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator}; use crate::properties::{CASCADE_PROPERTY, ComputedValueFlags}; use crate::rule_cache::{RuleCache, RuleCacheConditions}; @@ -20,7 +20,6 @@ use crate::selector_parser::PseudoElement; use crate::stylesheets::{Origin, PerOrigin}; use servo_arc::Arc; use crate::shared_lock::StylesheetGuards; -use smallbitvec::SmallBitVec; use smallvec::SmallVec; use std::borrow::Cow; use std::cell::RefCell; @@ -108,6 +107,84 @@ where ) } +struct DeclarationIterator<'a> { + // Global to the iteration. + guards: &'a StylesheetGuards<'a>, + restriction: Option, + // The rule we're iterating over. + current_rule_node: Option<&'a StrongRuleNode>, + // Per rule state. + declarations: DeclarationImportanceIterator<'a>, + origin: Origin, + importance: Importance, +} + +impl<'a> DeclarationIterator<'a> { + #[inline] + fn new(rule_node: &'a StrongRuleNode, guards: &'a StylesheetGuards, pseudo: Option<&PseudoElement>) -> Self { + let restriction = pseudo.and_then(|p| p.property_restriction()); + let mut iter = Self { + guards, + current_rule_node: Some(rule_node), + origin: Origin::Author, + importance: Importance::Normal, + declarations: DeclarationImportanceIterator::default(), + restriction, + }; + iter.update_for_node(rule_node); + iter + } + + fn update_for_node(&mut self, node: &'a StrongRuleNode) { + let origin = node.cascade_level().origin(); + self.origin = origin; + self.importance = node.importance(); + let guard = match origin { + Origin::Author => self.guards.author, + Origin::User | Origin::UserAgent => self.guards.ua_or_user, + }; + self.declarations = match node.style_source() { + Some(source) => source.read(guard).declaration_importance_iter(), + None => DeclarationImportanceIterator::default(), + }; + } + +} + +impl<'a> Iterator for DeclarationIterator<'a> { + type Item = (&'a PropertyDeclaration, Origin); + + #[inline] + fn next(&mut self) -> Option { + loop { + if let Some((decl, importance)) = self.declarations.next_back() { + if self.importance != importance { + continue; + } + + let origin = self.origin; + if let Some(restriction) = self.restriction { + // decl.id() is either a longhand or a custom + // property. Custom properties are always allowed, but + // longhands are only allowed if they have our + // restriction flag set. + if let PropertyDeclarationId::Longhand(id) = decl.id() { + if !id.flags().contains(restriction) && origin != Origin::UserAgent { + continue; + } + } + } + + return Some((decl, origin)); + } + + let next_node = self.current_rule_node.take()?.parent()?; + self.current_rule_node = Some(next_node); + self.update_for_node(next_node); + } + } +} + fn cascade_rules( device: &Device, pseudo: Option<&PseudoElement>, @@ -130,54 +207,12 @@ where parent_style.is_some(), parent_style_ignoring_first_line.is_some() ); - let empty = SmallBitVec::new(); - let restriction = pseudo.and_then(|p| p.property_restriction()); - let iter_declarations = || { - rule_node.self_and_ancestors().flat_map(|node| { - let origin = node.cascade_level().origin(); - let node_importance = node.importance(); - let guard = match origin { - Origin::Author => guards.author, - Origin::User | Origin::UserAgent => guards.ua_or_user, - }; - let declarations = match node.style_source() { - Some(source) => source - .read(guard) - .declaration_importance_iter(), - None => DeclarationImportanceIterator::new(&[], &empty), - }; - - declarations - // Yield declarations later in source order (with more precedence) first. - .rev() - .filter_map(move |(declaration, declaration_importance)| { - if let Some(restriction) = restriction { - // declaration.id() is either a longhand or a custom - // property. Custom properties are always allowed, but - // longhands are only allowed if they have our - // restriction flag set. - if let PropertyDeclarationId::Longhand(id) = declaration.id() { - if !id.flags().contains(restriction) && origin != Origin::UserAgent { - return None; - } - } - } - - if declaration_importance == node_importance { - Some((declaration, origin)) - } else { - None - } - }) - }) - }; - apply_declarations( device, pseudo, rule_node, guards, - iter_declarations, + DeclarationIterator::new(rule_node, guards, pseudo), parent_style, parent_style_ignoring_first_line, layout_parent_style, @@ -208,12 +243,12 @@ pub enum CascadeMode<'a> { /// NOTE: This function expects the declaration with more priority to appear /// first. -pub fn apply_declarations<'a, E, F, I>( +pub fn apply_declarations<'a, E, I>( device: &Device, pseudo: Option<&PseudoElement>, rules: &StrongRuleNode, guards: &StylesheetGuards, - iter_declarations: F, + iter: I, parent_style: Option<&ComputedValues>, parent_style_ignoring_first_line: Option<&ComputedValues>, layout_parent_style: Option<&ComputedValues>, @@ -226,7 +261,6 @@ pub fn apply_declarations<'a, E, F, I>( ) -> Arc where E: TElement, - F: Fn() -> I, I: Iterator, { debug_assert!(layout_parent_style.is_none() || parent_style.is_some()); @@ -253,7 +287,7 @@ where device, ); - for (declaration, origin) in iter_declarations() { + for (declaration, origin) in iter { declarations.push((declaration, origin)); if let PropertyDeclaration::Custom(ref declaration) = *declaration { builder.cascade(declaration, origin); diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 53b483887ce..76b7ed864ea 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -110,9 +110,17 @@ pub struct DeclarationImportanceIterator<'a> { iter: Zip, smallbitvec::Iter<'a>>, } +impl<'a> Default for DeclarationImportanceIterator<'a> { + fn default() -> Self { + Self { + iter: [].iter().zip(smallbitvec::Iter::default()), + } + } +} + impl<'a> DeclarationImportanceIterator<'a> { /// Constructor. - pub fn new(declarations: &'a [PropertyDeclaration], important: &'a SmallBitVec) -> Self { + fn new(declarations: &'a [PropertyDeclaration], important: &'a SmallBitVec) -> Self { DeclarationImportanceIterator { iter: declarations.iter().zip(important.iter()), } diff --git a/components/style/stylist.rs b/components/style/stylist.rs index e82128f22f7..bb6aec6c52d 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1318,12 +1318,6 @@ impl Stylist { use crate::font_metrics::get_metrics_provider_for_product; let block = declarations.read_with(guards.author); - let iter_declarations = || { - block - .declaration_importance_iter() - .map(|(declaration, _)| (declaration, Origin::Author)) - }; - let metrics = get_metrics_provider_for_product(); // We don't bother inserting these declarations in the rule tree, since @@ -1332,12 +1326,14 @@ impl Stylist { // TODO(emilio): Now that we fixed bug 1493420, we should consider // reversing this as it shouldn't be slow anymore, and should avoid // generating two instantiations of apply_declarations. - properties::apply_declarations::( + properties::apply_declarations::( &self.device, /* pseudo = */ None, self.rule_tree.root(), guards, - iter_declarations, + block + .declaration_importance_iter() + .map(|(declaration, _)| (declaration, Origin::Author)), Some(parent_style), Some(parent_style), Some(parent_style), From 4359aae44e08b92eda0de557c5abbf37491dba08 Mon Sep 17 00:00:00 2001 From: longsonr Date: Wed, 13 May 2020 09:10:54 +0000 Subject: [PATCH 16/42] style: Don't apply minimum font sizes to SVG text. Differential Revision: https://phabricator.services.mozilla.com/D74581 --- components/style/properties/cascade.rs | 6 +++--- components/style/properties/gecko.mako.rs | 6 +++--- components/style/values/computed/mod.rs | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index 807e1d70ff5..ab60099b3a4 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -920,8 +920,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { let builder = &mut self.context.builder; - let parent_zoom = builder.get_parent_font().gecko().mAllowZoom; - let zoom = builder.get_font().gecko().mAllowZoom; + let parent_zoom = builder.get_parent_font().gecko().mAllowZoomAndMinSize; + let zoom = builder.get_font().gecko().mAllowZoomAndMinSize; if zoom == parent_zoom { return; } @@ -968,7 +968,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { } let mut min = Au(parent_font.mScriptMinSize); - if font.mAllowZoom { + if font.mAllowZoomAndMinSize { min = builder.device.zoom_text(min); } diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 8b10d71c146..69d0ac3fb92 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -1108,12 +1108,12 @@ fn static_assert() { #[allow(non_snake_case)] pub fn set__x_text_zoom(&mut self, v: longhands::_x_text_zoom::computed_value::T) { - self.gecko.mAllowZoom = v.0; + self.gecko.mAllowZoomAndMinSize = v.0; } #[allow(non_snake_case)] pub fn copy__x_text_zoom_from(&mut self, other: &Self) { - self.gecko.mAllowZoom = other.gecko.mAllowZoom; + self.gecko.mAllowZoomAndMinSize = other.gecko.mAllowZoomAndMinSize; } #[allow(non_snake_case)] @@ -1123,7 +1123,7 @@ fn static_assert() { #[allow(non_snake_case)] pub fn clone__x_text_zoom(&self) -> longhands::_x_text_zoom::computed_value::T { - longhands::_x_text_zoom::computed_value::T(self.gecko.mAllowZoom) + longhands::_x_text_zoom::computed_value::T(self.gecko.mAllowZoomAndMinSize) } <% impl_simple_type_with_conversion("font_language_override", "mFont.languageOverride") %> diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index 468deac4592..aba433d7443 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -223,8 +223,8 @@ impl<'a> Context<'a> { pub fn maybe_zoom_text(&self, size: CSSPixelLength) -> CSSPixelLength { // We disable zoom for by unsetting the // -x-text-zoom property, which leads to a false value - // in mAllowZoom - if self.style().get_font().gecko.mAllowZoom { + // in mAllowZoomAndMinSize + if self.style().get_font().gecko.mAllowZoomAndMinSize { self.device().zoom_text(Au::from(size)).into() } else { size From 0c4bba6b526e56609a9d6756749629a71dd5b555 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 14 May 2020 16:46:08 +0000 Subject: [PATCH 17/42] style: Unprefix -moz-read-write / -moz-read-only. And remove some duplicated tests from WPT. Differential Revision: https://phabricator.services.mozilla.com/D75231 --- components/style/element_state.rs | 4 ++-- components/style/gecko/non_ts_pseudo_class_list.rs | 4 ++-- components/style/gecko/selector_parser.rs | 2 ++ components/style/gecko/wrapper.rs | 4 ++-- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/components/style/element_state.rs b/components/style/element_state.rs index cf943cc3c4e..190c1cc3eb5 100644 --- a/components/style/element_state.rs +++ b/components/style/element_state.rs @@ -87,9 +87,9 @@ bitflags! { /// const IN_OUTOFRANGE_STATE = 1 << 28; /// - const IN_MOZ_READONLY_STATE = 1 << 29; + const IN_READONLY_STATE = 1 << 29; /// - const IN_MOZ_READWRITE_STATE = 1 << 30; + const IN_READWRITE_STATE = 1 << 30; /// const IN_DEFAULT_STATE = 1 << 31; /// Non-standard: https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-submit-invalid diff --git a/components/style/gecko/non_ts_pseudo_class_list.rs b/components/style/gecko/non_ts_pseudo_class_list.rs index ea5db98030a..b82818b9bae 100644 --- a/components/style/gecko/non_ts_pseudo_class_list.rs +++ b/components/style/gecko/non_ts_pseudo_class_list.rs @@ -79,8 +79,8 @@ macro_rules! apply_non_ts_list { ("out-of-range", OutOfRange, IN_OUTOFRANGE_STATE, _), ("default", Default, IN_DEFAULT_STATE, _), ("placeholder-shown", PlaceholderShown, IN_PLACEHOLDER_SHOWN_STATE, _), - ("-moz-read-only", MozReadOnly, IN_MOZ_READONLY_STATE, _), - ("-moz-read-write", MozReadWrite, IN_MOZ_READWRITE_STATE, _), + ("read-only", ReadOnly, IN_READONLY_STATE, _), + ("read-write", ReadWrite, IN_READWRITE_STATE, _), ("-moz-submit-invalid", MozSubmitInvalid, IN_MOZ_SUBMITINVALID_STATE, _), ("-moz-ui-valid", MozUIValid, IN_MOZ_UI_VALID_STATE, _), ("-moz-ui-invalid", MozUIInvalid, IN_MOZ_UI_INVALID_STATE, _), diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index 4b96aa2148c..adb410908b9 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -119,6 +119,8 @@ impl NonTSPseudoClass { match_ignore_ascii_case! { &name, $($css => Some(NonTSPseudoClass::$name),)* "-moz-full-screen" => Some(NonTSPseudoClass::Fullscreen), + "-moz-read-only" => Some(NonTSPseudoClass::ReadOnly), + "-moz-read-write" => Some(NonTSPseudoClass::ReadWrite), _ => None, } } diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index af3173f92d1..0fbaf5f0aba 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -2037,8 +2037,8 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { NonTSPseudoClass::MozHandlerCrashed | NonTSPseudoClass::Required | NonTSPseudoClass::Optional | - NonTSPseudoClass::MozReadOnly | - NonTSPseudoClass::MozReadWrite | + NonTSPseudoClass::ReadOnly | + NonTSPseudoClass::ReadWrite | NonTSPseudoClass::FocusWithin | NonTSPseudoClass::FocusVisible | NonTSPseudoClass::MozDragOver | From 7cbc963fc7c624d5de71a20f8f9bf2d2ceace54e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 18 May 2020 10:54:16 +0000 Subject: [PATCH 18/42] style: Make ::-moz-focus-outer a no-op, and remove it on Nightly. See https://bugzilla.mozilla.org/show_bug.cgi?id=932410#c2 for the context for which this pseudo-element was added. In the previous patch, I had to special-case range appearance because of this pseudo-class, but that patch makes this pseudo-class completely redundant, as now all form controls, themed and unthemed, display outlines, unless the native theme displays a focus indicator on its own. Remove the special case, and make ranges use outlines like everything else rather than this bespoke pseudo-element. Differential Revision: https://phabricator.services.mozilla.com/D74734 --- components/style/gecko/pseudo_element.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/components/style/gecko/pseudo_element.rs b/components/style/gecko/pseudo_element.rs index 3ca06eea37e..b537295f785 100644 --- a/components/style/gecko/pseudo_element.rs +++ b/components/style/gecko/pseudo_element.rs @@ -159,7 +159,13 @@ impl PseudoElement { /// Whether this pseudo-element is enabled for all content. pub fn enabled_in_content(&self) -> bool { - (self.flags() & structs::CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS_AND_CHROME) == 0 + if (self.flags() & structs::CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS_AND_CHROME) != 0 { + return false; + } + match *self { + PseudoElement::MozFocusOuter => static_prefs::pref!("layout.css.moz-focus-outer.enabled"), + _ => true, + } } /// Whether this pseudo is enabled explicitly in UA sheets. From a457a2261b16f7cec7498ff0cfe4d290d13f2fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 20 May 2020 12:16:22 +0000 Subject: [PATCH 19/42] style: Fix parsing of :is() and :where() to account for constraints from parent selectors. Differential Revision: https://phabricator.services.mozilla.com/D75856 --- components/selectors/parser.rs | 173 +++++++++++++++++++-------------- 1 file changed, 99 insertions(+), 74 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 15440703033..1548a6fdcab 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -104,41 +104,59 @@ bitflags! { /// disallowed. If this flag is set, `AFTER_PSEUDO_ELEMENT` must be set /// as well. const AFTER_NON_STATEFUL_PSEUDO_ELEMENT = 1 << 4; + /// Whether we are after any of the pseudo-like things. const AFTER_PSEUDO = Self::AFTER_PART.bits | Self::AFTER_SLOTTED.bits | Self::AFTER_PSEUDO_ELEMENT.bits; + + /// Whether we explicitly disallow combinators. + const DISALLOW_COMBINATORS = 1 << 5; + + /// Whether we explicitly disallow pseudo-element-like things. + const DISALLOW_PSEUDOS = 1 << 6; } } impl SelectorParsingState { #[inline] - fn allows_functional_pseudo_classes(self) -> bool { - !self.intersects(SelectorParsingState::AFTER_PSEUDO) + fn allows_pseudos(self) -> bool { + // NOTE(emilio): We allow pseudos after ::part and such. + !self.intersects(Self::AFTER_PSEUDO_ELEMENT | Self::DISALLOW_PSEUDOS) } #[inline] fn allows_slotted(self) -> bool { - !self.intersects(SelectorParsingState::AFTER_PSEUDO) + !self.intersects(Self::AFTER_PSEUDO | Self::DISALLOW_PSEUDOS) } - // TODO(emilio): Should we allow other ::part()s after ::part()? - // - // See https://github.com/w3c/csswg-drafts/issues/3841 #[inline] fn allows_part(self) -> bool { - !self.intersects(SelectorParsingState::AFTER_PSEUDO) + !self.intersects(Self::AFTER_PSEUDO | Self::DISALLOW_PSEUDOS) + } + + // TODO(emilio): Maybe some of these should be allowed, but this gets us on + // the safe side for now, matching previous behavior. Gotta be careful with + // the ones like :-moz-any, which allow nested selectors but don't carry the + // state, and so on. + #[inline] + fn allows_custom_functional_pseudo_classes(self) -> bool { + !self.intersects(Self::AFTER_PSEUDO) } #[inline] fn allows_non_functional_pseudo_classes(self) -> bool { !self.intersects( - SelectorParsingState::AFTER_SLOTTED | - SelectorParsingState::AFTER_NON_STATEFUL_PSEUDO_ELEMENT, + Self::AFTER_SLOTTED | Self::AFTER_NON_STATEFUL_PSEUDO_ELEMENT, ) } #[inline] fn allows_tree_structural_pseudo_classes(self) -> bool { - !self.intersects(SelectorParsingState::AFTER_PSEUDO) + !self.intersects(Self::AFTER_PSEUDO) + } + + #[inline] + fn allows_combinators(self) -> bool { + !self.intersects(Self::DISALLOW_COMBINATORS) } } @@ -146,7 +164,6 @@ pub type SelectorParseError<'i> = ParseError<'i, SelectorParseErrorKind<'i>>; #[derive(Clone, Debug, PartialEq)] pub enum SelectorParseErrorKind<'i> { - PseudoElementInComplexSelector, NoQualifiedNameInAttributeSelector(Token<'i>), EmptySelector, DanglingCombinator, @@ -321,6 +338,17 @@ impl SelectorList { parser: &P, input: &mut CssParser<'i, 't>, ) -> Result> + where + P: Parser<'i, Impl = Impl>, + { + Self::parse_with_state(parser, input, SelectorParsingState::empty()) + } + + fn parse_with_state<'i, 't, P>( + parser: &P, + input: &mut CssParser<'i, 't>, + state: SelectorParsingState, + ) -> Result> where P: Parser<'i, Impl = Impl>, { @@ -328,7 +356,7 @@ impl SelectorList { loop { values.push( input - .parse_until_before(Delimiter::Comma, |input| parse_selector(parser, input))?, + .parse_until_before(Delimiter::Comma, |input| parse_selector(parser, input, state))?, ); match input.next() { Err(_) => return Ok(SelectorList(values)), @@ -344,30 +372,17 @@ impl SelectorList { } } -/// Parses one compound selector suitable for nested stuff like ::-moz-any, etc. +/// Parses one compound selector suitable for nested stuff like :-moz-any, etc. fn parse_inner_compound_selector<'i, 't, P, Impl>( parser: &P, input: &mut CssParser<'i, 't>, + state: SelectorParsingState, ) -> Result, ParseError<'i, P::Error>> where P: Parser<'i, Impl = Impl>, Impl: SelectorImpl, { - let location = input.current_source_location(); - let selector = parse_selector(parser, input)?; - - // Ensure they're actually all compound selectors without pseudo-elements. - if selector.has_pseudo_element() { - return Err( - location.new_custom_error(SelectorParseErrorKind::PseudoElementInComplexSelector) - ); - } - - if selector.iter_raw_match_order().any(|s| s.is_combinator()) { - return Err(location.new_custom_error(SelectorParseErrorKind::NonCompoundSelector)); - } - - Ok(selector) + parse_selector(parser, input, state | SelectorParsingState::DISALLOW_PSEUDOS | SelectorParsingState::DISALLOW_COMBINATORS) } /// Parse a comma separated list of compound selectors. @@ -380,7 +395,7 @@ where Impl: SelectorImpl, { input - .parse_comma_separated(|input| parse_inner_compound_selector(parser, input)) + .parse_comma_separated(|input| parse_inner_compound_selector(parser, input, SelectorParsingState::empty())) .map(|selectors| selectors.into_boxed_slice()) } @@ -1542,6 +1557,7 @@ fn display_to_css_identifier(x: &T, dest: &mut W) -> fn parse_selector<'i, 't, P, Impl>( parser: &P, input: &mut CssParser<'i, 't>, + mut state: SelectorParsingState, ) -> Result, ParseError<'i, P::Error>> where P: Parser<'i, Impl = Impl>, @@ -1554,16 +1570,14 @@ where let mut part = false; 'outer_loop: loop { // Parse a sequence of simple selectors. - let state = match parse_compound_selector(parser, input, &mut builder)? { - Some(state) => state, - None => { - return Err(input.new_custom_error(if builder.has_combinators() { - SelectorParseErrorKind::DanglingCombinator - } else { - SelectorParseErrorKind::EmptySelector - })); - }, - }; + let empty = parse_compound_selector(parser, &mut state, input, &mut builder)?; + if empty { + return Err(input.new_custom_error(if builder.has_combinators() { + SelectorParseErrorKind::DanglingCombinator + } else { + SelectorParseErrorKind::EmptySelector + })); + } if state.intersects(SelectorParsingState::AFTER_PSEUDO) { has_pseudo_element = state.intersects(SelectorParsingState::AFTER_PSEUDO_ELEMENT); @@ -1604,6 +1618,11 @@ where }, } } + + if !state.allows_combinators() { + return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); + } + builder.push_combinator(combinator); } @@ -1620,7 +1639,7 @@ impl Selector { where P: Parser<'i, Impl = Impl>, { - parse_selector(parser, input) + parse_selector(parser, input, SelectorParsingState::empty()) } } @@ -1630,6 +1649,7 @@ impl Selector { fn parse_type_selector<'i, 't, P, Impl, S>( parser: &P, input: &mut CssParser<'i, 't>, + state: SelectorParsingState, sink: &mut S, ) -> Result> where @@ -1644,6 +1664,9 @@ where }) | Ok(OptionalQName::None(_)) => Ok(false), Ok(OptionalQName::Some(namespace, local_name)) => { + if state.intersects(SelectorParsingState::AFTER_PSEUDO) { + return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); + } match namespace { QNamePrefix::ImplicitAnyNamespace => {}, QNamePrefix::ImplicitDefaultNamespace(url) => { @@ -2015,11 +2038,14 @@ fn parse_attribute_flags<'i, 't>( fn parse_negation<'i, 't, P, Impl>( parser: &P, input: &mut CssParser<'i, 't>, + state: SelectorParsingState, ) -> Result, ParseError<'i, P::Error>> where P: Parser<'i, Impl = Impl>, Impl: SelectorImpl, { + let state = state | SelectorParsingState::INSIDE_NEGATION; + // We use a sequence because a type selector may be represented as two Components. let mut sequence = SmallVec::<[Component; 2]>::new(); @@ -2027,7 +2053,7 @@ where // Get exactly one simple selector. The parse logic in the caller will verify // that there are no trailing tokens after we're done. - let is_type_sel = match parse_type_selector(parser, input, &mut sequence) { + let is_type_sel = match parse_type_selector(parser, input, state, &mut sequence) { Ok(result) => result, Err(ParseError { kind: ParseErrorKind::Basic(BasicParseErrorKind::EndOfInput), @@ -2036,7 +2062,7 @@ where Err(e) => return Err(e.into()), }; if !is_type_sel { - match parse_one_simple_selector(parser, input, SelectorParsingState::INSIDE_NEGATION)? { + match parse_one_simple_selector(parser, input, state)? { Some(SimpleSelectorParseResult::SimpleSelector(s)) => { sequence.push(s); }, @@ -2063,12 +2089,13 @@ where /// | [ HASH | class | attrib | pseudo | negation ]+ /// /// `Err(())` means invalid selector. -/// `Ok(None)` is an empty selector +/// `Ok(true)` is an empty selector fn parse_compound_selector<'i, 't, P, Impl>( parser: &P, + state: &mut SelectorParsingState, input: &mut CssParser<'i, 't>, builder: &mut SelectorBuilder, -) -> Result, ParseError<'i, P::Error>> +) -> Result> where P: Parser<'i, Impl = Impl>, Impl: SelectorImpl, @@ -2076,13 +2103,12 @@ where input.skip_whitespace(); let mut empty = true; - if parse_type_selector(parser, input, builder)? { + if parse_type_selector(parser, input, *state, builder)? { empty = false; } - let mut state = SelectorParsingState::empty(); loop { - let result = match parse_one_simple_selector(parser, input, state)? { + let result = match parse_one_simple_selector(parser, input, *state)? { None => break, Some(result) => result, }; @@ -2141,17 +2167,13 @@ where }, } } - if empty { - // An empty selector is invalid. - Ok(None) - } else { - Ok(Some(state)) - } + Ok(empty) } fn parse_is_or_where<'i, 't, P, Impl>( parser: &P, input: &mut CssParser<'i, 't>, + state: SelectorParsingState, component: impl FnOnce(Box<[Selector]>) -> Component, ) -> Result, ParseError<'i, P::Error>> where @@ -2159,15 +2181,12 @@ where Impl: SelectorImpl, { debug_assert!(parser.parse_is_and_where()); - let inner = SelectorList::parse(parser, input)?; // https://drafts.csswg.org/selectors/#matches-pseudo: // // Pseudo-elements cannot be represented by the matches-any // pseudo-class; they are not valid within :is(). // - if inner.0.iter().any(|i| i.has_pseudo_element()) { - return Err(input.new_custom_error(SelectorParseErrorKind::InvalidPseudoElementInsideWhere)); - } + let inner = SelectorList::parse_with_state(parser, input, state | SelectorParsingState::DISALLOW_PSEUDOS)?; Ok(component(inner.0.into_vec().into_boxed_slice())) } @@ -2181,40 +2200,46 @@ where P: Parser<'i, Impl = Impl>, Impl: SelectorImpl, { - if !state.allows_functional_pseudo_classes() { - return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); - } - debug_assert!(state.allows_tree_structural_pseudo_classes()); match_ignore_ascii_case! { &name, - "nth-child" => return Ok(parse_nth_pseudo_class(input, Component::NthChild)?), - "nth-of-type" => return Ok(parse_nth_pseudo_class(input, Component::NthOfType)?), - "nth-last-child" => return Ok(parse_nth_pseudo_class(input, Component::NthLastChild)?), - "nth-last-of-type" => return Ok(parse_nth_pseudo_class(input, Component::NthLastOfType)?), - "is" if parser.parse_is_and_where() => return parse_is_or_where(parser, input, Component::Is), - "where" if parser.parse_is_and_where() => return parse_is_or_where(parser, input, Component::Where), - "host" => return Ok(Component::Host(Some(parse_inner_compound_selector(parser, input)?))), + "nth-child" => return parse_nth_pseudo_class(parser, input, state, Component::NthChild), + "nth-of-type" => return parse_nth_pseudo_class(parser, input, state, Component::NthOfType), + "nth-last-child" => return parse_nth_pseudo_class(parser, input, state, Component::NthLastChild), + "nth-last-of-type" => return parse_nth_pseudo_class(parser, input, state, Component::NthLastOfType), + "is" if parser.parse_is_and_where() => return parse_is_or_where(parser, input, state, Component::Is), + "where" if parser.parse_is_and_where() => return parse_is_or_where(parser, input, state, Component::Where), + "host" => return Ok(Component::Host(Some(parse_inner_compound_selector(parser, input, state)?))), "not" => { if state.intersects(SelectorParsingState::INSIDE_NEGATION) { return Err(input.new_custom_error( SelectorParseErrorKind::UnexpectedIdent("not".into()) )); } - debug_assert!(state.is_empty()); - return parse_negation(parser, input) + return parse_negation(parser, input, state) }, _ => {} } + + if !state.allows_custom_functional_pseudo_classes() { + return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); + } + P::parse_non_ts_functional_pseudo_class(parser, name, input).map(Component::NonTSPseudoClass) } -fn parse_nth_pseudo_class<'i, 't, Impl, F>( +fn parse_nth_pseudo_class<'i, 't, P, Impl, F>( + _: &P, input: &mut CssParser<'i, 't>, + state: SelectorParsingState, selector: F, -) -> Result, BasicParseError<'i>> +) -> Result, ParseError<'i, P::Error>> where + P: Parser<'i, Impl = Impl>, Impl: SelectorImpl, F: FnOnce(i32, i32) -> Component, { + if !state.allows_tree_structural_pseudo_classes() { + return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); + } let (a, b) = parse_nth(input)?; Ok(selector(a, b)) } @@ -2299,7 +2324,7 @@ where }; let is_pseudo_element = !is_single_colon || is_css2_pseudo_element(&name); if is_pseudo_element { - if state.intersects(SelectorParsingState::AFTER_PSEUDO_ELEMENT) { + if !state.allows_pseudos() { return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); } let pseudo_element = if is_functional { @@ -2326,7 +2351,7 @@ where ); } let selector = input.parse_nested_block(|input| { - parse_inner_compound_selector(parser, input) + parse_inner_compound_selector(parser, input, state) })?; return Ok(Some(SimpleSelectorParseResult::SlottedPseudo(selector))); } From a40b2b610a854ef257144a868f5dfb164d25272c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 20 May 2020 23:54:16 +0000 Subject: [PATCH 20/42] style: Fix a case where we'd allow parsing functional :host incorrectly. This is a missing check I should've introduced in bug 1632647. Differential Revision: https://phabricator.services.mozilla.com/D76161 --- components/selectors/parser.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 1548a6fdcab..74f4a48de04 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -2207,7 +2207,12 @@ where "nth-last-of-type" => return parse_nth_pseudo_class(parser, input, state, Component::NthLastOfType), "is" if parser.parse_is_and_where() => return parse_is_or_where(parser, input, state, Component::Is), "where" if parser.parse_is_and_where() => return parse_is_or_where(parser, input, state, Component::Where), - "host" => return Ok(Component::Host(Some(parse_inner_compound_selector(parser, input, state)?))), + "host" => { + if !state.allows_tree_structural_pseudo_classes() { + return Err(input.new_custom_error(SelectorParseErrorKind::InvalidState)); + } + return Ok(Component::Host(Some(parse_inner_compound_selector(parser, input, state)?))); + }, "not" => { if state.intersects(SelectorParsingState::INSIDE_NEGATION) { return Err(input.new_custom_error( From bd23e05c47913d235db5794dba54daec5ec82b36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 20 May 2020 23:53:34 +0000 Subject: [PATCH 21/42] style: Fix a no-longer valid assumption in pseudo-element matching / invalidation code. After bug 1632647, we can have pseudo-classes inside :not / :is / :where, which the invalidation and matching code weren't handling. Add a few tests for this stuff working as expected. Differential Revision: https://phabricator.services.mozilla.com/D76160 --- components/selectors/matching.rs | 15 ++++--- components/selectors/parser.rs | 41 +++++++++++++++++++ .../style/invalidation/element/invalidator.rs | 28 ++++++++----- 3 files changed, 65 insertions(+), 19 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index d8d4ad59946..f55833f7fb6 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -322,14 +322,13 @@ where }, } - // The only other parser-allowed Component in this sequence is a state - // class. We just don't match in that case. - if let Some(s) = iter.next() { - debug_assert!( - matches!(*s, Component::NonTSPseudoClass(..)), - "Someone messed up pseudo-element parsing" - ); - return false; + for component in &mut iter { + // The only other parser-allowed Components in this sequence are + // state pseudo-classes, or one of the other things that can contain + // them. + if !component.matches_for_stateless_pseudo_element() { + return false; + } } // Advance to the non-pseudo-element part of the selector. diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 74f4a48de04..e2ad8e30caf 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -1054,6 +1054,47 @@ impl Component { } } + /// Whether this component is valid after a pseudo-element. Only intended + /// for sanity-checking. + pub fn maybe_allowed_after_pseudo_element(&self) -> bool { + match *self { + Component::NonTSPseudoClass(..) => true, + Component::Negation(ref components) => components.iter().all(|c| c.maybe_allowed_after_pseudo_element()), + Component::Is(ref selectors) | + Component::Where(ref selectors) => { + selectors.iter().all(|selector| { + selector.iter_raw_match_order().all(|c| c.maybe_allowed_after_pseudo_element()) + }) + }, + _ => false, + } + } + + /// Whether a given selector should match for stateless pseudo-elements. + /// + /// This is a bit subtle: Only selectors that return true in + /// `maybe_allowed_after_pseudo_element` should end up here, and + /// `NonTSPseudoClass` never matches (as it is a stateless pseudo after + /// all). + pub(crate) fn matches_for_stateless_pseudo_element(&self) -> bool { + debug_assert!( + self.maybe_allowed_after_pseudo_element(), + "Someone messed up pseudo-element parsing: {:?}", + *self + ); + match *self { + Component::Negation(ref components) => { + !components.iter().all(|c| c.matches_for_stateless_pseudo_element()) + }, + Component::Is(ref selectors) | Component::Where(ref selectors) => { + selectors.iter().any(|selector| { + selector.iter_raw_match_order().all(|c| c.matches_for_stateless_pseudo_element()) + }) + }, + _ => false, + } + } + pub fn visit(&self, visitor: &mut V) -> bool where V: SelectorVisitor, diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 4f5d005c120..8b387bc0c04 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -871,23 +871,29 @@ where // This will usually be the very next component, except for // the fact that we store compound selectors the other way // around, so there could also be state pseudo-classes. - let pseudo_selector = next_invalidation + let pseudo = next_invalidation .dependency .selector .iter_raw_parse_order_from(next_invalidation.offset) - .skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..))) + .flat_map(|c| { + if let Component::PseudoElement(ref pseudo) = *c { + return Some(pseudo); + } + + // TODO: Would be nice to make this a diagnostic_assert! of + // sorts. + debug_assert!( + c.maybe_allowed_after_pseudo_element(), + "Someone seriously messed up selector parsing: \ + {:?} at offset {:?}: {:?}", + next_invalidation.dependency, next_invalidation.offset, c, + ); + + None + }) .next() .unwrap(); - let pseudo = match *pseudo_selector { - Component::PseudoElement(ref pseudo) => pseudo, - _ => unreachable!( - "Someone seriously messed up selector parsing: \ - {:?} at offset {:?}: {:?}", - next_invalidation.dependency, next_invalidation.offset, pseudo_selector, - ), - }; - // FIXME(emilio): This is not ideal, and could not be // accurate if we ever have stateful element-backed eager // pseudos. From fc9321bb238f6e5a9f4c568372b276076fb36b38 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Thu, 21 May 2020 06:45:10 +0000 Subject: [PATCH 22/42] style: Let aspect-ratio (css-sizing-4) support 'auto | '. In order to test its parsing and serialization, we expose it but protect it behind a pref. Besides, I would like to drop layout.css.aspect-ratio-number.enabled in the next patch because the spec has been updated. It seems we don't have to keep this pref and we should always use Number. Differential Revision: https://phabricator.services.mozilla.com/D74955 --- components/style/lib.rs | 23 ++++++ components/style/properties/data.py | 2 +- .../properties/longhands/position.mako.rs | 14 ++-- components/style/values/computed/mod.rs | 15 +++- components/style/values/computed/position.rs | 6 +- components/style/values/generics/position.rs | 75 +++++++++++++++++++ components/style/values/specified/mod.rs | 18 ++++- components/style/values/specified/position.rs | 50 ++++++++++++- 8 files changed, 185 insertions(+), 18 deletions(-) diff --git a/components/style/lib.rs b/components/style/lib.rs index 7ec3ded3395..52b77a7507e 100644 --- a/components/style/lib.rs +++ b/components/style/lib.rs @@ -272,3 +272,26 @@ where ::is_zero(self) } } + +/// A trait pretty much similar to num_traits::One, but without the need of +/// implementing `Mul`. +pub trait One { + /// Reutrns the one value. + fn one() -> Self; + + /// Returns whether this value is one. + fn is_one(&self) -> bool; +} + +impl One for T +where + T: num_traits::One + PartialEq, +{ + fn one() -> Self { + ::one() + } + + fn is_one(&self) -> bool { + *self == One::one() + } +} diff --git a/components/style/properties/data.py b/components/style/properties/data.py index 56ac30dd458..8e6cdd79a1b 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -328,6 +328,7 @@ class Longhand(object): "AlignItems", "AlignSelf", "Appearance", + "AspectRatio", "BreakBetween", "BreakWithin", "BackgroundRepeat", @@ -364,7 +365,6 @@ class Longhand(object): "MozScriptSizeMultiplier", "TextDecorationSkipInk", "NonNegativeNumber", - "Number", "OffsetRotate", "Opacity", "OutlineStyle", diff --git a/components/style/properties/longhands/position.mako.rs b/components/style/properties/longhands/position.mako.rs index 04d735201d4..3af5f86561b 100644 --- a/components/style/properties/longhands/position.mako.rs +++ b/components/style/properties/longhands/position.mako.rs @@ -447,17 +447,13 @@ ${helpers.predefined_type( servo_restyle_damage="reflow", )} -// NOTE(emilio): Before exposing this property to content, we probably need to -// change syntax and such, and make it apply to more elements. -// -// For now, it's used only for mapped attributes. ${helpers.predefined_type( "aspect-ratio", - "Number", - "computed::Number::zero()", + "AspectRatio", + "computed::AspectRatio::auto()", engines="gecko servo-2013", - animation_value_type="ComputedValue", - spec="Internal, for now", - enabled_in="", + animation_value_type="discrete", + spec="https://drafts.csswg.org/css-sizing-4/#aspect-ratio", + gecko_pref="layout.css.aspect-ratio.enabled", servo_restyle_damage="reflow", )} diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index aba433d7443..f2d29eecac3 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -21,7 +21,7 @@ use crate::media_queries::Device; use crate::properties; use crate::properties::{ComputedValues, LonghandId, StyleBuilder}; use crate::rule_cache::RuleCacheConditions; -use crate::{ArcSlice, Atom}; +use crate::{ArcSlice, Atom, One}; use euclid::default::Size2D; use servo_arc::Arc; use std::cell::RefCell; @@ -68,6 +68,7 @@ pub use self::list::Quotes; pub use self::motion::{OffsetPath, OffsetRotate}; pub use self::outline::OutlineStyle; pub use self::percentage::{NonNegativePercentage, Percentage}; +pub use self::position::AspectRatio; pub use self::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto, ZIndex}; pub use self::rect::NonNegativeLengthOrNumberRect; pub use self::resolution::Resolution; @@ -600,6 +601,18 @@ impl From for CSSFloat { } } +impl One for NonNegativeNumber { + #[inline] + fn one() -> Self { + NonNegative(1.0) + } + + #[inline] + fn is_one(&self) -> bool { + self.0 == 1.0 + } +} + /// A wrapper of Number, but the value between 0 and 1 pub type ZeroToOneNumber = ZeroToOne; diff --git a/components/style/values/computed/position.rs b/components/style/values/computed/position.rs index 554c36295c7..6d09e327e63 100644 --- a/components/style/values/computed/position.rs +++ b/components/style/values/computed/position.rs @@ -7,7 +7,8 @@ //! //! [position]: https://drafts.csswg.org/css-backgrounds-3/#position -use crate::values::computed::{Integer, LengthPercentage, Percentage}; +use crate::values::computed::{Integer, LengthPercentage, NonNegativeNumber, Percentage}; +use crate::values::generics::position::AspectRatio as GenericAspectRatio; use crate::values::generics::position::Position as GenericPosition; use crate::values::generics::position::PositionComponent as GenericPositionComponent; use crate::values::generics::position::PositionOrAuto as GenericPositionOrAuto; @@ -68,3 +69,6 @@ impl GenericPositionComponent for LengthPercentage { /// A computed value for the `z-index` property. pub type ZIndex = GenericZIndex; + +/// A computed value for the `aspect-ratio` property. +pub type AspectRatio = GenericAspectRatio; diff --git a/components/style/values/generics/position.rs b/components/style/values/generics/position.rs index 00a9a219df4..8e2c362ef0b 100644 --- a/components/style/values/generics/position.rs +++ b/components/style/values/generics/position.rs @@ -5,6 +5,10 @@ //! Generic types for CSS handling of specified and computed values of //! [`position`](https://drafts.csswg.org/css-backgrounds-3/#position) +use crate::One; +use std::fmt::{self, Write}; +use style_traits::{CssWriter, ToCss}; + /// A generic type for representing a CSS [position](https://drafts.csswg.org/css-values/#position). #[derive( Animate, @@ -150,3 +154,74 @@ impl ZIndex { } } } + +/// A generic value for the `` value. +// FIXME: Use this for aspect-ratio in both css-sizing-4 and media-queries in the following patch. +#[derive( + Animate, + Clone, + ComputeSquaredDistance, + Copy, + Debug, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToAnimatedZero, + ToComputedValue, + ToResolvedValue, + ToShmem, +)] +#[repr(C)] +pub struct Ratio(pub N, pub N); + +impl ToCss for Ratio +where + N: ToCss + One + std::cmp::PartialEq, +{ + fn to_css(&self, dest: &mut CssWriter) -> fmt::Result + where + W: Write, + { + self.0.to_css(dest)?; + // The second defaults to 1. So if it is one, we omit it in serialization. + if !self.1.is_one() { + dest.write_str(" / ")?; + self.1.to_css(dest)?; + } + Ok(()) + } +} + +/// A generic value for the `aspect-ratio` property. +#[derive( + Animate, + Clone, + ComputeSquaredDistance, + Copy, + Debug, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToAnimatedZero, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +#[repr(C, u8)] +pub enum GenericAspectRatio { + /// The value. + Ratio(#[css(field_bound)] Ratio), + /// The keyword `auto`. + Auto, +} + +pub use self::GenericAspectRatio as AspectRatio; + +impl AspectRatio { + /// Returns `auto` + #[inline] + pub fn auto() -> Self { + AspectRatio::Auto + } +} diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index c1c11d5f816..8fc7dade9ec 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -18,9 +18,8 @@ use crate::context::QuirksMode; use crate::parser::{Parse, ParserContext}; use crate::values::serialize_atom_identifier; use crate::values::specified::calc::CalcNode; -use crate::{Atom, Namespace, Prefix, Zero}; +use crate::{Atom, Namespace, One, Prefix, Zero}; use cssparser::{Parser, Token}; -use num_traits::One; use std::f32; use std::fmt::{self, Write}; use std::ops::Add; @@ -72,6 +71,7 @@ pub use self::list::Quotes; pub use self::motion::{OffsetPath, OffsetRotate}; pub use self::outline::OutlineStyle; pub use self::percentage::Percentage; +pub use self::position::AspectRatio; pub use self::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto}; pub use self::position::{PositionComponent, ZIndex}; pub use self::rect::NonNegativeLengthOrNumberRect; @@ -375,6 +375,18 @@ impl Parse for NonNegativeNumber { } } +impl One for NonNegativeNumber { + #[inline] + fn one() -> Self { + NonNegativeNumber::new(1.0) + } + + #[inline] + fn is_one(&self) -> bool { + self.0.get() == 1.0 + } +} + impl NonNegativeNumber { /// Returns a new non-negative number with the value `val`. pub fn new(val: CSSFloat) -> Self { @@ -537,7 +549,7 @@ impl Zero for Integer { } } -impl One for Integer { +impl num_traits::One for Integer { #[inline] fn one() -> Self { Self::new(1) diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index 33b9ddcf5b5..cd1dacc8f26 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -12,13 +12,14 @@ use crate::selector_map::PrecomputedHashMap; use crate::str::HTML_SPACE_CHARACTERS; use crate::values::computed::LengthPercentage as ComputedLengthPercentage; use crate::values::computed::{Context, Percentage, ToComputedValue}; +use crate::values::generics::position::AspectRatio as GenericAspectRatio; use crate::values::generics::position::Position as GenericPosition; use crate::values::generics::position::PositionComponent as GenericPositionComponent; use crate::values::generics::position::PositionOrAuto as GenericPositionOrAuto; +use crate::values::generics::position::Ratio as GenericRatio; use crate::values::generics::position::ZIndex as GenericZIndex; -use crate::values::specified::{AllowQuirks, Integer, LengthPercentage}; -use crate::Atom; -use crate::Zero; +use crate::values::specified::{AllowQuirks, Integer, LengthPercentage, NonNegativeNumber}; +use crate::{Atom, One, Zero}; use cssparser::Parser; use selectors::parser::SelectorParseErrorKind; use servo_arc::Arc; @@ -877,3 +878,46 @@ impl GridTemplateAreas { /// A specified value for the `z-index` property. pub type ZIndex = GenericZIndex; + +/// A specified value for the `aspect-ratio` property. +pub type AspectRatio = GenericAspectRatio; + +// FIXME: Add field_bound for parse custom derive, so we can drop this. +impl Parse for AspectRatio { + fn parse<'i, 't>( + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { + if input + .try(|input| input.expect_ident_matching("auto")) + .is_ok() + { + return Ok(AspectRatio::Auto); + } + + GenericRatio::parse(context, input).map(AspectRatio::Ratio) + } +} + +// https://drafts.csswg.org/css-values-4/#ratios +impl Parse for GenericRatio { + fn parse<'i, 't>( + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { + let a = NonNegativeNumber::parse(context, input)?; + let b = match input.try_parse(|input| input.expect_delim('/')) { + Ok(()) => NonNegativeNumber::parse(context, input)?, + _ => One::one(), + }; + + // The computed value of a is the pair of numbers provided, unless + // both numbers are zero, in which case the computed value is the pair (1, 0) + // (same as 1 / 0). + // https://drafts.csswg.org/css-values-4/#ratios + if a.is_zero() && b.is_zero() { + return Ok(GenericRatio(One::one(), Zero::zero())); + } + return Ok(GenericRatio(a, b)); + } +} From 7022f451e14c1cb09411bd1146da2816f6eac2c7 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 20 May 2020 21:13:35 +0000 Subject: [PATCH 23/42] style: Replace AspectRatio with computed::position::Ratio in media-queries. Also, we drop the pref, layout.css.aspect-ratio-number.enabled, becacuse the spec of css-sizing-4 uses Number now. Differential Revision: https://phabricator.services.mozilla.com/D75233 --- components/style/gecko/media_features.rs | 11 ++-- .../style/media_queries/media_feature.rs | 5 +- .../media_queries/media_feature_expression.rs | 53 ++++--------------- components/style/values/computed/position.rs | 21 ++++++++ components/style/values/generics/position.rs | 1 - components/style/values/specified/mod.rs | 8 ++- components/style/values/specified/position.rs | 5 +- 7 files changed, 50 insertions(+), 54 deletions(-) diff --git a/components/style/gecko/media_features.rs b/components/style/gecko/media_features.rs index e3de8dbfaaa..63d5ce10772 100644 --- a/components/style/gecko/media_features.rs +++ b/components/style/gecko/media_features.rs @@ -8,8 +8,9 @@ use crate::gecko_bindings::bindings; use crate::gecko_bindings::structs; use crate::media_queries::media_feature::{AllowsRanges, ParsingRequirements}; use crate::media_queries::media_feature::{Evaluator, MediaFeatureDescription}; -use crate::media_queries::media_feature_expression::{AspectRatio, RangeOrOperator}; +use crate::media_queries::media_feature_expression::RangeOrOperator; use crate::media_queries::{Device, MediaType}; +use crate::values::computed::position::Ratio; use crate::values::computed::CSSPixelLength; use crate::values::computed::Resolution; use crate::Atom; @@ -91,7 +92,7 @@ fn eval_device_height( fn eval_aspect_ratio_for( device: &Device, - query_value: Option, + query_value: Option, range_or_operator: Option, get_size: F, ) -> bool @@ -104,14 +105,14 @@ where }; let size = get_size(device); - let value = AspectRatio(size.width.0 as f32, size.height.0 as f32); + let value = Ratio::new(size.width.0 as f32, size.height.0 as f32); RangeOrOperator::evaluate_with_query_value(range_or_operator, query_value, value) } /// https://drafts.csswg.org/mediaqueries-4/#aspect-ratio fn eval_aspect_ratio( device: &Device, - query_value: Option, + query_value: Option, range_or_operator: Option, ) -> bool { eval_aspect_ratio_for(device, query_value, range_or_operator, viewport_size) @@ -120,7 +121,7 @@ fn eval_aspect_ratio( /// https://drafts.csswg.org/mediaqueries-4/#device-aspect-ratio fn eval_device_aspect_ratio( device: &Device, - query_value: Option, + query_value: Option, range_or_operator: Option, ) -> bool { eval_aspect_ratio_for(device, query_value, range_or_operator, device_size) diff --git a/components/style/media_queries/media_feature.rs b/components/style/media_queries/media_feature.rs index a273e7f51be..cad695413c3 100644 --- a/components/style/media_queries/media_feature.rs +++ b/components/style/media_queries/media_feature.rs @@ -4,9 +4,10 @@ //! Media features. -use super::media_feature_expression::{AspectRatio, RangeOrOperator}; +use super::media_feature_expression::RangeOrOperator; use super::Device; use crate::parser::ParserContext; +use crate::values::computed::position::Ratio; use crate::values::computed::{CSSPixelLength, Resolution}; use crate::Atom; use cssparser::Parser; @@ -45,7 +46,7 @@ pub enum Evaluator { Float(MediaFeatureEvaluator), BoolInteger(MediaFeatureEvaluator), /// A non-negative number ratio, such as the one from device-pixel-ratio. - NumberRatio(MediaFeatureEvaluator), + NumberRatio(MediaFeatureEvaluator), /// A resolution. Resolution(MediaFeatureEvaluator), /// A keyword value. diff --git a/components/style/media_queries/media_feature_expression.rs b/components/style/media_queries/media_feature_expression.rs index 4704fab0ffc..9e2fdd2d47b 100644 --- a/components/style/media_queries/media_feature_expression.rs +++ b/components/style/media_queries/media_feature_expression.rs @@ -15,9 +15,8 @@ use crate::parser::{Parse, ParserContext}; #[cfg(feature = "servo")] use crate::servo::media_queries::MEDIA_FEATURES; use crate::str::{starts_with_ignore_ascii_case, string_as_ascii_lowercase}; +use crate::values::computed::position::Ratio; use crate::values::computed::{self, ToComputedValue}; -#[cfg(feature = "gecko")] -use crate::values::specified::NonNegativeNumber; use crate::values::specified::{Integer, Length, Number, Resolution}; use crate::values::{serialize_atom_identifier, CSSFloat}; use crate::{Atom, Zero}; @@ -26,30 +25,6 @@ use std::cmp::{Ordering, PartialOrd}; use std::fmt::{self, Write}; use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss}; -/// An aspect ratio, with a numerator and denominator. -#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToShmem)] -pub struct AspectRatio(pub CSSFloat, pub CSSFloat); - -impl ToCss for AspectRatio { - fn to_css(&self, dest: &mut CssWriter) -> fmt::Result - where - W: fmt::Write, - { - self.0.to_css(dest)?; - dest.write_str(" / ")?; - self.1.to_css(dest) - } -} - -impl PartialOrd for AspectRatio { - fn partial_cmp(&self, other: &AspectRatio) -> Option { - f64::partial_cmp( - &(self.0 as f64 * other.1 as f64), - &(self.1 as f64 * other.0 as f64), - ) - } -} - /// The kind of matching that should be performed on a media feature value. #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToShmem)] pub enum Range { @@ -460,7 +435,7 @@ pub enum MediaExpressionValue { BoolInteger(bool), /// A single non-negative number or two non-negative numbers separated by '/', /// with optional whitespace on either side of the '/'. - NumberRatio(AspectRatio), + NumberRatio(Ratio), /// A resolution. Resolution(Resolution), /// An enumerated value, defined by the variant keyword table in the @@ -517,24 +492,14 @@ impl MediaExpressionValue { MediaExpressionValue::Float(number.get()) }, Evaluator::NumberRatio(..) => { - #[cfg(feature = "gecko")] - { - if static_prefs::pref!("layout.css.aspect-ratio-number.enabled") { - let a = NonNegativeNumber::parse(context, input)?.0.get(); - let b = match input.try_parse(|input| input.expect_delim('/')) { - Ok(()) => NonNegativeNumber::parse(context, input)?.0.get(), - _ => 1.0, - }; - return Ok(MediaExpressionValue::NumberRatio(AspectRatio(a, b))); - } - } + use crate::values::generics::position::Ratio as GenericRatio; + use crate::values::generics::NonNegative; + use crate::values::specified::position::Ratio; - let a = Integer::parse_positive(context, input)?; - input.expect_delim('/')?; - let b = Integer::parse_positive(context, input)?; - MediaExpressionValue::NumberRatio(AspectRatio( - a.value() as CSSFloat, - b.value() as CSSFloat, + let ratio = Ratio::parse(context, input)?; + MediaExpressionValue::NumberRatio(GenericRatio( + NonNegative(ratio.0.get()), + NonNegative(ratio.1.get()), )) }, Evaluator::Resolution(..) => { diff --git a/components/style/values/computed/position.rs b/components/style/values/computed/position.rs index 6d09e327e63..9e5fe3be1d2 100644 --- a/components/style/values/computed/position.rs +++ b/components/style/values/computed/position.rs @@ -12,9 +12,11 @@ use crate::values::generics::position::AspectRatio as GenericAspectRatio; use crate::values::generics::position::Position as GenericPosition; use crate::values::generics::position::PositionComponent as GenericPositionComponent; use crate::values::generics::position::PositionOrAuto as GenericPositionOrAuto; +use crate::values::generics::position::Ratio as GenericRatio; use crate::values::generics::position::ZIndex as GenericZIndex; pub use crate::values::specified::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow}; use crate::Zero; +use std::cmp::{Ordering, PartialOrd}; use std::fmt::{self, Write}; use style_traits::{CssWriter, ToCss}; @@ -70,5 +72,24 @@ impl GenericPositionComponent for LengthPercentage { /// A computed value for the `z-index` property. pub type ZIndex = GenericZIndex; +/// A computed value. +pub type Ratio = GenericRatio; + +impl PartialOrd for Ratio { + fn partial_cmp(&self, other: &Self) -> Option { + f64::partial_cmp( + &((self.0).0 as f64 * (other.1).0 as f64), + &((self.1).0 as f64 * (other.0).0 as f64), + ) + } +} + +impl Ratio { + /// Returns a new Ratio. + pub fn new(a: f32, b: f32) -> Self { + GenericRatio(a.into(), b.into()) + } +} + /// A computed value for the `aspect-ratio` property. pub type AspectRatio = GenericAspectRatio; diff --git a/components/style/values/generics/position.rs b/components/style/values/generics/position.rs index 8e2c362ef0b..5f1659d3917 100644 --- a/components/style/values/generics/position.rs +++ b/components/style/values/generics/position.rs @@ -156,7 +156,6 @@ impl ZIndex { } /// A generic value for the `` value. -// FIXME: Use this for aspect-ratio in both css-sizing-4 and media-queries in the following patch. #[derive( Animate, Clone, diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index 8fc7dade9ec..e9d25330f64 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -383,7 +383,7 @@ impl One for NonNegativeNumber { #[inline] fn is_one(&self) -> bool { - self.0.get() == 1.0 + self.get() == 1.0 } } @@ -392,6 +392,12 @@ impl NonNegativeNumber { pub fn new(val: CSSFloat) -> Self { NonNegative::(Number::new(val.max(0.))) } + + /// Returns the numeric value. + #[inline] + pub fn get(&self) -> f32 { + self.0.get() + } } /// A Number which is >= 1.0. diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index cd1dacc8f26..76fa23152e5 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -899,8 +899,11 @@ impl Parse for AspectRatio { } } +/// A specified value for the `aspect-ratio` property. +pub type Ratio = GenericRatio; + // https://drafts.csswg.org/css-values-4/#ratios -impl Parse for GenericRatio { +impl Parse for Ratio { fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, From 35546aea54e73ef1428d6921c0ec059a7250d526 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 20 May 2020 21:13:37 +0000 Subject: [PATCH 24/42] style: Use style::One for Integer to avoid implementing Mul. Differential Revision: https://phabricator.services.mozilla.com/D76207 --- components/style/values/generics/font.rs | 4 ++-- components/style/values/specified/mod.rs | 12 ++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/components/style/values/generics/font.rs b/components/style/values/generics/font.rs index 2b29104ff81..237e862d4aa 100644 --- a/components/style/values/generics/font.rs +++ b/components/style/values/generics/font.rs @@ -5,9 +5,9 @@ //! Generic types for font stuff. use crate::parser::{Parse, ParserContext}; +use crate::One; use byteorder::{BigEndian, ReadBytesExt}; use cssparser::Parser; -use num_traits::One; use std::fmt::{self, Write}; use std::io::Cursor; use style_traits::{CssWriter, ParseError}; @@ -42,7 +42,7 @@ where { self.tag.to_css(dest)?; // Don't serialize the default value. - if self.value != Integer::one() { + if !self.value.is_one() { dest.write_char(' ')?; self.value.to_css(dest)?; } diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index e9d25330f64..e45be0513a4 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -555,19 +555,15 @@ impl Zero for Integer { } } -impl num_traits::One for Integer { +impl One for Integer { #[inline] fn one() -> Self { Self::new(1) } -} -// This is not great, because it loses calc-ness, but it's necessary for One. -impl ::std::ops::Mul for Integer { - type Output = Self; - - fn mul(self, other: Self) -> Self { - Self::new(self.value * other.value) + #[inline] + fn is_one(&self) -> bool { + self.value() == 1 } } From 66185e81f66c59dc1d26cbf91dc78bb3701f2b94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 21 May 2020 13:48:36 +0000 Subject: [PATCH 25/42] style: Support field_bound in #[derive(Parse)]. Differential Revision: https://phabricator.services.mozilla.com/D76268 --- components/style_derive/parse.rs | 41 ++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/components/style_derive/parse.rs b/components/style_derive/parse.rs index 80c214bd734..6dc2c8a715b 100644 --- a/components/style_derive/parse.rs +++ b/components/style_derive/parse.rs @@ -15,7 +15,14 @@ pub struct ParseVariantAttrs { pub condition: Option, } +#[darling(attributes(parse), default)] +#[derive(Default, FromField)] +pub struct ParseFieldAttrs { + field_bound: bool, +} + fn parse_non_keyword_variant( + where_clause: &mut Option, name: &syn::Ident, variant: &VariantInfo, variant_attrs: &CssVariantAttrs, @@ -32,7 +39,14 @@ fn parse_non_keyword_variant( "We only support deriving parse for simple variants" ); let variant_name = &variant.ast().ident; - let ty = &bindings[0].ast().ty; + let binding_ast = &bindings[0].ast(); + let ty = &binding_ast.ty; + + let field_attrs = cg::parse_field_attrs::(binding_ast); + if field_attrs.field_bound { + cg::add_predicate(where_clause, parse_quote!(#ty: crate::parser::Parse)); + } + let mut parse = if skip_try { quote! { let v = <#ty as crate::parser::Parse>::parse(context, input)?; @@ -67,15 +81,12 @@ fn parse_non_keyword_variant( } pub fn derive(mut input: DeriveInput) -> TokenStream { - { - let mut where_clause = input.generics.where_clause.take(); - for param in input.generics.type_params() { - cg::add_predicate( - &mut where_clause, - parse_quote!(#param: crate::parser::Parse), - ); - } - input.generics.where_clause = where_clause; + let mut where_clause = input.generics.where_clause.take(); + for param in input.generics.type_params() { + cg::add_predicate( + &mut where_clause, + parse_quote!(#param: crate::parser::Parse), + ); } let name = &input.ident; @@ -88,12 +99,13 @@ pub fn derive(mut input: DeriveInput) -> TokenStream { let mut effective_variants = 0; for variant in s.variants().iter() { let css_variant_attrs = cg::parse_variant_attrs_from_ast::(&variant.ast()); - let parse_attrs = cg::parse_variant_attrs_from_ast::(&variant.ast()); if css_variant_attrs.skip { continue; } effective_variants += 1; + let parse_attrs = cg::parse_variant_attrs_from_ast::(&variant.ast()); + saw_condition |= parse_attrs.condition.is_some(); if !variant.bindings().is_empty() { @@ -143,7 +155,7 @@ pub fn derive(mut input: DeriveInput) -> TokenStream { for (i, (variant, css_attrs, parse_attrs)) in non_keywords.iter().enumerate() { let skip_try = !has_keywords && i == non_keywords.len() - 1; let parse_variant = - parse_non_keyword_variant(name, variant, css_attrs, parse_attrs, skip_try); + parse_non_keyword_variant(&mut where_clause, name, variant, css_attrs, parse_attrs, skip_try); parse_non_keywords.extend(parse_variant); } @@ -171,6 +183,9 @@ pub fn derive(mut input: DeriveInput) -> TokenStream { quote! { Self::parse(input) } }; + let has_non_keywords = !non_keywords.is_empty(); + + input.generics.where_clause = where_clause; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); let parse_trait_impl = quote! { @@ -189,7 +204,7 @@ pub fn derive(mut input: DeriveInput) -> TokenStream { return parse_trait_impl; } - assert!(non_keywords.is_empty()); + assert!(!has_non_keywords); // TODO(emilio): It'd be nice to get rid of these, but that makes the // conversion harder... From 396338816d197ae110be8ca73d6521bc3d9dca7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 21 May 2020 16:52:20 +0000 Subject: [PATCH 26/42] style: Clean up parsing of UnicodeRange. Differential Revision: https://phabricator.services.mozilla.com/D76330 --- components/style/parser.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/components/style/parser.rs b/components/style/parser.rs index c6f7321b25d..d1afc2e96c8 100644 --- a/components/style/parser.rs +++ b/components/style/parser.rs @@ -208,10 +208,7 @@ where } impl Parse for UnicodeRange { - fn parse<'i, 't>( - _context: &ParserContext, - input: &mut Parser<'i, 't>, - ) -> Result> { - UnicodeRange::parse(input).map_err(|e| e.into()) + fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { + Ok(UnicodeRange::parse(input)?) } } From e259c53c62815725c65ebfe6d151cec991ef1c31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 21 May 2020 18:44:19 +0000 Subject: [PATCH 27/42] style: Derive parse for TextOverflowSide. Depends on D76330 Differential Revision: https://phabricator.services.mozilla.com/D76331 --- components/style/parser.rs | 6 ++++++ components/style/values/specified/text.rs | 25 +---------------------- 2 files changed, 7 insertions(+), 24 deletions(-) diff --git a/components/style/parser.rs b/components/style/parser.rs index d1afc2e96c8..f506b809955 100644 --- a/components/style/parser.rs +++ b/components/style/parser.rs @@ -207,6 +207,12 @@ where } } +impl Parse for crate::OwnedStr { + fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { + Ok(input.expect_string()?.as_ref().to_owned().into()) + } +} + impl Parse for UnicodeRange { fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { Ok(UnicodeRange::parse(input)?) diff --git a/components/style/values/specified/text.rs b/components/style/values/specified/text.rs index 703e51a5248..bee3c665840 100644 --- a/components/style/values/specified/text.rs +++ b/components/style/values/specified/text.rs @@ -128,6 +128,7 @@ impl ToComputedValue for LineHeight { Eq, MallocSizeOf, PartialEq, + Parse, SpecifiedValueInfo, ToComputedValue, ToCss, @@ -144,30 +145,6 @@ pub enum TextOverflowSide { String(crate::OwnedStr), } -impl Parse for TextOverflowSide { - fn parse<'i, 't>( - _context: &ParserContext, - input: &mut Parser<'i, 't>, - ) -> Result> { - let location = input.current_source_location(); - match *input.next()? { - Token::Ident(ref ident) => { - match_ignore_ascii_case! { ident, - "clip" => Ok(TextOverflowSide::Clip), - "ellipsis" => Ok(TextOverflowSide::Ellipsis), - _ => Err(location.new_custom_error( - SelectorParseErrorKind::UnexpectedIdent(ident.clone()) - )) - } - }, - Token::QuotedString(ref v) => { - Ok(TextOverflowSide::String(v.as_ref().to_owned().into())) - }, - ref t => Err(location.new_unexpected_token_error(t.clone())), - } - } -} - #[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss, ToShmem)] /// text-overflow. Specifies rendering when inline content overflows its line box edge. pub struct TextOverflow { From 224550f818b60da6eec744db625d0f2fdcccf779 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 21 May 2020 18:43:12 +0000 Subject: [PATCH 28/42] style: Derive parse for ShapeRadius. Depends on D76331 Differential Revision: https://phabricator.services.mozilla.com/D76332 --- components/style/values/generics/basic_shape.rs | 1 + components/style/values/specified/basic_shape.rs | 16 ---------------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/components/style/values/generics/basic_shape.rs b/components/style/values/generics/basic_shape.rs index 745d6e07bbf..60619b46e2c 100644 --- a/components/style/values/generics/basic_shape.rs +++ b/components/style/values/generics/basic_shape.rs @@ -270,6 +270,7 @@ pub struct Ellipse { Copy, Debug, MallocSizeOf, + Parse, PartialEq, SpecifiedValueInfo, ToAnimatedValue, diff --git a/components/style/values/specified/basic_shape.rs b/components/style/values/specified/basic_shape.rs index b1782c5b294..135ac85d330 100644 --- a/components/style/values/specified/basic_shape.rs +++ b/components/style/values/specified/basic_shape.rs @@ -263,22 +263,6 @@ impl Ellipse { } } -impl Parse for ShapeRadius { - fn parse<'i, 't>( - context: &ParserContext, - input: &mut Parser<'i, 't>, - ) -> Result> { - if let Ok(lp) = input.try(|i| NonNegativeLengthPercentage::parse(context, i)) { - return Ok(generic::ShapeRadius::Length(lp)); - } - - try_match_ident_ignore_ascii_case! { input, - "closest-side" => Ok(generic::ShapeRadius::ClosestSide), - "farthest-side" => Ok(generic::ShapeRadius::FarthestSide), - } - } -} - impl Parse for Polygon { fn parse<'i, 't>( context: &ParserContext, From 1fcc00a11ae701541017b76d8ec8b0398da8bb7f Mon Sep 17 00:00:00 2001 From: Mats Palmgren Date: Thu, 21 May 2020 21:13:10 +0000 Subject: [PATCH 29/42] style: Allow 'opacity' on ::first-letter/::first-line pseudos. Differential Revision: https://phabricator.services.mozilla.com/D76387 --- components/style/properties/data.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/style/properties/data.py b/components/style/properties/data.py index 8e6cdd79a1b..9ab3a40ca66 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -688,6 +688,7 @@ class PropertyRestrictions: def first_letter(data): props = set([ "color", + "opacity", "float", "initial-letter", @@ -722,6 +723,7 @@ class PropertyRestrictions: props = set([ # Per spec. "color", + "opacity", # Kinda like css-fonts? "-moz-osx-font-smoothing", From 54d869c1115afa571067dd107318b4aee8a73b7f Mon Sep 17 00:00:00 2001 From: sefeng Date: Sat, 23 May 2020 01:45:33 +0000 Subject: [PATCH 30/42] style: Push/Pop dialog to top layer when needed. This patch completes the top layer requirement for showModal() Spec: https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-showmodal Differential Revision: https://phabricator.services.mozilla.com/D74922 --- components/style/element_state.rs | 4 ++++ components/style/gecko/non_ts_pseudo_class_list.rs | 1 + components/style/gecko/wrapper.rs | 1 + 3 files changed, 6 insertions(+) diff --git a/components/style/element_state.rs b/components/style/element_state.rs index 190c1cc3eb5..3fe94f3e882 100644 --- a/components/style/element_state.rs +++ b/components/style/element_state.rs @@ -141,6 +141,10 @@ bitflags! { /// /// https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo const IN_FOCUS_VISIBLE_STATE = 1 << 52; + /// State that dialog element is modal, for centered alignment + /// + /// https://html.spec.whatwg.org/#centered-alignment + const IN_MODAL_DIALOG_STATE = 1 << 53; } } diff --git a/components/style/gecko/non_ts_pseudo_class_list.rs b/components/style/gecko/non_ts_pseudo_class_list.rs index b82818b9bae..7b737f4d9cf 100644 --- a/components/style/gecko/non_ts_pseudo_class_list.rs +++ b/components/style/gecko/non_ts_pseudo_class_list.rs @@ -50,6 +50,7 @@ macro_rules! apply_non_ts_list { ("-moz-devtools-highlighted", MozDevtoolsHighlighted, IN_DEVTOOLS_HIGHLIGHTED_STATE, PSEUDO_CLASS_ENABLED_IN_UA_SHEETS), ("-moz-styleeditor-transitioning", MozStyleeditorTransitioning, IN_STYLEEDITOR_TRANSITIONING_STATE, PSEUDO_CLASS_ENABLED_IN_UA_SHEETS), ("fullscreen", Fullscreen, IN_FULLSCREEN_STATE, _), + ("-moz-modal-dialog", MozModalDialog, IN_MODAL_DIALOG_STATE, PSEUDO_CLASS_ENABLED_IN_UA_SHEETS), // TODO(emilio): This is inconsistently named (the capital R). ("-moz-focusring", MozFocusRing, IN_FOCUSRING_STATE, _), ("-moz-broken", MozBroken, IN_BROKEN_STATE, _), diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 0fbaf5f0aba..62af71f212b 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -2062,6 +2062,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { NonTSPseudoClass::MozDirAttrRTL | NonTSPseudoClass::MozDirAttrLikeAuto | NonTSPseudoClass::MozAutofill | + NonTSPseudoClass::MozModalDialog | NonTSPseudoClass::Active | NonTSPseudoClass::Hover | NonTSPseudoClass::MozAutofillPreview => { From 0c8865b8e18f8ee42dd00e1aca983dae74e36a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 21 May 2020 12:10:34 +0000 Subject: [PATCH 31/42] style: Enable :is() and :where() in UA sheets. This will allow us to clean them up. Differential Revision: https://phabricator.services.mozilla.com/D76262 --- components/style/gecko/selector_parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index adb410908b9..a46068d923a 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -354,7 +354,7 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { #[inline] fn parse_is_and_where(&self) -> bool { - static_prefs::pref!("layout.css.is-where-selectors.enabled") + self.in_user_agent_stylesheet() || static_prefs::pref!("layout.css.is-where-selectors.enabled") } #[inline] From ab79cc0e39783c41e39307b46e71306cdfc90907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 25 May 2020 23:54:10 +0000 Subject: [PATCH 32/42] style: Implement the ::file-chooser-button pseudo-element. As per https://github.com/w3c/csswg-drafts/issues/5049. Don't enable it unconditionally just yet, as the name may change. I had to move some rules in forms.css because otherwise you get specificity conflicts. Differential Revision: https://phabricator.services.mozilla.com/D76214 --- components/style/gecko/pseudo_element.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/components/style/gecko/pseudo_element.rs b/components/style/gecko/pseudo_element.rs index b537295f785..631172d9511 100644 --- a/components/style/gecko/pseudo_element.rs +++ b/components/style/gecko/pseudo_element.rs @@ -37,7 +37,8 @@ impl ::selectors::parser::PseudoElement for PseudoElement { PseudoElement::Before | PseudoElement::After | PseudoElement::Marker | - PseudoElement::Placeholder + PseudoElement::Placeholder | + PseudoElement::FileChooserButton ) } @@ -159,12 +160,12 @@ impl PseudoElement { /// Whether this pseudo-element is enabled for all content. pub fn enabled_in_content(&self) -> bool { - if (self.flags() & structs::CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS_AND_CHROME) != 0 { - return false; - } match *self { PseudoElement::MozFocusOuter => static_prefs::pref!("layout.css.moz-focus-outer.enabled"), - _ => true, + PseudoElement::FileChooserButton => static_prefs::pref!("layout.css.file-chooser-button.enabled"), + _ => { + (self.flags() & structs::CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS_AND_CHROME) == 0 + } } } From 3f25ac6b51ecb6081aabb0fb34acd1a39fd5971b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 26 May 2020 09:39:33 +0000 Subject: [PATCH 33/42] style: Remove the @-moz-document url-prefix() hack preference, enable it everywhere. It doesn't seem like realistically we're going to be able to get rid of this any time soon. Differential Revision: https://phabricator.services.mozilla.com/D76809 --- components/style/stylesheets/document_rule.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/components/style/stylesheets/document_rule.rs b/components/style/stylesheets/document_rule.rs index e779863f031..35c6b443fe2 100644 --- a/components/style/stylesheets/document_rule.rs +++ b/components/style/stylesheets/document_rule.rs @@ -264,10 +264,6 @@ impl DocumentCondition { return true; } - if !pref!("layout.css.moz-document.url-prefix-hack.enabled") { - return false; - } - // Allow a single url-prefix() for compatibility. // // See bug 1446470 and dependencies. From a9c88729a8fd87fce4fc9336695bf767fa62c634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 26 May 2020 21:58:25 +0000 Subject: [PATCH 34/42] style: Remove two useless mem::replace calls. Differential Revision: https://phabricator.services.mozilla.com/D76884 --- components/style/values/generics/calc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/style/values/generics/calc.rs b/components/style/values/generics/calc.rs index dd77fd28552..cd331818386 100644 --- a/components/style/values/generics/calc.rs +++ b/components/style/values/generics/calc.rs @@ -337,7 +337,7 @@ impl CalcNode { ($slot:expr) => {{ let dummy = Self::MinMax(Default::default(), MinMaxOp::Max); let result = mem::replace($slot, dummy); - let _ = mem::replace(self, result); + *self = result; }}; } match *self { @@ -464,7 +464,7 @@ impl CalcNode { replace_self_with!(&mut children[0]); } else { // Else put our simplified children back. - let _ = mem::replace(children_slot, children.into_boxed_slice().into()); + *children_slot = children.into_boxed_slice().into(); } }, Self::Leaf(ref mut l) => { From 4cf9aeeaf775989afee836ff46543473ccc441fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 27 May 2020 00:00:52 +0000 Subject: [PATCH 35/42] style: Manually tweak inlining in stateless pseudo selector matching. This addresses a minor regression in bloom-matching.html. The common case here is that there's no selector to the right of the pseudo-element, so keep that path inline, while keeping all other checks out of line. Differential Revision: https://phabricator.services.mozilla.com/D76793 --- components/selectors/matching.rs | 9 ++------- components/selectors/parser.rs | 29 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index f55833f7fb6..3414f9d6efa 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -322,13 +322,8 @@ where }, } - for component in &mut iter { - // The only other parser-allowed Components in this sequence are - // state pseudo-classes, or one of the other things that can contain - // them. - if !component.matches_for_stateless_pseudo_element() { - return false; - } + if !iter.matches_for_stateless_pseudo_element() { + return false; } // Advance to the non-pseudo-element part of the selector. diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index e2ad8e30caf..e66fa75c587 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -802,6 +802,33 @@ impl<'a, Impl: 'a + SelectorImpl> SelectorIter<'a, Impl> { self.next_sequence().is_none() } + #[inline] + pub(crate) fn matches_for_stateless_pseudo_element(&mut self) -> bool { + let first = match self.next() { + Some(c) => c, + // Note that this is the common path that we keep inline: the + // pseudo-element not having anything to its right. + None => return true, + }; + self.matches_for_stateless_pseudo_element_internal(first) + } + + #[inline(never)] + fn matches_for_stateless_pseudo_element_internal(&mut self, first: &Component) -> bool { + if !first.matches_for_stateless_pseudo_element() { + return false; + } + for component in self { + // The only other parser-allowed Components in this sequence are + // state pseudo-classes, or one of the other things that can contain + // them. + if !component.matches_for_stateless_pseudo_element() { + return false; + } + } + true + } + /// Returns remaining count of the simple selectors and combinators in the Selector. #[inline] pub fn selector_length(&self) -> usize { @@ -1076,7 +1103,7 @@ impl Component { /// `maybe_allowed_after_pseudo_element` should end up here, and /// `NonTSPseudoClass` never matches (as it is a stateless pseudo after /// all). - pub(crate) fn matches_for_stateless_pseudo_element(&self) -> bool { + fn matches_for_stateless_pseudo_element(&self) -> bool { debug_assert!( self.maybe_allowed_after_pseudo_element(), "Someone messed up pseudo-element parsing: {:?}", From 964716f72a9e5a1c823670cdb059914c01849a41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 27 May 2020 09:17:47 +0000 Subject: [PATCH 36/42] style: Finer grained invalidation for attribute changes. This should help out quite a bit with uBO, which has lots of very general attribute selectors. We invalidate per attribute name rather than using a SelectorMap, which prevents matching for attribute selectors that can't have changed. The idea is that this should be generally cheaper, though there are cases where this would be a slight pesimization. For example, if there's an attribute selector like: my-specific-element[my-attribute] { /* ... */ } And you change `my-attribute` in an element that isn't a `my-specific-element`, before that the SelectorMap would've prevented us from selector-matching completely. Now we'd still run selector-matching for that (though the matching would be pretty cheap). However I think this should speed up things generally, let's see what the perf tests think before landing this though. Differential Revision: https://phabricator.services.mozilla.com/D76825 --- components/style/gecko/selector_parser.rs | 11 -- components/style/gecko/snapshot.rs | 12 +- .../invalidation/element/invalidation_map.rs | 111 ++++++++---------- .../element/state_and_attributes.rs | 41 ++----- components/style/servo/selector_parser.rs | 6 - 5 files changed, 68 insertions(+), 113 deletions(-) diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index a46068d923a..e5d332df689 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -229,17 +229,6 @@ impl NonTSPseudoClass { NonTSPseudoClass::MozLWThemeDarkText ) } - - /// Returns true if the evaluation of the pseudo-class depends on the - /// element's attributes. - pub fn is_attr_based(&self) -> bool { - matches!( - *self, - NonTSPseudoClass::MozTableBorderNonzero | - NonTSPseudoClass::MozBrowserFrame | - NonTSPseudoClass::Lang(..) - ) - } } impl ::selectors::parser::NonTSPseudoClass for NonTSPseudoClass { diff --git a/components/style/gecko/snapshot.rs b/components/style/gecko/snapshot.rs index b2a66f709e2..26b3e1393dd 100644 --- a/components/style/gecko/snapshot.rs +++ b/components/style/gecko/snapshot.rs @@ -70,11 +70,15 @@ impl GeckoElementSnapshot { self.mClassAttributeChanged() } - /// Returns true if the snapshot recorded an attribute change which isn't a - /// class / id + /// Executes the callback once for each attribute that changed. #[inline] - pub fn other_attr_changed(&self) -> bool { - self.mOtherAttributeChanged() + pub fn each_attr_changed(&self, mut callback: F) + where + F: FnMut(&Atom), + { + for attr in self.mChangedAttrNames.iter() { + unsafe { Atom::with(attr.mRawPtr, &mut callback) } + } } /// selectors::Element::attr_matches diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index b760d865aa2..59143dd0025 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -6,7 +6,7 @@ use crate::context::QuirksMode; use crate::element_state::{DocumentState, ElementState}; -use crate::selector_map::{MaybeCaseInsensitiveHashMap, SelectorMap, SelectorMapEntry}; +use crate::selector_map::{MaybeCaseInsensitiveHashMap, PrecomputedHashMap, SelectorMap, SelectorMapEntry}; use crate::selector_parser::SelectorImpl; use crate::{Atom, LocalName, Namespace}; use fallible::FallibleVec; @@ -175,19 +175,6 @@ pub struct DocumentStateDependency { pub state: DocumentState, } -bitflags! { - /// A set of flags that denote whether any invalidations have occurred - /// for a particular attribute selector. - #[derive(MallocSizeOf)] - #[repr(C)] - pub struct InvalidationMapFlags : u8 { - /// Whether [class] or such is used. - const HAS_CLASS_ATTR_SELECTOR = 1 << 0; - /// Whether [id] or such is used. - const HAS_ID_ATTR_SELECTOR = 1 << 1; - } -} - /// A map where we store invalidations. /// /// This is slightly different to a SelectorMap, in the sense of that the same @@ -209,10 +196,7 @@ pub struct InvalidationMap { /// A list of document state dependencies in the rules we represent. pub document_state_selectors: Vec, /// A map of other attribute affecting selectors. - pub other_attribute_affecting_selectors: SelectorMap, - /// A set of flags that contain whether various special attributes are used - /// in this invalidation map. - pub flags: InvalidationMapFlags, + pub other_attribute_affecting_selectors: PrecomputedHashMap>, } impl InvalidationMap { @@ -223,8 +207,7 @@ impl InvalidationMap { id_to_selector: MaybeCaseInsensitiveHashMap::new(), state_affecting_selectors: SelectorMap::new(), document_state_selectors: Vec::new(), - other_attribute_affecting_selectors: SelectorMap::new(), - flags: InvalidationMapFlags::empty(), + other_attribute_affecting_selectors: PrecomputedHashMap::default(), } } @@ -232,7 +215,9 @@ impl InvalidationMap { pub fn len(&self) -> usize { self.state_affecting_selectors.len() + self.document_state_selectors.len() + - self.other_attribute_affecting_selectors.len() + + self.other_attribute_affecting_selectors + .iter() + .fold(0, |accum, (_, ref v)| accum + v.len()) + self.id_to_selector .iter() .fold(0, |accum, (_, ref v)| accum + v.len()) + @@ -248,7 +233,6 @@ impl InvalidationMap { self.state_affecting_selectors.clear(); self.document_state_selectors.clear(); self.other_attribute_affecting_selectors.clear(); - self.flags = InvalidationMapFlags::empty(); } /// Adds a selector to this `InvalidationMap`. Returns Err(..) to @@ -300,9 +284,6 @@ struct PerCompoundState { /// The state this compound selector is affected by. element_state: ElementState, - - /// Whether we've added a generic attribute dependency for this selector. - added_attr_dependency: bool, } impl PerCompoundState { @@ -310,7 +291,6 @@ impl PerCompoundState { Self { offset, element_state: ElementState::empty(), - added_attr_dependency: false, } } } @@ -342,7 +322,7 @@ struct SelectorDependencyCollector<'a> { compound_state: PerCompoundState, /// The allocation error, if we OOM. - alloc_error: &'a mut Option, + alloc_error: &'a mut Option, } impl<'a> SelectorDependencyCollector<'a> { @@ -387,20 +367,25 @@ impl<'a> SelectorDependencyCollector<'a> { } } - fn add_attr_dependency(&mut self) -> bool { - debug_assert!(!self.compound_state.added_attr_dependency); - self.compound_state.added_attr_dependency = true; - + fn add_attr_dependency(&mut self, name: LocalName) -> bool { let dependency = self.dependency(); - let result = self.map.other_attribute_affecting_selectors.insert( - dependency, - self.quirks_mode, - ); - if let Err(alloc_error) = result { - *self.alloc_error = Some(alloc_error); - return false; + + let map = &mut self.map.other_attribute_affecting_selectors; + let entry = match map.try_entry(name) { + Ok(entry) => entry, + Err(err) => { + *self.alloc_error = Some(err); + return false; + }, + }; + + match entry.or_insert_with(SmallVec::new).try_push(dependency) { + Ok(..) => true, + Err(err) => { + *self.alloc_error = Some(err); + return false; + } } - true } fn dependency(&self) -> Dependency { @@ -494,7 +479,13 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { return false; }, }; - entry.or_insert_with(SmallVec::new).try_push(dependency).is_ok() + match entry.or_insert_with(SmallVec::new).try_push(dependency) { + Ok(..) => true, + Err(err) => { + *self.alloc_error = Some(err); + return false; + } + } }, Component::NonTSPseudoClass(ref pc) => { self.compound_state.element_state |= match *pc { @@ -504,11 +495,16 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { }; *self.document_state |= pc.document_state_flag(); - if !self.compound_state.added_attr_dependency && pc.is_attr_based() { - self.add_attr_dependency() - } else { - true - } + let attr_name = match *pc { + #[cfg(feature = "gecko")] + NonTSPseudoClass::MozTableBorderNonzero => local_name!("border"), + #[cfg(feature = "gecko")] + NonTSPseudoClass::MozBrowserFrame => local_name!("mozbrowser"), + NonTSPseudoClass::Lang(..) => local_name!("lang"), + _ => return true, + }; + + self.add_attr_dependency(attr_name) }, _ => true, } @@ -516,27 +512,18 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { fn visit_attribute_selector( &mut self, - constraint: &NamespaceConstraint<&Namespace>, - _local_name: &LocalName, + _: &NamespaceConstraint<&Namespace>, + local_name: &LocalName, local_name_lower: &LocalName, ) -> bool { - let may_match_in_no_namespace = match *constraint { - NamespaceConstraint::Any => true, - NamespaceConstraint::Specific(ref ns) => ns.is_empty(), - }; - - if may_match_in_no_namespace { - if *local_name_lower == local_name!("id") { - self.map.flags.insert(InvalidationMapFlags::HAS_ID_ATTR_SELECTOR) - } else if *local_name_lower == local_name!("class") { - self.map.flags.insert(InvalidationMapFlags::HAS_CLASS_ATTR_SELECTOR) - } + if !self.add_attr_dependency(local_name.clone()) { + return false; } - if !self.compound_state.added_attr_dependency { - self.add_attr_dependency() - } else { - true + if local_name != local_name_lower && !self.add_attr_dependency(local_name_lower.clone()) { + return false; } + + true } } diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index 828bc99ed20..25536eb6bdf 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -42,7 +42,6 @@ where descendant_invalidations: &'a mut DescendantInvalidationLists<'selectors>, sibling_invalidations: &'a mut InvalidationVector<'selectors>, invalidates_self: bool, - attr_selector_flags: InvalidationMapFlags, } /// An invalidation processor for style changes due to state and attribute @@ -197,8 +196,6 @@ where return false; } - let mut attr_selector_flags = InvalidationMapFlags::empty(); - // If we the visited state changed, we force a restyle here. Matching // doesn't depend on the actual visited state at all, so we can't look // at matching results to decide what to do for this case. @@ -216,7 +213,6 @@ where let mut classes_removed = SmallVec::<[Atom; 8]>::new(); let mut classes_added = SmallVec::<[Atom; 8]>::new(); if snapshot.class_changed() { - attr_selector_flags.insert(InvalidationMapFlags::HAS_CLASS_ATTR_SELECTOR); // TODO(emilio): Do this more efficiently! snapshot.each_class(|c| { if !element.has_class(c, CaseSensitivity::CaseSensitive) { @@ -234,7 +230,6 @@ where let mut id_removed = None; let mut id_added = None; if snapshot.id_changed() { - attr_selector_flags.insert(InvalidationMapFlags::HAS_ID_ATTR_SELECTOR); let old_id = snapshot.id_attr(); let current_id = element.id(); @@ -245,10 +240,7 @@ where } if log_enabled!(::log::Level::Debug) { - debug!( - "Collecting changes for: {:?}, flags {:?}", - element, attr_selector_flags - ); + debug!("Collecting changes for: {:?}", element); if !state_changes.is_empty() { debug!(" > state: {:?}", state_changes); } @@ -261,7 +253,9 @@ where classes_added, classes_removed ); } - if snapshot.other_attr_changed() { + let mut attributes_changed = false; + snapshot.each_attr_changed(|_| { attributes_changed = true; }); + if attributes_changed { debug!( " > attributes changed, old: {}", snapshot.debug_list_attributes() @@ -296,7 +290,6 @@ where descendant_invalidations, sibling_invalidations, invalidates_self: false, - attr_selector_flags, }; let document_origins = if !matches_document_author_rules { @@ -406,12 +399,13 @@ where } } - let should_examine_attribute_selector_map = - self.snapshot.other_attr_changed() || map.flags.intersects(self.attr_selector_flags); - - if should_examine_attribute_selector_map { - self.collect_dependencies_in_map(&map.other_attribute_affecting_selectors) - } + self.snapshot.each_attr_changed(|attribute| { + if let Some(deps) = map.other_attribute_affecting_selectors.get(attribute) { + for dep in deps { + self.scan_dependency(dep); + } + } + }); let state_changes = self.state_changes; if !state_changes.is_empty() { @@ -419,19 +413,6 @@ where } } - fn collect_dependencies_in_map(&mut self, map: &'selectors SelectorMap) { - map.lookup_with_additional( - self.lookup_element, - self.matching_context.quirks_mode(), - self.removed_id, - self.classes_removed, - |dependency| { - self.scan_dependency(dependency); - true - }, - ); - } - fn collect_state_dependencies( &mut self, map: &'selectors SelectorMap, diff --git a/components/style/servo/selector_parser.rs b/components/style/servo/selector_parser.rs index 7f8ac65089d..d8b84b13280 100644 --- a/components/style/servo/selector_parser.rs +++ b/components/style/servo/selector_parser.rs @@ -391,12 +391,6 @@ impl NonTSPseudoClass { pub fn needs_cache_revalidation(&self) -> bool { self.state_flag().is_empty() } - - /// Returns true if the evaluation of the pseudo-class depends on the - /// element's attributes. - pub fn is_attr_based(&self) -> bool { - matches!(*self, NonTSPseudoClass::Lang(..)) - } } /// The abstract struct we implement the selector parser implementation on top From eff8f0fca0ce44f10a1d0ed8106add85e627252c Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 27 May 2020 21:43:00 +0000 Subject: [PATCH 37/42] style: Update aspect-ratio syntax for HTML IMG mapped ratio. Differential Revision: https://phabricator.services.mozilla.com/D76942 --- components/style/values/generics/position.rs | 45 ++++++++++++++++--- components/style/values/specified/position.rs | 41 +++++++++++++---- 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/components/style/values/generics/position.rs b/components/style/values/generics/position.rs index 5f1659d3917..2b4448f0335 100644 --- a/components/style/values/generics/position.rs +++ b/components/style/values/generics/position.rs @@ -191,7 +191,7 @@ where } } -/// A generic value for the `aspect-ratio` property. +/// Ratio or None. #[derive( Animate, Clone, @@ -208,19 +208,50 @@ where ToShmem, )] #[repr(C, u8)] -pub enum GenericAspectRatio { - /// The value. +pub enum PreferredRatio { + /// Without specified ratio + #[css(skip)] + None, + /// With specified ratio Ratio(#[css(field_bound)] Ratio), - /// The keyword `auto`. - Auto, +} + +/// A generic value for the `aspect-ratio` property, the value is `auto || `. +#[derive( + Animate, + Clone, + ComputeSquaredDistance, + Copy, + Debug, + MallocSizeOf, + PartialEq, + SpecifiedValueInfo, + ToAnimatedZero, + ToComputedValue, + ToCss, + ToResolvedValue, + ToShmem, +)] +#[repr(C)] +pub struct GenericAspectRatio { + /// Specifiy auto or not. + #[animation(constant)] + #[css(represents_keyword)] + pub auto: bool, + /// The preferred aspect-ratio value. + #[css(field_bound)] + pub ratio: PreferredRatio, } pub use self::GenericAspectRatio as AspectRatio; -impl AspectRatio { +impl AspectRatio { /// Returns `auto` #[inline] pub fn auto() -> Self { - AspectRatio::Auto + AspectRatio { + auto: true, + ratio: PreferredRatio::None, + } } } diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index 76fa23152e5..95e1e2ec712 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -882,24 +882,49 @@ pub type ZIndex = GenericZIndex; /// A specified value for the `aspect-ratio` property. pub type AspectRatio = GenericAspectRatio; -// FIXME: Add field_bound for parse custom derive, so we can drop this. impl Parse for AspectRatio { fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - if input - .try(|input| input.expect_ident_matching("auto")) - .is_ok() - { - return Ok(AspectRatio::Auto); + use crate::values::generics::position::PreferredRatio; + + let location = input.current_source_location(); + let mut auto = input.try(|i| i.expect_ident_matching("auto")); + let ratio = input.try(|i| Ratio::parse(context, i)); + if auto.is_err() { + auto = input.try(|i| i.expect_ident_matching("auto")); } - GenericRatio::parse(context, input).map(AspectRatio::Ratio) + if auto.is_err() && ratio.is_err() { + return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError)); + } + + Ok(AspectRatio { + auto: auto.is_ok(), + ratio: match ratio { + Ok(ratio) => PreferredRatio::Ratio(ratio), + Err(..) => PreferredRatio::None, + }, + }) } } -/// A specified value for the `aspect-ratio` property. +impl AspectRatio { + /// Returns Self by a valid ratio. + pub fn from_mapped_ratio(w: f32, h: f32) -> Self { + use crate::values::generics::position::PreferredRatio; + AspectRatio { + auto: true, + ratio: PreferredRatio::Ratio(GenericRatio( + NonNegativeNumber::new(w), + NonNegativeNumber::new(h), + )), + } + } +} + +/// A specified value. pub type Ratio = GenericRatio; // https://drafts.csswg.org/css-values-4/#ratios From 332aec212c4fff6bca32817749a361f35542f469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 4 Jun 2020 01:08:06 +0200 Subject: [PATCH 38/42] style: Miscellaneous servo build fixes. --- Cargo.lock | 4 +-- components/script/dom/document.rs | 13 +++++++-- .../invalidation/element/invalidation_map.rs | 3 +- components/style/matching.rs | 3 +- .../helpers/animated_properties.mako.rs | 9 ++++-- components/style/selector_map.rs | 9 ++++-- components/style/servo/selector_parser.rs | 29 ++++++++++--------- components/style/shared_lock.rs | 1 - components/style/values/animated/color.rs | 3 +- components/style/values/computed/align.rs | 2 +- components/style/values/generics/flex.rs | 2 +- components/style/values/generics/length.rs | 2 +- components/style_traits/dom.rs | 4 +-- components/url/lib.rs | 3 +- 14 files changed, 52 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 449ceba3875..a0a6ed54a51 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5329,9 +5329,9 @@ checksum = "c111b5bd5695e56cffe5129854aa230b39c93a305372fdbb2668ca2394eea9f8" [[package]] name = "smallbitvec" -version = "2.3.0" +version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1764fe2b30ee783bfe3b9b37b2649d8d590b3148bb12e0079715d4d5c673562e" +checksum = "797a4eaffb90d896f29698d45676f9f940a71936d7574996a7df54593ba209fa" [[package]] name = "smallvec" diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index b9af35b8cac..157829412ce 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -3331,7 +3331,7 @@ impl Document { pub fn element_state_will_change(&self, el: &Element) { let mut entry = self.ensure_pending_restyle(el); if entry.snapshot.is_none() { - entry.snapshot = Some(Snapshot::new(el.html_element_in_html_document())); + entry.snapshot = Some(Snapshot::new()); } let snapshot = entry.snapshot.as_mut().unwrap(); if snapshot.state.is_none() { @@ -3347,7 +3347,7 @@ impl Document { // could in theory do it in the DOM I think. let mut entry = self.ensure_pending_restyle(el); if entry.snapshot.is_none() { - entry.snapshot = Some(Snapshot::new(el.html_element_in_html_document())); + entry.snapshot = Some(Snapshot::new()); } if attr.local_name() == &local_name!("style") { entry.hint.insert(RestyleHint::RESTYLE_STYLE_ATTRIBUTE); @@ -3359,12 +3359,21 @@ impl Document { let snapshot = entry.snapshot.as_mut().unwrap(); if attr.local_name() == &local_name!("id") { + if snapshot.id_changed { + return; + } snapshot.id_changed = true; } else if attr.local_name() == &local_name!("class") { + if snapshot.class_changed { + return; + } snapshot.class_changed = true; } else { snapshot.other_attributes_changed = true; } + if !snapshot.changed_attrs.contains(attr.local_name()) { + snapshot.changed_attrs.push(attr.local_name().clone()); + } if snapshot.attrs.is_none() { let attrs = el .attrs() diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index 59143dd0025..b822e47545e 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -196,7 +196,7 @@ pub struct InvalidationMap { /// A list of document state dependencies in the rules we represent. pub document_state_selectors: Vec, /// A map of other attribute affecting selectors. - pub other_attribute_affecting_selectors: PrecomputedHashMap>, + pub other_attribute_affecting_selectors: PrecomputedHashMap>, } impl InvalidationMap { @@ -461,7 +461,6 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { } fn visit_simple_selector(&mut self, s: &Component) -> bool { - #[cfg(feature = "gecko")] use crate::selector_parser::NonTSPseudoClass; match *s { diff --git a/components/style/matching.rs b/components/style/matching.rs index 677f024acfa..e8c6e55520f 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -7,7 +7,6 @@ #![allow(unsafe_code)] #![deny(missing_docs)] -use crate::animation::AnimationState; use crate::computed_value_flags::ComputedValueFlags; use crate::context::{ElementCascadeInputs, QuirksMode, SelectorFlagsMap}; use crate::context::{SharedStyleContext, StyleContext}; @@ -438,6 +437,8 @@ trait PrivateMatchMethods: TElement { _restyle_hint: RestyleHint, _important_rules_changed: bool, ) { + use crate::animation::AnimationState; + let this_opaque = self.as_node().opaque(); let shared_context = context.shared; let mut animation_states = shared_context.animation_states.write(); diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 863da15d65c..f0c9c7cad78 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -68,8 +68,7 @@ pub type AnimationValueMap = FxHashMap; /// /// FIXME: We need to add a path for custom properties, but that's trivial after /// this (is a similar path to that of PropertyDeclaration). -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] -#[derive(Debug)] +#[derive(Debug, MallocSizeOf)] #[repr(u16)] pub enum AnimationValue { % for prop in data.longhands: @@ -420,6 +419,7 @@ impl AnimationValue { /// /// SERVO ONLY: This doesn't properly handle things like updating 'em' units /// when animated font-size. + #[cfg(feature = "servo")] pub fn set_in_style_for_servo(&self, style: &mut ComputedValues) { match self { % for prop in data.longhands: @@ -439,6 +439,11 @@ impl AnimationValue { % endfor } } + + /// As above, but a stub for Gecko. + #[cfg(feature = "gecko")] + pub fn set_in_style_for_servo(&self, _: &mut ComputedValues) { + } } fn animate_discrete(this: &T, other: &T, procedure: Procedure) -> Result { diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 3c811e2e458..2088a5baab1 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -278,9 +278,12 @@ impl SelectorMap { } impl SelectorMap { - /// Inserts into the correct hash, trying id, class, localname and - /// namespace. - pub fn insert(&mut self, entry: T, quirks_mode: QuirksMode) -> Result<(), FailedAllocationError> { + /// Inserts an entry into the correct bucket(s). + pub fn insert( + &mut self, + entry: T, + quirks_mode: QuirksMode, + ) -> Result<(), FailedAllocationError> { self.count += 1; // NOTE(emilio): It'd be nice for this to be a separate function, but diff --git a/components/style/servo/selector_parser.rs b/components/style/servo/selector_parser.rs index d8b84b13280..af4dd92fcf7 100644 --- a/components/style/servo/selector_parser.rs +++ b/components/style/servo/selector_parser.rs @@ -615,15 +615,14 @@ impl DerefMut for SnapshotMap { } /// Servo's version of an element snapshot. -#[derive(Debug)] -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] +#[derive(Debug, Default, MallocSizeOf)] pub struct ServoElementSnapshot { /// The stored state of the element. pub state: Option, /// The set of stored attributes and its values. pub attrs: Option>, - /// Whether this element is an HTML element in an HTML document. - pub is_html_element_in_html_document: bool, + /// The set of changed attributes and its values. + pub changed_attrs: Vec, /// Whether the class attribute changed or not. pub class_changed: bool, /// Whether the id attribute changed or not. @@ -634,15 +633,8 @@ pub struct ServoElementSnapshot { impl ServoElementSnapshot { /// Create an empty element snapshot. - pub fn new(is_html_element_in_html_document: bool) -> Self { - ServoElementSnapshot { - state: None, - attrs: None, - is_html_element_in_html_document: is_html_element_in_html_document, - class_changed: false, - id_changed: false, - other_attributes_changed: false, - } + pub fn new() -> Self { + Self::default() } /// Returns whether the id attribute changed or not. @@ -669,6 +661,17 @@ impl ServoElementSnapshot { .map(|&(_, ref v)| v) } + /// Executes the callback once for each attribute that changed. + #[inline] + pub fn each_attr_changed(&self, mut callback: F) + where + F: FnMut(&LocalName), + { + for name in &self.changed_attrs { + callback(name) + } + } + fn any_attr_ignore_ns(&self, name: &LocalName, mut f: F) -> bool where F: FnMut(&AttrValue) -> bool, diff --git a/components/style/shared_lock.rs b/components/style/shared_lock.rs index 107fd6d7103..d151062199f 100644 --- a/components/style/shared_lock.rs +++ b/components/style/shared_lock.rs @@ -15,7 +15,6 @@ use std::cell::UnsafeCell; use std::fmt; #[cfg(feature = "servo")] use std::mem; -use std::mem::ManuallyDrop; #[cfg(feature = "gecko")] use std::ptr; use to_shmem::{SharedMemoryBuilder, ToShmem}; diff --git a/components/style/values/animated/color.rs b/components/style/values/animated/color.rs index 14b1d0c539c..9f4fa5c52b5 100644 --- a/components/style/values/animated/color.rs +++ b/components/style/values/animated/color.rs @@ -12,8 +12,7 @@ use crate::values::generics::color::{Color as GenericColor, ComplexColorRatios}; /// /// Unlike in computed values, each component value may exceed the /// range `[0.0, 1.0]`. -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] -#[derive(Clone, Copy, Debug, PartialEq, ToAnimatedZero)] +#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToAnimatedZero)] pub struct RGBA { /// The red component. pub red: f32, diff --git a/components/style/values/computed/align.rs b/components/style/values/computed/align.rs index 893f6fcce51..5cfb53f89ee 100644 --- a/components/style/values/computed/align.rs +++ b/components/style/values/computed/align.rs @@ -35,7 +35,7 @@ pub use super::specified::{AlignSelf, JustifySelf}; /// sucks :(. /// /// See the discussion in https://bugzil.la/1384542. -#[derive(Clone, Copy, Debug, Eq, PartialEq, ToCss, ToResolvedValue)] +#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToCss, ToResolvedValue)] #[repr(C)] pub struct ComputedJustifyItems { /// The specified value for the property. Can contain the bare `legacy` diff --git a/components/style/values/generics/flex.rs b/components/style/values/generics/flex.rs index adff13b5b42..85b64000f2b 100644 --- a/components/style/values/generics/flex.rs +++ b/components/style/values/generics/flex.rs @@ -5,13 +5,13 @@ //! Generic types for CSS values related to flexbox. /// A generic value for the `flex-basis` property. -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] #[derive( Animate, Clone, ComputeSquaredDistance, Copy, Debug, + MallocSizeOf, Parse, PartialEq, SpecifiedValueInfo, diff --git a/components/style/values/generics/length.rs b/components/style/values/generics/length.rs index 4183f40a942..bed574861cb 100644 --- a/components/style/values/generics/length.rs +++ b/components/style/values/generics/length.rs @@ -174,13 +174,13 @@ impl Size { /// A generic value for the `max-width` or `max-height` property. #[allow(missing_docs)] -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] #[derive( Animate, Clone, ComputeSquaredDistance, Copy, Debug, + MallocSizeOf, PartialEq, SpecifiedValueInfo, ToAnimatedValue, diff --git a/components/style_traits/dom.rs b/components/style_traits/dom.rs index f9844519279..03d5264abf5 100644 --- a/components/style_traits/dom.rs +++ b/components/style_traits/dom.rs @@ -13,8 +13,8 @@ /// Because the script task's GC does not trace layout, node data cannot be safely stored in layout /// data structures. Also, layout code tends to be faster when the DOM is not being accessed, for /// locality reasons. Using `OpaqueNode` enforces this invariant. -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] -#[cfg_attr(feature = "servo", derive(MallocSizeOf, Deserialize, Serialize))] +#[derive(Clone, Copy, Debug, Eq, Hash, MallocSizeOf, PartialEq)] +#[cfg_attr(feature = "servo", derive(Deserialize, Serialize))] pub struct OpaqueNode(pub usize); impl OpaqueNode { diff --git a/components/url/lib.rs b/components/url/lib.rs index 8a05837a461..f58bbbad3ff 100644 --- a/components/url/lib.rs +++ b/components/url/lib.rs @@ -20,7 +20,6 @@ pub use crate::origin::{ImmutableOrigin, MutableOrigin, OpaqueOrigin}; use std::collections::hash_map::DefaultHasher; use std::fmt; use std::hash::Hasher; -use std::mem::ManuallyDrop; use std::net::IpAddr; use std::ops::{Index, Range, RangeFrom, RangeFull, RangeTo}; use std::path::Path; @@ -34,7 +33,7 @@ pub use url::Host; pub struct ServoUrl(#[ignore_malloc_size_of = "Arc"] Arc); impl ToShmem for ServoUrl { - fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> ManuallyDrop { + fn to_shmem(&self, _builder: &mut SharedMemoryBuilder) -> to_shmem::Result { unimplemented!("If servo wants to share stylesheets across processes, ToShmem for Url must be implemented") } } From 69c7077b3dcd2574002ba656fed0cc25a745a1ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 3 Jun 2020 19:28:16 +0200 Subject: [PATCH 39/42] style: Don't use lazy_static for media feature parsing. This used to be needed when destructors in statics were not allowed, but we can get rid of it nowadays. Differential Revision: https://phabricator.services.mozilla.com/D78109 --- components/style/gecko/media_features.rs | 440 +++++++++++------------ 1 file changed, 219 insertions(+), 221 deletions(-) diff --git a/components/style/gecko/media_features.rs b/components/style/gecko/media_features.rs index 63d5ce10772..d11aa0d6893 100644 --- a/components/style/gecko/media_features.rs +++ b/components/style/gecko/media_features.rs @@ -538,225 +538,223 @@ macro_rules! system_metric_feature { }}; } -lazy_static! { - /// Adding new media features requires (1) adding the new feature to this - /// array, with appropriate entries (and potentially any new code needed - /// to support new types in these entries and (2) ensuring that either - /// nsPresContext::MediaFeatureValuesChanged is called when the value that - /// would be returned by the evaluator function could change. - pub static ref MEDIA_FEATURES: [MediaFeatureDescription; 53] = [ - feature!( - atom!("width"), - AllowsRanges::Yes, - Evaluator::Length(eval_width), - ParsingRequirements::empty(), - ), - feature!( - atom!("height"), - AllowsRanges::Yes, - Evaluator::Length(eval_height), - ParsingRequirements::empty(), - ), - feature!( - atom!("aspect-ratio"), - AllowsRanges::Yes, - Evaluator::NumberRatio(eval_aspect_ratio), - ParsingRequirements::empty(), - ), - feature!( - atom!("orientation"), - AllowsRanges::No, - keyword_evaluator!(eval_orientation, Orientation), - ParsingRequirements::empty(), - ), - feature!( - atom!("device-width"), - AllowsRanges::Yes, - Evaluator::Length(eval_device_width), - ParsingRequirements::empty(), - ), - feature!( - atom!("device-height"), - AllowsRanges::Yes, - Evaluator::Length(eval_device_height), - ParsingRequirements::empty(), - ), - feature!( - atom!("device-aspect-ratio"), - AllowsRanges::Yes, - Evaluator::NumberRatio(eval_device_aspect_ratio), - ParsingRequirements::empty(), - ), - feature!( - atom!("-moz-device-orientation"), - AllowsRanges::No, - keyword_evaluator!(eval_device_orientation, Orientation), - ParsingRequirements::empty(), - ), - // Webkit extensions that we support for de-facto web compatibility. - // -webkit-{min|max}-device-pixel-ratio (controlled with its own pref): - feature!( - atom!("device-pixel-ratio"), - AllowsRanges::Yes, - Evaluator::Float(eval_device_pixel_ratio), - ParsingRequirements::WEBKIT_PREFIX, - ), - // -webkit-transform-3d. - feature!( - atom!("transform-3d"), - AllowsRanges::No, - Evaluator::BoolInteger(eval_transform_3d), - ParsingRequirements::WEBKIT_PREFIX, - ), - feature!( - atom!("-moz-device-pixel-ratio"), - AllowsRanges::Yes, - Evaluator::Float(eval_device_pixel_ratio), - ParsingRequirements::empty(), - ), - feature!( - atom!("resolution"), - AllowsRanges::Yes, - Evaluator::Resolution(eval_resolution), - ParsingRequirements::empty(), - ), - feature!( - atom!("display-mode"), - AllowsRanges::No, - keyword_evaluator!(eval_display_mode, DisplayMode), - ParsingRequirements::empty(), - ), - feature!( - atom!("grid"), - AllowsRanges::No, - Evaluator::BoolInteger(eval_grid), - ParsingRequirements::empty(), - ), - feature!( - atom!("scan"), - AllowsRanges::No, - keyword_evaluator!(eval_scan, Scan), - ParsingRequirements::empty(), - ), - feature!( - atom!("color"), - AllowsRanges::Yes, - Evaluator::Integer(eval_color), - ParsingRequirements::empty(), - ), - feature!( - atom!("color-index"), - AllowsRanges::Yes, - Evaluator::Integer(eval_color_index), - ParsingRequirements::empty(), - ), - feature!( - atom!("monochrome"), - AllowsRanges::Yes, - Evaluator::Integer(eval_monochrome), - ParsingRequirements::empty(), - ), - feature!( - atom!("prefers-reduced-motion"), - AllowsRanges::No, - keyword_evaluator!(eval_prefers_reduced_motion, PrefersReducedMotion), - ParsingRequirements::empty(), - ), - feature!( - atom!("overflow-block"), - AllowsRanges::No, - keyword_evaluator!(eval_overflow_block, OverflowBlock), - ParsingRequirements::empty(), - ), - feature!( - atom!("overflow-inline"), - AllowsRanges::No, - keyword_evaluator!(eval_overflow_inline, OverflowInline), - ParsingRequirements::empty(), - ), - feature!( - atom!("prefers-color-scheme"), - AllowsRanges::No, - keyword_evaluator!(eval_prefers_color_scheme, PrefersColorScheme), - ParsingRequirements::empty(), - ), - feature!( - atom!("pointer"), - AllowsRanges::No, - keyword_evaluator!(eval_pointer, Pointer), - ParsingRequirements::empty(), - ), - feature!( - atom!("any-pointer"), - AllowsRanges::No, - keyword_evaluator!(eval_any_pointer, Pointer), - ParsingRequirements::empty(), - ), - feature!( - atom!("hover"), - AllowsRanges::No, - keyword_evaluator!(eval_hover, Hover), - ParsingRequirements::empty(), - ), - feature!( - atom!("any-hover"), - AllowsRanges::No, - keyword_evaluator!(eval_any_hover, Hover), - ParsingRequirements::empty(), - ), +/// Adding new media features requires (1) adding the new feature to this +/// array, with appropriate entries (and potentially any new code needed +/// to support new types in these entries and (2) ensuring that either +/// nsPresContext::MediaFeatureValuesChanged is called when the value that +/// would be returned by the evaluator function could change. +pub static MEDIA_FEATURES: [MediaFeatureDescription; 53] = [ + feature!( + atom!("width"), + AllowsRanges::Yes, + Evaluator::Length(eval_width), + ParsingRequirements::empty(), + ), + feature!( + atom!("height"), + AllowsRanges::Yes, + Evaluator::Length(eval_height), + ParsingRequirements::empty(), + ), + feature!( + atom!("aspect-ratio"), + AllowsRanges::Yes, + Evaluator::NumberRatio(eval_aspect_ratio), + ParsingRequirements::empty(), + ), + feature!( + atom!("orientation"), + AllowsRanges::No, + keyword_evaluator!(eval_orientation, Orientation), + ParsingRequirements::empty(), + ), + feature!( + atom!("device-width"), + AllowsRanges::Yes, + Evaluator::Length(eval_device_width), + ParsingRequirements::empty(), + ), + feature!( + atom!("device-height"), + AllowsRanges::Yes, + Evaluator::Length(eval_device_height), + ParsingRequirements::empty(), + ), + feature!( + atom!("device-aspect-ratio"), + AllowsRanges::Yes, + Evaluator::NumberRatio(eval_device_aspect_ratio), + ParsingRequirements::empty(), + ), + feature!( + atom!("-moz-device-orientation"), + AllowsRanges::No, + keyword_evaluator!(eval_device_orientation, Orientation), + ParsingRequirements::empty(), + ), + // Webkit extensions that we support for de-facto web compatibility. + // -webkit-{min|max}-device-pixel-ratio (controlled with its own pref): + feature!( + atom!("device-pixel-ratio"), + AllowsRanges::Yes, + Evaluator::Float(eval_device_pixel_ratio), + ParsingRequirements::WEBKIT_PREFIX, + ), + // -webkit-transform-3d. + feature!( + atom!("transform-3d"), + AllowsRanges::No, + Evaluator::BoolInteger(eval_transform_3d), + ParsingRequirements::WEBKIT_PREFIX, + ), + feature!( + atom!("-moz-device-pixel-ratio"), + AllowsRanges::Yes, + Evaluator::Float(eval_device_pixel_ratio), + ParsingRequirements::empty(), + ), + feature!( + atom!("resolution"), + AllowsRanges::Yes, + Evaluator::Resolution(eval_resolution), + ParsingRequirements::empty(), + ), + feature!( + atom!("display-mode"), + AllowsRanges::No, + keyword_evaluator!(eval_display_mode, DisplayMode), + ParsingRequirements::empty(), + ), + feature!( + atom!("grid"), + AllowsRanges::No, + Evaluator::BoolInteger(eval_grid), + ParsingRequirements::empty(), + ), + feature!( + atom!("scan"), + AllowsRanges::No, + keyword_evaluator!(eval_scan, Scan), + ParsingRequirements::empty(), + ), + feature!( + atom!("color"), + AllowsRanges::Yes, + Evaluator::Integer(eval_color), + ParsingRequirements::empty(), + ), + feature!( + atom!("color-index"), + AllowsRanges::Yes, + Evaluator::Integer(eval_color_index), + ParsingRequirements::empty(), + ), + feature!( + atom!("monochrome"), + AllowsRanges::Yes, + Evaluator::Integer(eval_monochrome), + ParsingRequirements::empty(), + ), + feature!( + atom!("prefers-reduced-motion"), + AllowsRanges::No, + keyword_evaluator!(eval_prefers_reduced_motion, PrefersReducedMotion), + ParsingRequirements::empty(), + ), + feature!( + atom!("overflow-block"), + AllowsRanges::No, + keyword_evaluator!(eval_overflow_block, OverflowBlock), + ParsingRequirements::empty(), + ), + feature!( + atom!("overflow-inline"), + AllowsRanges::No, + keyword_evaluator!(eval_overflow_inline, OverflowInline), + ParsingRequirements::empty(), + ), + feature!( + atom!("prefers-color-scheme"), + AllowsRanges::No, + keyword_evaluator!(eval_prefers_color_scheme, PrefersColorScheme), + ParsingRequirements::empty(), + ), + feature!( + atom!("pointer"), + AllowsRanges::No, + keyword_evaluator!(eval_pointer, Pointer), + ParsingRequirements::empty(), + ), + feature!( + atom!("any-pointer"), + AllowsRanges::No, + keyword_evaluator!(eval_any_pointer, Pointer), + ParsingRequirements::empty(), + ), + feature!( + atom!("hover"), + AllowsRanges::No, + keyword_evaluator!(eval_hover, Hover), + ParsingRequirements::empty(), + ), + feature!( + atom!("any-hover"), + AllowsRanges::No, + keyword_evaluator!(eval_any_hover, Hover), + ParsingRequirements::empty(), + ), - // Internal -moz-is-glyph media feature: applies only inside SVG glyphs. - // Internal because it is really only useful in the user agent anyway - // and therefore not worth standardizing. - feature!( - atom!("-moz-is-glyph"), - AllowsRanges::No, - Evaluator::BoolInteger(eval_moz_is_glyph), - ParsingRequirements::CHROME_AND_UA_ONLY, - ), - feature!( - atom!("-moz-is-resource-document"), - AllowsRanges::No, - Evaluator::BoolInteger(eval_moz_is_resource_document), - ParsingRequirements::CHROME_AND_UA_ONLY, - ), - feature!( - atom!("-moz-os-version"), - AllowsRanges::No, - Evaluator::Ident(eval_moz_os_version), - ParsingRequirements::CHROME_AND_UA_ONLY, - ), - system_metric_feature!(atom!("-moz-scrollbar-start-backward")), - system_metric_feature!(atom!("-moz-scrollbar-start-forward")), - system_metric_feature!(atom!("-moz-scrollbar-end-backward")), - system_metric_feature!(atom!("-moz-scrollbar-end-forward")), - system_metric_feature!(atom!("-moz-scrollbar-thumb-proportional")), - system_metric_feature!(atom!("-moz-overlay-scrollbars")), - system_metric_feature!(atom!("-moz-windows-default-theme")), - system_metric_feature!(atom!("-moz-mac-graphite-theme")), - system_metric_feature!(atom!("-moz-mac-yosemite-theme")), - system_metric_feature!(atom!("-moz-windows-accent-color-in-titlebar")), - system_metric_feature!(atom!("-moz-windows-compositor")), - system_metric_feature!(atom!("-moz-windows-classic")), - system_metric_feature!(atom!("-moz-windows-glass")), - system_metric_feature!(atom!("-moz-menubar-drag")), - system_metric_feature!(atom!("-moz-swipe-animation-enabled")), - system_metric_feature!(atom!("-moz-gtk-csd-available")), - system_metric_feature!(atom!("-moz-gtk-csd-hide-titlebar-by-default")), - system_metric_feature!(atom!("-moz-gtk-csd-transparent-background")), - system_metric_feature!(atom!("-moz-gtk-csd-minimize-button")), - system_metric_feature!(atom!("-moz-gtk-csd-maximize-button")), - system_metric_feature!(atom!("-moz-gtk-csd-close-button")), - system_metric_feature!(atom!("-moz-gtk-csd-reversed-placement")), - system_metric_feature!(atom!("-moz-system-dark-theme")), - // This is the only system-metric media feature that's accessible to - // content as of today. - // FIXME(emilio): Restrict (or remove?) when bug 1035774 lands. - feature!( - atom!("-moz-touch-enabled"), - AllowsRanges::No, - Evaluator::BoolInteger(eval_moz_touch_enabled), - ParsingRequirements::empty(), - ), - ]; -} + // Internal -moz-is-glyph media feature: applies only inside SVG glyphs. + // Internal because it is really only useful in the user agent anyway + // and therefore not worth standardizing. + feature!( + atom!("-moz-is-glyph"), + AllowsRanges::No, + Evaluator::BoolInteger(eval_moz_is_glyph), + ParsingRequirements::CHROME_AND_UA_ONLY, + ), + feature!( + atom!("-moz-is-resource-document"), + AllowsRanges::No, + Evaluator::BoolInteger(eval_moz_is_resource_document), + ParsingRequirements::CHROME_AND_UA_ONLY, + ), + feature!( + atom!("-moz-os-version"), + AllowsRanges::No, + Evaluator::Ident(eval_moz_os_version), + ParsingRequirements::CHROME_AND_UA_ONLY, + ), + system_metric_feature!(atom!("-moz-scrollbar-start-backward")), + system_metric_feature!(atom!("-moz-scrollbar-start-forward")), + system_metric_feature!(atom!("-moz-scrollbar-end-backward")), + system_metric_feature!(atom!("-moz-scrollbar-end-forward")), + system_metric_feature!(atom!("-moz-scrollbar-thumb-proportional")), + system_metric_feature!(atom!("-moz-overlay-scrollbars")), + system_metric_feature!(atom!("-moz-windows-default-theme")), + system_metric_feature!(atom!("-moz-mac-graphite-theme")), + system_metric_feature!(atom!("-moz-mac-yosemite-theme")), + system_metric_feature!(atom!("-moz-windows-accent-color-in-titlebar")), + system_metric_feature!(atom!("-moz-windows-compositor")), + system_metric_feature!(atom!("-moz-windows-classic")), + system_metric_feature!(atom!("-moz-windows-glass")), + system_metric_feature!(atom!("-moz-menubar-drag")), + system_metric_feature!(atom!("-moz-swipe-animation-enabled")), + system_metric_feature!(atom!("-moz-gtk-csd-available")), + system_metric_feature!(atom!("-moz-gtk-csd-hide-titlebar-by-default")), + system_metric_feature!(atom!("-moz-gtk-csd-transparent-background")), + system_metric_feature!(atom!("-moz-gtk-csd-minimize-button")), + system_metric_feature!(atom!("-moz-gtk-csd-maximize-button")), + system_metric_feature!(atom!("-moz-gtk-csd-close-button")), + system_metric_feature!(atom!("-moz-gtk-csd-reversed-placement")), + system_metric_feature!(atom!("-moz-system-dark-theme")), + // This is the only system-metric media feature that's accessible to + // content as of today. + // FIXME(emilio): Restrict (or remove?) when bug 1035774 lands. + feature!( + atom!("-moz-touch-enabled"), + AllowsRanges::No, + Evaluator::BoolInteger(eval_moz_touch_enabled), + ParsingRequirements::empty(), + ), +]; From 762abbaf9f886fc4463124d88f54fafab0ca1a6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 4 Jun 2020 02:02:50 +0200 Subject: [PATCH 40/42] style: Rustfmt recent changes. --- components/selectors/parser.rs | 54 +++++++++------- components/style/build_gecko.rs | 1 + components/style/dom_apis.rs | 15 +++-- components/style/gecko/media_features.rs | 1 - components/style/gecko/pseudo_element.rs | 12 ++-- components/style/gecko/selector_parser.rs | 3 +- .../style/gecko_bindings/sugar/ownership.rs | 4 +- .../invalidation/element/document_state.rs | 5 +- .../invalidation/element/invalidation_map.rs | 23 ++++--- .../style/invalidation/element/invalidator.rs | 61 +++++++++++-------- .../element/state_and_attributes.rs | 17 ++++-- components/style/parser.rs | 10 ++- components/style/selector_map.rs | 11 +++- components/style/values/computed/align.rs | 3 +- components/style/values/computed/mod.rs | 9 ++- components/style/values/specified/align.rs | 16 ++--- components/style/values/specified/mod.rs | 4 +- components/style/values/specified/position.rs | 5 +- components/style_derive/parse.rs | 10 ++- 19 files changed, 163 insertions(+), 101 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index e66fa75c587..9980bdeeb60 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -144,9 +144,7 @@ impl SelectorParsingState { #[inline] fn allows_non_functional_pseudo_classes(self) -> bool { - !self.intersects( - Self::AFTER_SLOTTED | Self::AFTER_NON_STATEFUL_PSEUDO_ELEMENT, - ) + !self.intersects(Self::AFTER_SLOTTED | Self::AFTER_NON_STATEFUL_PSEUDO_ELEMENT) } #[inline] @@ -354,10 +352,9 @@ impl SelectorList { { let mut values = SmallVec::new(); loop { - values.push( - input - .parse_until_before(Delimiter::Comma, |input| parse_selector(parser, input, state))?, - ); + values.push(input.parse_until_before(Delimiter::Comma, |input| { + parse_selector(parser, input, state) + })?); match input.next() { Err(_) => return Ok(SelectorList(values)), Ok(&Token::Comma) => continue, @@ -382,7 +379,11 @@ where P: Parser<'i, Impl = Impl>, Impl: SelectorImpl, { - parse_selector(parser, input, state | SelectorParsingState::DISALLOW_PSEUDOS | SelectorParsingState::DISALLOW_COMBINATORS) + parse_selector( + parser, + input, + state | SelectorParsingState::DISALLOW_PSEUDOS | SelectorParsingState::DISALLOW_COMBINATORS, + ) } /// Parse a comma separated list of compound selectors. @@ -395,7 +396,9 @@ where Impl: SelectorImpl, { input - .parse_comma_separated(|input| parse_inner_compound_selector(parser, input, SelectorParsingState::empty())) + .parse_comma_separated(|input| { + parse_inner_compound_selector(parser, input, SelectorParsingState::empty()) + }) .map(|selectors| selectors.into_boxed_slice()) } @@ -450,9 +453,7 @@ where }, // In quirks mode, class and id selectors should match // case-insensitively, so just avoid inserting them into the filter. - Component::ID(ref id) if quirks_mode != QuirksMode::Quirks => { - id.precomputed_hash() - }, + Component::ID(ref id) if quirks_mode != QuirksMode::Quirks => id.precomputed_hash(), Component::Class(ref class) if quirks_mode != QuirksMode::Quirks => { class.precomputed_hash() }, @@ -466,7 +467,7 @@ where } } continue; - } + }, _ => continue, }; @@ -1086,11 +1087,14 @@ impl Component { pub fn maybe_allowed_after_pseudo_element(&self) -> bool { match *self { Component::NonTSPseudoClass(..) => true, - Component::Negation(ref components) => components.iter().all(|c| c.maybe_allowed_after_pseudo_element()), - Component::Is(ref selectors) | - Component::Where(ref selectors) => { + Component::Negation(ref components) => components + .iter() + .all(|c| c.maybe_allowed_after_pseudo_element()), + Component::Is(ref selectors) | Component::Where(ref selectors) => { selectors.iter().all(|selector| { - selector.iter_raw_match_order().all(|c| c.maybe_allowed_after_pseudo_element()) + selector + .iter_raw_match_order() + .all(|c| c.maybe_allowed_after_pseudo_element()) }) }, _ => false, @@ -1110,12 +1114,14 @@ impl Component { *self ); match *self { - Component::Negation(ref components) => { - !components.iter().all(|c| c.matches_for_stateless_pseudo_element()) - }, + Component::Negation(ref components) => !components + .iter() + .all(|c| c.matches_for_stateless_pseudo_element()), Component::Is(ref selectors) | Component::Where(ref selectors) => { selectors.iter().any(|selector| { - selector.iter_raw_match_order().all(|c| c.matches_for_stateless_pseudo_element()) + selector + .iter_raw_match_order() + .all(|c| c.matches_for_stateless_pseudo_element()) }) }, _ => false, @@ -2254,7 +2260,11 @@ where // Pseudo-elements cannot be represented by the matches-any // pseudo-class; they are not valid within :is(). // - let inner = SelectorList::parse_with_state(parser, input, state | SelectorParsingState::DISALLOW_PSEUDOS)?; + let inner = SelectorList::parse_with_state( + parser, + input, + state | SelectorParsingState::DISALLOW_PSEUDOS, + )?; Ok(component(inner.0.into_vec().into_boxed_slice())) } diff --git a/components/style/build_gecko.rs b/components/style/build_gecko.rs index a502b7d45f9..dfd2a213d99 100644 --- a/components/style/build_gecko.rs +++ b/components/style/build_gecko.rs @@ -131,6 +131,7 @@ impl BuilderExt for Builder { // them. let mut builder = Builder::default() .rust_target(RustTarget::Stable_1_25) + .size_t_is_usize(true) .disable_untagged_union(); let rustfmt_path = env::var_os("RUSTFMT") diff --git a/components/style/dom_apis.rs b/components/style/dom_apis.rs index 685b5b4e8a0..f4e814acd11 100644 --- a/components/style/dom_apis.rs +++ b/components/style/dom_apis.rs @@ -7,9 +7,9 @@ use crate::context::QuirksMode; use crate::dom::{TDocument, TElement, TNode, TShadowRoot}; +use crate::invalidation::element::invalidation_map::Dependency; use crate::invalidation::element::invalidator::{DescendantInvalidationLists, Invalidation}; use crate::invalidation::element::invalidator::{InvalidationProcessor, InvalidationVector}; -use crate::invalidation::element::invalidation_map::Dependency; use crate::Atom; use selectors::attr::CaseSensitivity; use selectors::matching::{self, MatchingContext, MatchingMode}; @@ -145,7 +145,10 @@ where } fn check_outer_dependency(&mut self, _: &Dependency, _: E) -> bool { - debug_assert!(false, "How? We should only have parent-less dependencies here!"); + debug_assert!( + false, + "How? We should only have parent-less dependencies here!" + ); true } @@ -647,9 +650,11 @@ pub fn query_selector( if root_element.is_some() || !invalidation_may_be_useful { query_selector_slow::(root, selector_list, results, &mut matching_context); } else { - let dependencies = selector_list.0.iter().map(|selector| { - Dependency::for_full_selector_invalidation(selector.clone()) - }).collect::>(); + let dependencies = selector_list + .0 + .iter() + .map(|selector| Dependency::for_full_selector_invalidation(selector.clone())) + .collect::>(); let mut processor = QuerySelectorProcessor:: { results, matching_context, diff --git a/components/style/gecko/media_features.rs b/components/style/gecko/media_features.rs index d11aa0d6893..4d60dc2ff26 100644 --- a/components/style/gecko/media_features.rs +++ b/components/style/gecko/media_features.rs @@ -703,7 +703,6 @@ pub static MEDIA_FEATURES: [MediaFeatureDescription; 53] = [ keyword_evaluator!(eval_any_hover, Hover), ParsingRequirements::empty(), ), - // Internal -moz-is-glyph media feature: applies only inside SVG glyphs. // Internal because it is really only useful in the user agent anyway // and therefore not worth standardizing. diff --git a/components/style/gecko/pseudo_element.rs b/components/style/gecko/pseudo_element.rs index 631172d9511..16e3ab312f4 100644 --- a/components/style/gecko/pseudo_element.rs +++ b/components/style/gecko/pseudo_element.rs @@ -161,11 +161,13 @@ impl PseudoElement { /// Whether this pseudo-element is enabled for all content. pub fn enabled_in_content(&self) -> bool { match *self { - PseudoElement::MozFocusOuter => static_prefs::pref!("layout.css.moz-focus-outer.enabled"), - PseudoElement::FileChooserButton => static_prefs::pref!("layout.css.file-chooser-button.enabled"), - _ => { - (self.flags() & structs::CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS_AND_CHROME) == 0 - } + PseudoElement::MozFocusOuter => { + static_prefs::pref!("layout.css.moz-focus-outer.enabled") + }, + PseudoElement::FileChooserButton => { + static_prefs::pref!("layout.css.file-chooser-button.enabled") + }, + _ => (self.flags() & structs::CSS_PSEUDO_ELEMENT_ENABLED_IN_UA_SHEETS_AND_CHROME) == 0, } } diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index e5d332df689..fd0494fa4b4 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -343,7 +343,8 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { #[inline] fn parse_is_and_where(&self) -> bool { - self.in_user_agent_stylesheet() || static_prefs::pref!("layout.css.is-where-selectors.enabled") + self.in_user_agent_stylesheet() || + static_prefs::pref!("layout.css.is-where-selectors.enabled") } #[inline] diff --git a/components/style/gecko_bindings/sugar/ownership.rs b/components/style/gecko_bindings/sugar/ownership.rs index 0ece222a1c4..366e4619c68 100644 --- a/components/style/gecko_bindings/sugar/ownership.rs +++ b/components/style/gecko_bindings/sugar/ownership.rs @@ -4,12 +4,12 @@ //! Helpers for different FFI pointer kinds that Gecko's FFI layer uses. +use gecko_bindings::structs::root::mozilla::detail::CopyablePtr; use servo_arc::{Arc, RawOffsetArc}; use std::marker::PhantomData; use std::mem::{forget, transmute}; use std::ops::{Deref, DerefMut}; use std::ptr; -use gecko_bindings::structs::root::mozilla::detail::CopyablePtr; /// Indicates that a given Servo type has a corresponding Gecko FFI type. pub unsafe trait HasFFI: Sized + 'static { @@ -345,4 +345,4 @@ impl DerefMut for CopyablePtr { fn deref_mut<'a>(&'a mut self) -> &'a mut T { &mut self.mPtr } -} \ No newline at end of file +} diff --git a/components/style/invalidation/element/document_state.rs b/components/style/invalidation/element/document_state.rs index 4659eb42f5e..9ee97344a4c 100644 --- a/components/style/invalidation/element/document_state.rs +++ b/components/style/invalidation/element/document_state.rs @@ -67,7 +67,10 @@ where I: Iterator, { fn check_outer_dependency(&mut self, _: &Dependency, _: E) -> bool { - debug_assert!(false, "how, we should only have parent-less dependencies here!"); + debug_assert!( + false, + "how, we should only have parent-less dependencies here!" + ); true } diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index b822e47545e..6d9b6c1e85d 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -6,7 +6,9 @@ use crate::context::QuirksMode; use crate::element_state::{DocumentState, ElementState}; -use crate::selector_map::{MaybeCaseInsensitiveHashMap, PrecomputedHashMap, SelectorMap, SelectorMapEntry}; +use crate::selector_map::{ + MaybeCaseInsensitiveHashMap, PrecomputedHashMap, SelectorMap, SelectorMapEntry, +}; use crate::selector_parser::SelectorImpl; use crate::{Atom, LocalName, Namespace}; use fallible::FallibleVec; @@ -196,7 +198,8 @@ pub struct InvalidationMap { /// A list of document state dependencies in the rules we represent. pub document_state_selectors: Vec, /// A map of other attribute affecting selectors. - pub other_attribute_affecting_selectors: PrecomputedHashMap>, + pub other_attribute_affecting_selectors: + PrecomputedHashMap>, } impl InvalidationMap { @@ -331,7 +334,11 @@ impl<'a> SelectorDependencyCollector<'a> { self.visit_whole_selector_from(iter, 0) } - fn visit_whole_selector_from(&mut self, mut iter: SelectorIter, mut index: usize) -> bool { + fn visit_whole_selector_from( + &mut self, + mut iter: SelectorIter, + mut index: usize, + ) -> bool { loop { // Reset the compound state. self.compound_state = PerCompoundState::new(index); @@ -384,7 +391,7 @@ impl<'a> SelectorDependencyCollector<'a> { Err(err) => { *self.alloc_error = Some(err); return false; - } + }, } } @@ -395,8 +402,7 @@ impl<'a> SelectorDependencyCollector<'a> { // cache them or something. for &(ref selector, ref selector_offset) in self.parent_selectors.iter() { debug_assert_ne!( - self.compound_state.offset, - 0, + self.compound_state.offset, 0, "Shouldn't bother creating nested dependencies for the rightmost compound", ); let new_parent = Dependency { @@ -442,7 +448,8 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { index += 1; // account for the combinator. - self.parent_selectors.push((self.selector.clone(), self.compound_state.offset)); + self.parent_selectors + .push((self.selector.clone(), self.compound_state.offset)); let mut nested = SelectorDependencyCollector { map: &mut *self.map, document_state: &mut *self.document_state, @@ -483,7 +490,7 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> { Err(err) => { *self.alloc_error = Some(err); return false; - } + }, } }, Component::NonTSPseudoClass(ref pc) => { diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 8b387bc0c04..c148b63e3e3 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -177,13 +177,10 @@ pub struct Invalidation<'a> { impl<'a> Invalidation<'a> { /// Create a new invalidation for matching a dependency. - pub fn new( - dependency: &'a Dependency, - scope: Option, - ) -> Self { + pub fn new(dependency: &'a Dependency, scope: Option) -> Self { debug_assert!( dependency.selector_offset == dependency.selector.len() + 1 || - dependency.invalidation_kind() != DependencyInvalidationKind::Element, + dependency.invalidation_kind() != DependencyInvalidationKind::Element, "No point to this, if the dependency matched the element we should just invalidate it" ); Self { @@ -206,7 +203,11 @@ impl<'a> Invalidation<'a> { // for the weird pseudos in . // // We should be able to do better here! - match self.dependency.selector.combinator_at_parse_order(self.offset - 1) { + match self + .dependency + .selector + .combinator_at_parse_order(self.offset - 1) + { Combinator::Descendant | Combinator::LaterSibling | Combinator::PseudoElement => true, Combinator::Part | Combinator::SlotAssignment | @@ -220,7 +221,11 @@ impl<'a> Invalidation<'a> { return InvalidationKind::Descendant(DescendantInvalidationKind::Dom); } - match self.dependency.selector.combinator_at_parse_order(self.offset - 1) { + match self + .dependency + .selector + .combinator_at_parse_order(self.offset - 1) + { Combinator::Child | Combinator::Descendant | Combinator::PseudoElement => { InvalidationKind::Descendant(DescendantInvalidationKind::Dom) }, @@ -238,7 +243,11 @@ impl<'a> fmt::Debug for Invalidation<'a> { use cssparser::ToCss; f.write_str("Invalidation(")?; - for component in self.dependency.selector.iter_raw_parse_order_from(self.offset) { + for component in self + .dependency + .selector + .iter_raw_parse_order_from(self.offset) + { if matches!(*component, Component::Combinator(..)) { break; } @@ -816,9 +825,11 @@ where let mut cur_dependency = invalidation.dependency; loop { cur_dependency = match cur_dependency.parent { - None => return SingleInvalidationResult { - invalidated_self: true, - matched: true, + None => { + return SingleInvalidationResult { + invalidated_self: true, + matched: true, + } }, Some(ref p) => &**p, }; @@ -828,11 +839,14 @@ where // The inner selector changed, now check if the full // previous part of the selector did, before keeping // checking for descendants. - if !self.processor.check_outer_dependency(cur_dependency, self.element) { + if !self + .processor + .check_outer_dependency(cur_dependency, self.element) + { return SingleInvalidationResult { invalidated_self: false, matched: false, - } + }; } if cur_dependency.invalidation_kind() == DependencyInvalidationKind::Element { @@ -840,24 +854,21 @@ where } debug!(" > Generating invalidation"); - break Invalidation::new(cur_dependency, invalidation.scope) + break Invalidation::new(cur_dependency, invalidation.scope); } }, CompoundSelectorMatchingResult::Matched { next_combinator_offset, - } => { - Invalidation { - dependency: invalidation.dependency, - scope: invalidation.scope, - offset: next_combinator_offset + 1, - matched_by_any_previous: false, - } + } => Invalidation { + dependency: invalidation.dependency, + scope: invalidation.scope, + offset: next_combinator_offset + 1, + matched_by_any_previous: false, }, }; debug_assert_ne!( - next_invalidation.offset, - 0, + next_invalidation.offset, 0, "Rightmost selectors shouldn't generate more invalidations", ); @@ -886,7 +897,9 @@ where c.maybe_allowed_after_pseudo_element(), "Someone seriously messed up selector parsing: \ {:?} at offset {:?}: {:?}", - next_invalidation.dependency, next_invalidation.offset, c, + next_invalidation.dependency, + next_invalidation.offset, + c, ); None diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index 25536eb6bdf..79f1b53ada7 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -254,7 +254,9 @@ where ); } let mut attributes_changed = false; - snapshot.each_attr_changed(|_| { attributes_changed = true; }); + snapshot.each_attr_changed(|_| { + attributes_changed = true; + }); if attributes_changed { debug!( " > attributes changed, old: {}", @@ -436,7 +438,12 @@ where /// Check whether a dependency should be taken into account. #[inline] fn check_dependency(&mut self, dependency: &Dependency) -> bool { - check_dependency(dependency, &self.element, &self.wrapper, &mut self.matching_context) + check_dependency( + dependency, + &self.element, + &self.wrapper, + &mut self.matching_context, + ) } fn scan_dependency(&mut self, dependency: &'selectors Dependency) { @@ -472,10 +479,8 @@ where debug_assert_ne!(dependency.selector_offset, 0); debug_assert_ne!(dependency.selector_offset, dependency.selector.len()); - let invalidation = Invalidation::new( - &dependency, - self.matching_context.current_host.clone(), - ); + let invalidation = + Invalidation::new(&dependency, self.matching_context.current_host.clone()); match invalidation_kind { DependencyInvalidationKind::Element => unreachable!(), diff --git a/components/style/parser.rs b/components/style/parser.rs index f506b809955..26e7cab1ad9 100644 --- a/components/style/parser.rs +++ b/components/style/parser.rs @@ -208,13 +208,19 @@ where } impl Parse for crate::OwnedStr { - fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { + fn parse<'i, 't>( + _: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { Ok(input.expect_string()?.as_ref().to_owned().into()) } } impl Parse for UnicodeRange { - fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { + fn parse<'i, 't>( + _: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { Ok(UnicodeRange::parse(input)?) } } diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 2088a5baab1..6350caef0cf 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -331,8 +331,9 @@ impl SelectorMap { .try_entry(url.clone())? .or_insert_with(SmallVec::new), Bucket::Universal => &mut self.other, - }.try_push($entry)?; - }} + } + .try_push($entry)?; + }}; } let bucket = { @@ -354,7 +355,11 @@ impl SelectorMap { // This is specially true if there's any universal selector in the // `disjoint_selectors` set, at which point we'd just be doing // wasted work. - if !disjoint_buckets.is_empty() && disjoint_buckets.iter().all(|b| b.more_specific_than(&bucket)) { + if !disjoint_buckets.is_empty() && + disjoint_buckets + .iter() + .all(|b| b.more_specific_than(&bucket)) + { for bucket in &disjoint_buckets { let entry = entry.clone(); insert_into_bucket!(entry, *bucket); diff --git a/components/style/values/computed/align.rs b/components/style/values/computed/align.rs index 5cfb53f89ee..94847fd80f0 100644 --- a/components/style/values/computed/align.rs +++ b/components/style/values/computed/align.rs @@ -10,7 +10,8 @@ use crate::values::computed::{Context, ToComputedValue}; use crate::values::specified; pub use super::specified::{ - AlignContent, AlignItems, AlignTracks, ContentDistribution, JustifyContent, JustifyTracks, SelfAlignment, + AlignContent, AlignItems, AlignTracks, ContentDistribution, JustifyContent, JustifyTracks, + SelfAlignment, }; pub use super::specified::{AlignSelf, JustifySelf}; diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index f2d29eecac3..547c1618f70 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -29,7 +29,10 @@ use std::cmp; use std::f32; #[cfg(feature = "gecko")] -pub use self::align::{AlignContent, AlignItems, AlignTracks, JustifyContent, JustifyItems, JustifyTracks, SelfAlignment}; +pub use self::align::{ + AlignContent, AlignItems, AlignTracks, JustifyContent, JustifyItems, JustifyTracks, + SelfAlignment, +}; #[cfg(feature = "gecko")] pub use self::align::{AlignSelf, JustifySelf}; pub use self::angle::Angle; @@ -69,7 +72,9 @@ pub use self::motion::{OffsetPath, OffsetRotate}; pub use self::outline::OutlineStyle; pub use self::percentage::{NonNegativePercentage, Percentage}; pub use self::position::AspectRatio; -pub use self::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto, ZIndex}; +pub use self::position::{ + GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto, ZIndex, +}; pub use self::rect::NonNegativeLengthOrNumberRect; pub use self::resolution::Resolution; pub use self::svg::MozContextProperties; diff --git a/components/style/values/specified/align.rs b/components/style/values/specified/align.rs index 4b483962f2f..51b5e625672 100644 --- a/components/style/values/specified/align.rs +++ b/components/style/values/specified/align.rs @@ -300,19 +300,14 @@ impl SpecifiedValueInfo for AlignContent { )] #[repr(transparent)] #[css(comma)] -pub struct AlignTracks( - #[css(iterable, if_empty = "normal")] - pub crate::OwnedSlice -); +pub struct AlignTracks(#[css(iterable, if_empty = "normal")] pub crate::OwnedSlice); impl Parse for AlignTracks { fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - let values = input.parse_comma_separated(|input| { - AlignContent::parse(context, input) - })?; + let values = input.parse_comma_separated(|input| AlignContent::parse(context, input))?; Ok(AlignTracks(values.into())) } } @@ -373,8 +368,7 @@ impl SpecifiedValueInfo for JustifyContent { #[repr(transparent)] #[css(comma)] pub struct JustifyTracks( - #[css(iterable, if_empty = "normal")] - pub crate::OwnedSlice + #[css(iterable, if_empty = "normal")] pub crate::OwnedSlice, ); impl Parse for JustifyTracks { @@ -382,9 +376,7 @@ impl Parse for JustifyTracks { context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - let values = input.parse_comma_separated(|input| { - JustifyContent::parse(context, input) - })?; + let values = input.parse_comma_separated(|input| JustifyContent::parse(context, input))?; Ok(JustifyTracks(values.into())) } } diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index e45be0513a4..4bb910fc97d 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -72,7 +72,9 @@ pub use self::motion::{OffsetPath, OffsetRotate}; pub use self::outline::OutlineStyle; pub use self::percentage::Percentage; pub use self::position::AspectRatio; -pub use self::position::{GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto}; +pub use self::position::{ + GridAutoFlow, GridTemplateAreas, MasonryAutoFlow, Position, PositionOrAuto, +}; pub use self::position::{PositionComponent, ZIndex}; pub use self::rect::NonNegativeLengthOrNumberRect; pub use self::resolution::Resolution; diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index 95e1e2ec712..c2998de328b 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -452,13 +452,12 @@ pub struct MasonryAutoFlow { #[inline] fn is_pack_with_non_default_order(placement: &MasonryPlacement, order: &MasonryItemOrder) -> bool { - *placement == MasonryPlacement::Pack && - *order != MasonryItemOrder::DefiniteFirst + *placement == MasonryPlacement::Pack && *order != MasonryItemOrder::DefiniteFirst } #[inline] fn is_item_order_definite_first(order: &MasonryItemOrder) -> bool { - *order == MasonryItemOrder::DefiniteFirst + *order == MasonryItemOrder::DefiniteFirst } impl MasonryAutoFlow { diff --git a/components/style_derive/parse.rs b/components/style_derive/parse.rs index 6dc2c8a715b..ebf46a3735e 100644 --- a/components/style_derive/parse.rs +++ b/components/style_derive/parse.rs @@ -154,8 +154,14 @@ pub fn derive(mut input: DeriveInput) -> TokenStream { let mut parse_non_keywords = quote! {}; for (i, (variant, css_attrs, parse_attrs)) in non_keywords.iter().enumerate() { let skip_try = !has_keywords && i == non_keywords.len() - 1; - let parse_variant = - parse_non_keyword_variant(&mut where_clause, name, variant, css_attrs, parse_attrs, skip_try); + let parse_variant = parse_non_keyword_variant( + &mut where_clause, + name, + variant, + css_attrs, + parse_attrs, + skip_try, + ); parse_non_keywords.extend(parse_variant); } From 16cb51097f2681a1c859634a63b2915b18d52cbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 4 Jun 2020 04:12:03 +0200 Subject: [PATCH 41/42] style: Fix some unit tests. We need to grow dependency by a pointer because of the parent chain for :is() / :where() unfortunately. --- tests/unit/style/size_of.rs | 2 +- tests/unit/style/stylist.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/style/size_of.rs b/tests/unit/style/size_of.rs index ed435345a28..7306cf059ef 100644 --- a/tests/unit/style/size_of.rs +++ b/tests/unit/style/size_of.rs @@ -6,7 +6,7 @@ use selectors::parser::{SelectorParseError, SelectorParseErrorKind}; use style::invalidation::element::invalidation_map::Dependency; use style::properties; -size_of_test!(test_size_of_dependency, Dependency, 16); +size_of_test!(test_size_of_dependency, Dependency, 24); size_of_test!( test_size_of_property_declaration, diff --git a/tests/unit/style/stylist.rs b/tests/unit/style/stylist.rs index 0d7511f0583..8836436c8be 100644 --- a/tests/unit/style/stylist.rs +++ b/tests/unit/style/stylist.rs @@ -201,13 +201,13 @@ fn test_insert() { 0, selector_map .class_hash - .get(&Atom::from("foo"), QuirksMode::NoQuirks) + .get(&Atom::from("intro"), QuirksMode::NoQuirks) .unwrap()[0] .source_order ); assert!(selector_map .class_hash - .get(&Atom::from("intro"), QuirksMode::NoQuirks) + .get(&Atom::from("foo"), QuirksMode::NoQuirks) .is_none()); } From 79c2c447fcdb49758b9c30a2fc0f55a630c5d21f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 4 Jun 2020 05:01:54 +0200 Subject: [PATCH 42/42] style: fix tidy. --- components/style/element_state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/element_state.rs b/components/style/element_state.rs index 3fe94f3e882..027ebc839b4 100644 --- a/components/style/element_state.rs +++ b/components/style/element_state.rs @@ -143,7 +143,7 @@ bitflags! { const IN_FOCUS_VISIBLE_STATE = 1 << 52; /// State that dialog element is modal, for centered alignment /// - /// https://html.spec.whatwg.org/#centered-alignment + /// https://html.spec.whatwg.org/multipage/#centered-alignment const IN_MODAL_DIALOG_STATE = 1 << 53; } }