style: Fix cascade order of !important in Shadow DOM.

No cleaner ideas right now that carrying that counter around... Maybe a custom
type may be cleaner?

This makes ApplicableDeclarationBlock a bit bigger. I could probably try to make
the counter a 4 / 5-bit number or something and pack the counter there in the
SourceOrderAndCascadeLevel somehow...

But doesn't seem really worth the churn, and can be done as a followup in any
case. Let me know if you want to block on that.

Bug: 1454162
Reviewed-by: heycam
MozReview-Commit-ID: 1LdW9S4xA6f
This commit is contained in:
Emilio Cobos Álvarez 2018-04-18 09:56:33 +02:00
parent 78f9672b6c
commit 84d6c13871
No known key found for this signature in database
GPG key ID: 056B727BB9C1027C
6 changed files with 133 additions and 44 deletions

View file

@ -5,7 +5,7 @@
//! Applicable declarations management. //! Applicable declarations management.
use properties::PropertyDeclarationBlock; use properties::PropertyDeclarationBlock;
use rule_tree::{CascadeLevel, StyleSource}; use rule_tree::{CascadeLevel, ShadowCascadeOrder, StyleSource};
use servo_arc::Arc; use servo_arc::Arc;
use shared_lock::Locked; use shared_lock::Locked;
use smallvec::SmallVec; use smallvec::SmallVec;
@ -83,6 +83,8 @@ pub struct ApplicableDeclarationBlock {
order_and_level: SourceOrderAndCascadeLevel, order_and_level: SourceOrderAndCascadeLevel,
/// The specificity of the selector this block is represented by. /// The specificity of the selector this block is represented by.
pub specificity: u32, pub specificity: u32,
/// The order in the tree of trees we carry on.
pub shadow_cascade_order: ShadowCascadeOrder,
} }
impl ApplicableDeclarationBlock { impl ApplicableDeclarationBlock {
@ -97,16 +99,24 @@ impl ApplicableDeclarationBlock {
source: StyleSource::Declarations(declarations), source: StyleSource::Declarations(declarations),
order_and_level: SourceOrderAndCascadeLevel::new(0, level), order_and_level: SourceOrderAndCascadeLevel::new(0, level),
specificity: 0, specificity: 0,
shadow_cascade_order: 0,
} }
} }
/// Constructs an applicable declaration block from the given components /// Constructs an applicable declaration block from the given components
#[inline] #[inline]
pub fn new(source: StyleSource, order: u32, level: CascadeLevel, specificity: u32) -> Self { pub fn new(
source: StyleSource,
order: u32,
level: CascadeLevel,
specificity: u32,
shadow_cascade_order: u32,
) -> Self {
ApplicableDeclarationBlock { ApplicableDeclarationBlock {
source: source, source,
order_and_level: SourceOrderAndCascadeLevel::new(order, level), order_and_level: SourceOrderAndCascadeLevel::new(order, level),
specificity: specificity, specificity,
shadow_cascade_order,
} }
} }
@ -122,11 +132,11 @@ impl ApplicableDeclarationBlock {
self.order_and_level.level() self.order_and_level.level()
} }
/// Convenience method to consume self and return the source alongside the /// Convenience method to consume self and return the right thing for the
/// level. /// rule tree to iterate over.
#[inline] #[inline]
pub fn order_and_level(self) -> (StyleSource, CascadeLevel) { pub fn for_rule_tree(self) -> (StyleSource, CascadeLevel, ShadowCascadeOrder) {
let level = self.level(); let level = self.level();
(self.source, level) (self.source, level, self.shadow_cascade_order)
} }
} }

View file

