Bug 1454162: Fix cascade order of !important in Shadow DOM. r=heycam

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.

MozReview-Commit-ID: 1LdW9S4xA6f
This commit is contained in:
Emilio Cobos Álvarez 2018-04-18 09:56:33 +02:00
Родитель 11d031b2f8
Коммит 630760c6c7
7 изменённых файлов: 133 добавлений и 87 удалений

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

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

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

@ -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.
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 {
/// Construct a new rule tree.
pub fn new() -> Self {
@ -198,7 +208,7 @@ impl RuleTree {
guards: &StylesheetGuards,
) -> StrongRuleNode
where
I: Iterator<Item = (StyleSource, CascadeLevel)>,
I: Iterator<Item = (StyleSource, CascadeLevel, ShadowCascadeOrder)>,
{
use self::CascadeLevel::*;
let mut current = self.root.clone();
@ -206,13 +216,18 @@ impl RuleTree {
let mut found_important = false;
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_ua = SmallVec::<[StyleSource; 4]>::new();
let mut transition = None;
for (source, level) in iter {
debug_assert!(last_level <= level, "Not really ordered");
let mut last_cascade_order = 0;
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");
let any_important = {
let pdb = source.read(level.guard(guards));
@ -222,7 +237,22 @@ impl RuleTree {
if any_important {
found_important = true;
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()),
UserNormal => important_user.push(source.clone()),
StyleAttributeNormal => {
@ -265,14 +295,20 @@ impl RuleTree {
// followed by any transition rule.
//
for source in important_author.drain() {
current = current.ensure_child(self.root.downgrade(), source, AuthorImportant);
for source in important_same_tree.drain() {
current = current.ensure_child(self.root.downgrade(), source, SameTreeAuthorImportant);
}
if let Some(source) = important_style_attr {
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() {
current = current.ensure_child(self.root.downgrade(), source, UserImportant);
}
@ -295,9 +331,10 @@ impl RuleTree {
applicable_declarations: &mut ApplicableDeclarationList,
guards: &StylesheetGuards,
) -> StrongRuleNode {
let rules = applicable_declarations.drain().map(|d| d.order_and_level());
let rule_node = self.insert_ordered_rules_with_important(rules, guards);
rule_node
self.insert_ordered_rules_with_important(
applicable_declarations.drain().map(|d| d.for_rule_tree()),
guards,
)
}
/// 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
// in-place mutation of the source, which is, at best, fiddly,
// so let's skip it for now.
let is_here_already = match &current.get().source {
&StyleSource::Declarations(ref already_here) => {
let is_here_already = match current.get().source {
StyleSource::Declarations(ref already_here) => {
pdb.with_arc(|arc| Arc::ptr_eq(arc, already_here))
},
_ => 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
/// _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
/// [2]: https://drafts.csswg.org/css-cascade/#preshint
/// [3]: https://html.spec.whatwg.org/multipage/#presentational-hints
/// [4]: https://drafts.csswg.org/css-scoping/#shadow-cascading
#[repr(u8)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd)]
#[cfg_attr(feature = "servo", derive(MallocSizeOf))]
@ -513,18 +563,27 @@ pub enum CascadeLevel {
UserNormal,
/// Presentational hints.
PresHints,
/// Author normal rules.
AuthorNormal,
/// Shadow DOM styles from "inner" shadow trees.
///
/// 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.
StyleAttributeNormal,
/// SVG SMIL animations.
SMILOverride,
/// CSS animations and script-generated animations.
Animations,
/// Author-supplied important rules.
AuthorImportant,
/// Author-supplied important rules from the same tree the element came
/// from.
SameTreeAuthorImportant,
/// Style attribute important rules.
StyleAttributeImportant,
/// Shadow DOM important rules.
InnerShadowImportant,
/// User important rules.
UserImportant,
/// User-agent important rules.
@ -571,7 +630,8 @@ impl CascadeLevel {
#[inline]
pub fn is_important(&self) -> bool {
match *self {
CascadeLevel::AuthorImportant |
CascadeLevel::SameTreeAuthorImportant |
CascadeLevel::InnerShadowImportant |
CascadeLevel::StyleAttributeImportant |
CascadeLevel::UserImportant |
CascadeLevel::UAImportant => true,
@ -1302,11 +1362,13 @@ impl StrongRuleNode {
},
// Author rules:
CascadeLevel::PresHints |
CascadeLevel::AuthorNormal |
CascadeLevel::SameTreeAuthorNormal |
CascadeLevel::InnerShadowNormal |
CascadeLevel::StyleAttributeNormal |
CascadeLevel::SMILOverride |
CascadeLevel::Animations |
CascadeLevel::AuthorImportant |
CascadeLevel::SameTreeAuthorImportant |
CascadeLevel::InnerShadowImportant |
CascadeLevel::StyleAttributeImportant |
CascadeLevel::Transitions => {
for (id, declaration) in longhands {

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

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

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

@ -23,7 +23,7 @@ use media_queries::Device;
use properties::{self, CascadeFlags, ComputedValues};
use properties::{AnimationRules, PropertyDeclarationBlock};
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_parser::{PerPseudoElementMap, PseudoElement, SelectorImpl, SnapshotMap};
use selectors::NthIndexCache;
@ -693,7 +693,7 @@ impl Stylist {
match declarations {
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,
),
None => self.rule_tree.root().clone(),
@ -1020,7 +1020,7 @@ impl Stylist {
);
if !declarations.is_empty() {
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,
);
if rule_node != *self.rule_tree.root() {
@ -1187,6 +1187,7 @@ impl Stylist {
context,
flags_setter,
CascadeLevel::UANormal,
0,
);
}
@ -1208,6 +1209,7 @@ impl Stylist {
context,
flags_setter,
CascadeLevel::UserNormal,
0,
);
}
}
@ -1232,6 +1234,7 @@ impl Stylist {
}
let mut match_document_author_rules = matches_author_rules;
let mut shadow_cascade_order = 0;
// XBL / Shadow DOM rules, which are author rules too.
//
@ -1249,9 +1252,11 @@ impl Stylist {
applicable_declarations,
context,
flags_setter,
CascadeLevel::AuthorNormal,
CascadeLevel::InnerShadowNormal,
shadow_cascade_order,
);
});
shadow_cascade_order += 1;
}
}
@ -1275,9 +1280,11 @@ impl Stylist {
applicable_declarations,
context,
flags_setter,
CascadeLevel::AuthorNormal,
CascadeLevel::InnerShadowNormal,
shadow_cascade_order,
);
});
shadow_cascade_order += 1;
}
}
@ -1291,9 +1298,11 @@ impl Stylist {
applicable_declarations,
context,
flags_setter,
CascadeLevel::AuthorNormal,
CascadeLevel::SameTreeAuthorNormal,
shadow_cascade_order,
);
});
shadow_cascade_order += 1;
}
match_document_author_rules = false;
@ -1309,10 +1318,6 @@ impl Stylist {
if let Some(map) = cascade_data.normal_rules(pseudo_element) {
// NOTE(emilio): This is needed because the XBL stylist may
// 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(
context.matching_mode(),
context.bloom_filter,
@ -1322,13 +1327,16 @@ impl Stylist {
matching_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(
element,
rule_hash_target,
applicable_declarations,
&mut matching_context,
flags_setter,
CascadeLevel::AuthorNormal,
CascadeLevel::SameTreeAuthorNormal,
shadow_cascade_order,
);
}
});
@ -1344,7 +1352,8 @@ impl Stylist {
applicable_declarations,
context,
flags_setter,
CascadeLevel::AuthorNormal,
CascadeLevel::SameTreeAuthorNormal,
shadow_cascade_order,
);
}
}
@ -2172,6 +2181,7 @@ impl CascadeData {
self.rules_source_order,
CascadeLevel::UANormal,
selector.specificity(),
0,
));
continue;
}
@ -2468,9 +2478,10 @@ impl Rule {
pub fn to_applicable_declaration_block(
&self,
level: CascadeLevel,
shadow_cascade_order: ShadowCascadeOrder,
) -> ApplicableDeclarationBlock {
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.

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

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

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

@ -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_application_declaration_block, ApplicableDeclarationBlock, 24);
size_of_test!(test_size_of_application_declaration_block, ApplicableDeclarationBlock, 28);
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,

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

@ -1,43 +0,0 @@
[shadow-cascade-order-001.html]
[D1. document vs ::slotted both with !important, ::slotted rule should win for open mode.]
expected: FAIL
[D2. document vs :host both with !important, :host rule should win for open mode.]
expected: FAIL
[D4. ::slotted vs :host both with !important, later in tree-of-trees rule should win for open mode.]
expected: FAIL
[D5. ::slotted vs inline both with !important, ::slotted rule should win for open mode.]
expected: FAIL
[D6. :host vs inline both with !important, :host rule should win for open mode.]
expected: FAIL
[E2. all styles with !important applied, rule in the last tree-of-trees should win for open mode.]
expected: FAIL
[F6. all rules with !important, the last rule in tree-of-trees should win for open mode.]
expected: FAIL
[D1. document vs ::slotted both with !important, ::slotted rule should win for closed mode.]
expected: FAIL
[D2. document vs :host both with !important, :host rule should win for closed mode.]
expected: FAIL
[D4. ::slotted vs :host both with !important, later in tree-of-trees rule should win for closed mode.]
expected: FAIL
[D5. ::slotted vs inline both with !important, ::slotted rule should win for closed mode.]
expected: FAIL
[D6. :host vs inline both with !important, :host rule should win for closed mode.]
expected: FAIL
[E2. all styles with !important applied, rule in the last tree-of-trees should win for closed mode.]
expected: FAIL
[F6. all rules with !important, the last rule in tree-of-trees should win for closed mode.]
expected: FAIL