@ -162,6 +162,16 @@ const FREE_LIST_SENTINEL: *mut RuleNode = 0x01 as *mut RuleNode;
/// another thread is currently adding an entry). We spin if we find this value. /// another thread is currently adding an entry). We spin if we find this value.
const FREE_LIST_LOCKED: *mut RuleNode = 0x02 as *mut RuleNode; const FREE_LIST_LOCKED: *mut RuleNode = 0x02 as *mut RuleNode;
/// A counter to track how many inner shadow roots rules deep we are.
///
/// This is used to handle:
///
/// https://drafts.csswg.org/css-scoping/#shadow-cascading
///
/// In particular, it'd be `0` for the innermost shadow host, `1` for the next,
/// and so on.
pub type ShadowCascadeOrder = u32;
impl RuleTree { impl RuleTree {
/// Construct a new rule tree. /// Construct a new rule tree.
pub fn new() -> Self { pub fn new() -> Self {
@ -198,7 +208,7 @@ impl RuleTree {
guards: &StylesheetGuards, guards: &StylesheetGuards,
) -> StrongRuleNode ) -> StrongRuleNode
where where
I: Iterator<Item = (StyleSource, CascadeLevel)>, I: Iterator<Item = (StyleSource, CascadeLevel, ShadowCascadeOrder)>,
{ {
use self::CascadeLevel::*; use self::CascadeLevel::*;
let mut current = self.root.clone(); let mut current = self.root.clone();
@ -206,13 +216,18 @@ impl RuleTree {
let mut found_important = false; let mut found_important = false;
let mut important_style_attr = None; let mut important_style_attr = None;
let mut important_author = SmallVec::<[StyleSource; 4]>::new();
let mut important_same_tree = SmallVec::<[StyleSource; 4]>::new();
let mut important_inner_shadow = SmallVec::<[SmallVec<[StyleSource; 4]>; 4]>::new();
important_inner_shadow.push(SmallVec::new());
let mut important_user = SmallVec::<[StyleSource; 4]>::new(); let mut important_user = SmallVec::<[StyleSource; 4]>::new();
let mut important_ua = SmallVec::<[StyleSource; 4]>::new(); let mut important_ua = SmallVec::<[StyleSource; 4]>::new();
let mut transition = None; let mut transition = None;
for (source, level) in iter { let mut last_cascade_order = 0;
debug_assert!(last_level <= level, "Not really ordered"); for (source, level, shadow_cascade_order) in iter {
debug_assert!(level >= last_level, "Not really ordered");
debug_assert!(!level.is_important(), "Important levels handled internally"); debug_assert!(!level.is_important(), "Important levels handled internally");
let any_important = { let any_important = {
let pdb = source.read(level.guard(guards)); let pdb = source.read(level.guard(guards));
@ -222,7 +237,22 @@ impl RuleTree {
if any_important { if any_important {
found_important = true; found_important = true;
match level { match level {
AuthorNormal => important_author.push(source.clone()), InnerShadowNormal => {
debug_assert!(
shadow_cascade_order >= last_cascade_order,
"Not really ordered"
);
if shadow_cascade_order > last_cascade_order &&
!important_inner_shadow.last().unwrap().is_empty()
{
last_cascade_order = shadow_cascade_order;
important_inner_shadow.push(SmallVec::new());
}
important_inner_shadow.last_mut().unwrap().push(source.clone())
}
SameTreeAuthorNormal => {
important_same_tree.push(source.clone())
},
UANormal => important_ua.push(source.clone()), UANormal => important_ua.push(source.clone()),
UserNormal => important_user.push(source.clone()), UserNormal => important_user.push(source.clone()),
StyleAttributeNormal => { StyleAttributeNormal => {
@ -265,14 +295,20 @@ impl RuleTree {
// followed by any transition rule. // followed by any transition rule.
// //
for source in important_author.drain() { for source in important_same_tree.drain() {
current = current.ensure_child(self.root.downgrade(), source, AuthorImportant); current = current.ensure_child(self.root.downgrade(), source, SameTreeAuthorImportant);
} }
if let Some(source) = important_style_attr { if let Some(source) = important_style_attr {
current = current.ensure_child(self.root.downgrade(), source, StyleAttributeImportant); current = current.ensure_child(self.root.downgrade(), source, StyleAttributeImportant);
} }
for mut list in important_inner_shadow.drain().rev() {
for source in list.drain() {
current = current.ensure_child(self.root.downgrade(), source, InnerShadowImportant);
}
}
for source in important_user.drain() { for source in important_user.drain() {
current = current.ensure_child(self.root.downgrade(), source, UserImportant); current = current.ensure_child(self.root.downgrade(), source, UserImportant);
} }
@ -295,9 +331,10 @@ impl RuleTree {
applicable_declarations: &mut ApplicableDeclarationList, applicable_declarations: &mut ApplicableDeclarationList,
guards: &StylesheetGuards, guards: &StylesheetGuards,
) -> StrongRuleNode { ) -> StrongRuleNode {
let rules = applicable_declarations.drain().map(|d| d.order_and_level()); self.insert_ordered_rules_with_important(
let rule_node = self.insert_ordered_rules_with_important(rules, guards); applicable_declarations.drain().map(|d| d.for_rule_tree()),
rule_node guards,
)
} }
/// Insert the given rules, that must be in proper order by specifity, and /// Insert the given rules, that must be in proper order by specifity, and
@ -381,8 +418,8 @@ impl RuleTree {
// also equally valid. This is less likely, and would require an // also equally valid. This is less likely, and would require an
// in-place mutation of the source, which is, at best, fiddly, // in-place mutation of the source, which is, at best, fiddly,
// so let's skip it for now. // so let's skip it for now.
let is_here_already = match &current.get().source { let is_here_already = match current.get().source {
&StyleSource::Declarations(ref already_here) => { StyleSource::Declarations(ref already_here) => {
pdb.with_arc(|arc| Arc::ptr_eq(arc, already_here)) pdb.with_arc(|arc| Arc::ptr_eq(arc, already_here))
}, },
_ => unreachable!("Replacing non-declarations style?"), _ => unreachable!("Replacing non-declarations style?"),
@ -500,9 +537,22 @@ const RULE_TREE_GC_INTERVAL: usize = 300;
/// The order of variants declared here is significant, and must be in /// The order of variants declared here is significant, and must be in
/// _ascending_ order of precedence. /// _ascending_ order of precedence.
/// ///
/// See also [4] for the Shadow DOM bits. We rely on the invariant that rules
/// from outside the tree the element is in can't affect the element.
///
/// The opposite is not true (i.e., :host and ::slotted) from an "inner" shadow
/// tree may affect an element connected to the document or an "outer" shadow
/// tree.
///
/// We need to differentiate between rules from the same tree and "inner" shadow
/// trees in order to be able to find the right position for the style attribute
/// easily. Otherwise we wouldn't be able to avoid selector-matching when a
/// style attribute is added or removed.
///
/// [1]: https://drafts.csswg.org/css-cascade/#cascade-origin /// [1]: https://drafts.csswg.org/css-cascade/#cascade-origin
/// [2]: https://drafts.csswg.org/css-cascade/#preshint /// [2]: https://drafts.csswg.org/css-cascade/#preshint
/// [3]: https://html.spec.whatwg.org/multipage/#presentational-hints /// [3]: https://html.spec.whatwg.org/multipage/#presentational-hints
/// [4]: https://drafts.csswg.org/css-scoping/#shadow-cascading
#[repr(u8)] #[repr(u8)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd)] #[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd)]
#[cfg_attr(feature = "servo", derive(MallocSizeOf))] #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
@ -513,18 +563,27 @@ pub enum CascadeLevel {
UserNormal, UserNormal,
/// Presentational hints. /// Presentational hints.
PresHints, PresHints,
/// Author normal rules. /// Shadow DOM styles from "inner" shadow trees.
AuthorNormal, ///
/// See above for why this is needed instead of merging InnerShadowNormal,
/// SameTreeAuthorNormal and StyleAttributeNormal inside something like
/// AuthorNormal.
InnerShadowNormal,
/// Author normal rules from the same tree the element is in.
SameTreeAuthorNormal,
/// Style attribute normal rules. /// Style attribute normal rules.
StyleAttributeNormal, StyleAttributeNormal,
/// SVG SMIL animations. /// SVG SMIL animations.
SMILOverride, SMILOverride,
/// CSS animations and script-generated animations. /// CSS animations and script-generated animations.
Animations, Animations,
/// Author-supplied important rules. /// Author-supplied important rules from the same tree the element came
AuthorImportant, /// from.
SameTreeAuthorImportant,
/// Style attribute important rules. /// Style attribute important rules.
StyleAttributeImportant, StyleAttributeImportant,
/// Shadow DOM important rules.
InnerShadowImportant,
/// User important rules. /// User important rules.
UserImportant, UserImportant,
/// User-agent important rules. /// User-agent important rules.
@ -571,7 +630,8 @@ impl CascadeLevel {
#[inline] #[inline]
pub fn is_important(&self) -> bool { pub fn is_important(&self) -> bool {
match *self { match *self {
CascadeLevel::AuthorImportant | CascadeLevel::SameTreeAuthorImportant |
CascadeLevel::InnerShadowImportant |
CascadeLevel::StyleAttributeImportant | CascadeLevel::StyleAttributeImportant |
CascadeLevel::UserImportant | CascadeLevel::UserImportant |
CascadeLevel::UAImportant => true, CascadeLevel::UAImportant => true,
@ -1302,11 +1362,13 @@ impl StrongRuleNode {
}, },
// Author rules: // Author rules:
CascadeLevel::PresHints | CascadeLevel::PresHints |
CascadeLevel::AuthorNormal | CascadeLevel::SameTreeAuthorNormal |
CascadeLevel::InnerShadowNormal |
CascadeLevel::StyleAttributeNormal | CascadeLevel::StyleAttributeNormal |
CascadeLevel::SMILOverride | CascadeLevel::SMILOverride |
CascadeLevel::Animations | CascadeLevel::Animations |
CascadeLevel::AuthorImportant | CascadeLevel::SameTreeAuthorImportant |
CascadeLevel::InnerShadowImportant |
CascadeLevel::StyleAttributeImportant | CascadeLevel::StyleAttributeImportant |
CascadeLevel::Transitions => { CascadeLevel::Transitions => {
for (id, declaration) in longhands { for (id, declaration) in longhands {

View file

@ -14,7 +14,7 @@ use hash::{HashMap, HashSet};
use hash::map as hash_map; use hash::map as hash_map;
use hashglobe::FailedAllocationError; use hashglobe::FailedAllocationError;
use precomputed_hash::PrecomputedHash; use precomputed_hash::PrecomputedHash;
use rule_tree::CascadeLevel; use rule_tree::{CascadeLevel, ShadowCascadeOrder};
use selector_parser::SelectorImpl; use selector_parser::SelectorImpl;
use selectors::matching::{matches_selector, ElementSelectorFlags, MatchingContext}; use selectors::matching::{matches_selector, ElementSelectorFlags, MatchingContext};
use selectors::parser::{Combinator, Component, SelectorIter}; use selectors::parser::{Combinator, Component, SelectorIter};
@ -163,6 +163,7 @@ impl SelectorMap<Rule> {
context: &mut MatchingContext<E::Impl>, context: &mut MatchingContext<E::Impl>,
flags_setter: &mut F, flags_setter: &mut F,
cascade_level: CascadeLevel, cascade_level: CascadeLevel,
shadow_cascade_order: ShadowCascadeOrder,
) where ) where
E: TElement, E: TElement,
F: FnMut(&E, ElementSelectorFlags), F: FnMut(&E, ElementSelectorFlags),
@ -185,6 +186,7 @@ impl SelectorMap<Rule> {
context, context,
flags_setter, flags_setter,
cascade_level, cascade_level,
shadow_cascade_order,
) )
} }
} }
@ -198,6 +200,7 @@ impl SelectorMap<Rule> {
context, context,
flags_setter, flags_setter,
cascade_level, cascade_level,
shadow_cascade_order,
) )
} }
}); });
@ -210,6 +213,7 @@ impl SelectorMap<Rule> {
context, context,
flags_setter, flags_setter,
cascade_level, cascade_level,
shadow_cascade_order,
) )
} }
@ -220,6 +224,7 @@ impl SelectorMap<Rule> {
context, context,
flags_setter, flags_setter,
cascade_level, cascade_level,
shadow_cascade_order,
); );
// Sort only the rules we just added. // Sort only the rules we just added.
@ -235,6 +240,7 @@ impl SelectorMap<Rule> {
context: &mut MatchingContext<E::Impl>, context: &mut MatchingContext<E::Impl>,
flags_setter: &mut F, flags_setter: &mut F,
cascade_level: CascadeLevel, cascade_level: CascadeLevel,
shadow_cascade_order: ShadowCascadeOrder,
) where ) where
E: TElement, E: TElement,
F: FnMut(&E, ElementSelectorFlags), F: FnMut(&E, ElementSelectorFlags),
@ -248,7 +254,7 @@ impl SelectorMap<Rule> {
context, context,
flags_setter, flags_setter,
) { ) {
matching_rules.push(rule.to_applicable_declaration_block(cascade_level)); matching_rules.push(rule.to_applicable_declaration_block(cascade_level, shadow_cascade_order));
} }
} }
} }

View file

@ -23,7 +23,7 @@ use media_queries::Device;
use properties::{self, CascadeFlags, ComputedValues}; use properties::{self, CascadeFlags, ComputedValues};
use properties::{AnimationRules, PropertyDeclarationBlock}; use properties::{AnimationRules, PropertyDeclarationBlock};
use rule_cache::{RuleCache, RuleCacheConditions}; use rule_cache::{RuleCache, RuleCacheConditions};
use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource}; use rule_tree::{CascadeLevel, RuleTree, ShadowCascadeOrder, StrongRuleNode, StyleSource};
use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry}; use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry};
use selector_parser::{PerPseudoElementMap, PseudoElement, SelectorImpl, SnapshotMap}; use selector_parser::{PerPseudoElementMap, PseudoElement, SelectorImpl, SnapshotMap};
use selectors::NthIndexCache; use selectors::NthIndexCache;
@ -693,7 +693,7 @@ impl Stylist {
match declarations { match declarations {
Some(decls) => self.rule_tree.insert_ordered_rules_with_important( Some(decls) => self.rule_tree.insert_ordered_rules_with_important(
decls.into_iter().map(|a| (a.source.clone(), a.level())), decls.into_iter().map(|a| a.clone().for_rule_tree()),
guards, guards,
), ),
None => self.rule_tree.root().clone(), None => self.rule_tree.root().clone(),
@ -1020,7 +1020,7 @@ impl Stylist {
); );
if !declarations.is_empty() { if !declarations.is_empty() {
let rule_node = self.rule_tree.insert_ordered_rules_with_important( let rule_node = self.rule_tree.insert_ordered_rules_with_important(
declarations.drain().map(|a| a.order_and_level()), declarations.drain().map(|a| a.for_rule_tree()),
guards, guards,
); );
if rule_node != *self.rule_tree.root() { if rule_node != *self.rule_tree.root() {
@ -1187,6 +1187,7 @@ impl Stylist {
context, context,
flags_setter, flags_setter,
CascadeLevel::UANormal, CascadeLevel::UANormal,
0,
); );
} }
@ -1208,6 +1209,7 @@ impl Stylist {
context, context,
flags_setter, flags_setter,
CascadeLevel::UserNormal, CascadeLevel::UserNormal,
0,
); );
} }
} }
@ -1232,6 +1234,7 @@ impl Stylist {
} }
let mut match_document_author_rules = matches_author_rules; let mut match_document_author_rules = matches_author_rules;
let mut shadow_cascade_order = 0;
// XBL / Shadow DOM rules, which are author rules too. // XBL / Shadow DOM rules, which are author rules too.
// //
@ -1249,9 +1252,11 @@ impl Stylist {
applicable_declarations, applicable_declarations,
context, context,
flags_setter, flags_setter,
CascadeLevel::AuthorNormal, CascadeLevel::InnerShadowNormal,
shadow_cascade_order,
); );
}); });
shadow_cascade_order += 1;
} }
} }
@ -1275,9 +1280,11 @@ impl Stylist {
applicable_declarations, applicable_declarations,
context, context,
flags_setter, flags_setter,
CascadeLevel::AuthorNormal, CascadeLevel::InnerShadowNormal,
shadow_cascade_order,
); );
}); });
shadow_cascade_order += 1;
} }
} }
@ -1291,9 +1298,11 @@ impl Stylist {
applicable_declarations, applicable_declarations,
context, context,
flags_setter, flags_setter,
CascadeLevel::AuthorNormal, CascadeLevel::SameTreeAuthorNormal,
shadow_cascade_order,
); );
}); });
shadow_cascade_order += 1;
} }
match_document_author_rules = false; match_document_author_rules = false;
@ -1309,10 +1318,6 @@ impl Stylist {
if let Some(map) = cascade_data.normal_rules(pseudo_element) { if let Some(map) = cascade_data.normal_rules(pseudo_element) {
// NOTE(emilio): This is needed because the XBL stylist may // NOTE(emilio): This is needed because the XBL stylist may
// think it has a different quirks mode than the document. // think it has a different quirks mode than the document.
//
// FIXME(emilio): this should use the same VisitedMatchingMode
// as `context`, write a test-case of :visited not working on
// Shadow DOM and fix it!
let mut matching_context = MatchingContext::new( let mut matching_context = MatchingContext::new(
context.matching_mode(), context.matching_mode(),
context.bloom_filter, context.bloom_filter,
@ -1322,13 +1327,16 @@ impl Stylist {
matching_context.pseudo_element_matching_fn = matching_context.pseudo_element_matching_fn =
context.pseudo_element_matching_fn; context.pseudo_element_matching_fn;
// SameTreeAuthorNormal instead of InnerShadowNormal to
// preserve behavior, though that's kinda fishy...
map.get_all_matching_rules( map.get_all_matching_rules(
element, element,
rule_hash_target, rule_hash_target,
applicable_declarations, applicable_declarations,
&mut matching_context, &mut matching_context,
flags_setter, flags_setter,
CascadeLevel::AuthorNormal, CascadeLevel::SameTreeAuthorNormal,
shadow_cascade_order,
); );
} }
}); });
@ -1344,7 +1352,8 @@ impl Stylist {
applicable_declarations, applicable_declarations,
context, context,
flags_setter, flags_setter,
CascadeLevel::AuthorNormal, CascadeLevel::SameTreeAuthorNormal,
shadow_cascade_order,
); );
} }
} }
@ -2172,6 +2181,7 @@ impl CascadeData {
self.rules_source_order, self.rules_source_order,
CascadeLevel::UANormal, CascadeLevel::UANormal,
selector.specificity(), selector.specificity(),
0,
)); ));
continue; continue;
} }
@ -2468,9 +2478,10 @@ impl Rule {
pub fn to_applicable_declaration_block( pub fn to_applicable_declaration_block(
&self, &self,
level: CascadeLevel, level: CascadeLevel,
shadow_cascade_order: ShadowCascadeOrder,
) -> ApplicableDeclarationBlock { ) -> ApplicableDeclarationBlock {
let source = StyleSource::Style(self.style_rule.clone()); let source = StyleSource::Style(self.style_rule.clone());
ApplicableDeclarationBlock::new(source, self.source_order, level, self.specificity()) ApplicableDeclarationBlock::new(source, self.source_order, level, self.specificity(), shadow_cascade_order)
} }
/// Creates a new Rule. /// Creates a new Rule.

View file

@ -2756,7 +2756,7 @@ pub unsafe extern "C" fn Servo_ComputedValues_GetForAnonymousBox(
let level = match origin { let level = match origin {
Origin::UserAgent => CascadeLevel::UANormal, Origin::UserAgent => CascadeLevel::UANormal,
Origin::User => CascadeLevel::UserNormal, Origin::User => CascadeLevel::UserNormal,
Origin::Author => CascadeLevel::AuthorNormal, Origin::Author => CascadeLevel::SameTreeAuthorNormal,
}; };
for rule in data.pages.iter() { for rule in data.pages.iter() {
declarations.push(ApplicableDeclarationBlock::from_declarations( declarations.push(ApplicableDeclarationBlock::from_declarations(

View file

@ -35,7 +35,7 @@ size_of_test!(test_size_of_element_data, ElementData, 24);
size_of_test!(test_size_of_property_declaration, style::properties::PropertyDeclaration, 32); size_of_test!(test_size_of_property_declaration, style::properties::PropertyDeclaration, 32);
size_of_test!(test_size_of_application_declaration_block, ApplicableDeclarationBlock, 24); size_of_test!(test_size_of_application_declaration_block, ApplicableDeclarationBlock, 32);
size_of_test!(test_size_of_rule_node, RuleNode, 80); size_of_test!(test_size_of_rule_node, RuleNode, 80);
// This is huge, but we allocate it on the stack and then never move it, // This is huge, but we allocate it on the stack and then never move it,