From 206ca74d01d776c72de09acdeafcd4dfc1acb62b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 8 Apr 2024 08:44:42 +0000 Subject: [PATCH] Bug 1864418 - Append implicit parent selector during parsing. r=dshin,devtools-reviewers,nchevobbe This matches recent spec changes. Differential Revision: https://phabricator.services.mozilla.com/D206766 --- .../test/browser_changes_nested_rules.js | 4 +- .../rules/test/browser_rules_content_01.js | 2 +- .../rules/test/browser_rules_nested_rules.js | 6 +- .../browser_rules_select-and-copy-styles.js | 2 +- ...rules_selector-highlighter-nested-rules.js | 4 +- servo/components/selectors/builder.rs | 196 +++++++-------- servo/components/selectors/parser.rs | 228 ++++++++---------- .../css-nesting/conditional-rules.html.ini | 2 - .../meta/css/css-nesting/parsing.html.ini | 15 -- 9 files changed, 214 insertions(+), 245 deletions(-) delete mode 100644 testing/web-platform/meta/css/css-nesting/parsing.html.ini diff --git a/devtools/client/inspector/changes/test/browser_changes_nested_rules.js b/devtools/client/inspector/changes/test/browser_changes_nested_rules.js index 789d88fdda0a..d6da1af72d9d 100644 --- a/devtools/client/inspector/changes/test/browser_changes_nested_rules.js +++ b/devtools/client/inspector/changes/test/browser_changes_nested_rules.js @@ -12,7 +12,7 @@ // --- @container myContainer (width > 10px) { // ----- div { // ------- & > span { … } -// ------- .mySpan { +// ------- & .mySpan { // --------- &:not(:focus) { const spanNotFocusedRule = `&:not(:focus) { @@ -94,7 +94,7 @@ const EXPECTED_AFTER_SPAN_PROP_CHANGES = EXPECTED_AFTER_DIV_PROP_CHANGE.map( }) ).concat([ { - text: ".mySpan {", + text: "& .mySpan {", copyRuleClipboard: applyModificationAfterSpanPropertiesChange(spanClassRule), }, diff --git a/devtools/client/inspector/rules/test/browser_rules_content_01.js b/devtools/client/inspector/rules/test/browser_rules_content_01.js index b92ec47db031..1985f07f5f87 100644 --- a/devtools/client/inspector/rules/test/browser_rules_content_01.js +++ b/devtools/client/inspector/rules/test/browser_rules_content_01.js @@ -103,7 +103,7 @@ add_task(async function () { matches: true, }, { - selector: ".unmatched", + selector: "& .unmatched", matches: false, }, ]); diff --git a/devtools/client/inspector/rules/test/browser_rules_nested_rules.js b/devtools/client/inspector/rules/test/browser_rules_nested_rules.js index 925f36a0e681..0dc068232bed 100644 --- a/devtools/client/inspector/rules/test/browser_rules_nested_rules.js +++ b/devtools/client/inspector/rules/test/browser_rules_nested_rules.js @@ -93,7 +93,7 @@ add_task(async function () { checkRuleViewContent(view, [ { selector: "element", ancestorRulesData: null, declarations: [] }, { - selector: `.foo`, + selector: `& .foo`, // prettier-ignore ancestorRulesData: [ `body {`, @@ -108,7 +108,7 @@ add_task(async function () { checkRuleViewContent(view, [ { selector: "element", ancestorRulesData: null, declarations: [] }, { - selector: `#bar`, + selector: `& #bar`, // prettier-ignore ancestorRulesData: [ `body {`, @@ -138,7 +138,7 @@ add_task(async function () { checkRuleViewContent(view, [ { selector: "element", ancestorRulesData: null, declarations: [] }, { - selector: `[href]`, + selector: `& [href]`, ancestorRulesData: [ `body {`, ` @media screen {`, diff --git a/devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js b/devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js index f9d245828ee7..48b12fda481d 100644 --- a/devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js +++ b/devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js @@ -211,7 +211,7 @@ async function checkCopyNestedRule(view) { const expectedNested = `html { body { @container (1px < width) { - #nested { + & #nested { background: tomato; color: gold; } diff --git a/devtools/client/inspector/rules/test/browser_rules_selector-highlighter-nested-rules.js b/devtools/client/inspector/rules/test/browser_rules_selector-highlighter-nested-rules.js index 4a5e8bcd0c89..ecf41fb9204f 100644 --- a/devtools/client/inspector/rules/test/browser_rules_selector-highlighter-nested-rules.js +++ b/devtools/client/inspector/rules/test/browser_rules_selector-highlighter-nested-rules.js @@ -88,8 +88,8 @@ add_task(async function () { ); ok(highlighterData.isShown, "The selector highlighter was shown"); - info(`Clicking on ".title" selector icon`); - highlighterData = await clickSelectorIcon(view, ".title"); + info(`Clicking on "& .title" selector icon`); + highlighterData = await clickSelectorIcon(view, "& .title"); is( highlighterData.nodeFront.nodeName.toLowerCase(), "h1", diff --git a/servo/components/selectors/builder.rs b/servo/components/selectors/builder.rs index c05d6e609481..c70655418512 100644 --- a/servo/components/selectors/builder.rs +++ b/servo/components/selectors/builder.rs @@ -6,59 +6,45 @@ //! //! Our selector representation is designed to optimize matching, and has //! several requirements: -//! * All simple selectors and combinators are stored inline in the same buffer -//! as Component instances. -//! * We store the top-level compound selectors from right to left, i.e. in -//! matching order. -//! * We store the simple selectors for each combinator from left to right, so -//! that we match the cheaper simple selectors first. +//! * All simple selectors and combinators are stored inline in the same buffer as Component +//! instances. +//! * We store the top-level compound selectors from right to left, i.e. in matching order. +//! * We store the simple selectors for each combinator from left to right, so that we match the +//! cheaper simple selectors first. //! -//! Meeting all these constraints without extra memmove traffic during parsing -//! is non-trivial. This module encapsulates those details and presents an -//! easy-to-use API for the parser. +//! For example, the selector: +//! +//! .bar:hover > .baz:nth-child(2) + .qux +//! +//! Gets stored as: +//! +//! [.qux, + , .baz, :nth-child(2), > , .bar, :hover] +//! +//! Meeting all these constraints without extra memmove traffic during parsing is non-trivial. This +//! module encapsulates those details and presents an easy-to-use API for the parser. -use crate::parser::{Combinator, Component, RelativeSelector, Selector, SelectorImpl}; +use crate::parser::{Combinator, Component, RelativeSelector, Selector, SelectorImpl, ParseRelative}; use crate::sink::Push; use servo_arc::{Arc, ThinArc}; use smallvec::SmallVec; use std::cmp; -use std::iter; -use std::ptr; use std::slice; -/// Top-level SelectorBuilder struct. This should be stack-allocated by the -/// consumer and never moved (because it contains a lot of inline data that -/// would be slow to memmov). +/// Top-level SelectorBuilder struct. This should be stack-allocated by the consumer and never +/// moved (because it contains a lot of inline data that would be slow to memmove). /// -/// After instantation, callers may call the push_simple_selector() and -/// push_combinator() methods to append selector data as it is encountered -/// (from left to right). Once the process is complete, callers should invoke -/// build(), which transforms the contents of the SelectorBuilder into a heap- -/// allocated Selector and leaves the builder in a drained state. +/// After instantiation, callers may call the push_simple_selector() and push_combinator() methods +/// to append selector data as it is encountered (from left to right). Once the process is +/// complete, callers should invoke build(), which transforms the contents of the SelectorBuilder +/// into a heap- allocated Selector and leaves the builder in a drained state. #[derive(Debug)] pub struct SelectorBuilder { - /// The entire sequence of simple selectors, from left to right, without combinators. - /// - /// We make this large because the result of parsing a selector is fed into a new - /// Arc-ed allocation, so any spilled vec would be a wasted allocation. Also, - /// Components are large enough that we don't have much cache locality benefit - /// from reserving stack space for fewer of them. - simple_selectors: SmallVec<[Component; 32]>, - /// The combinators, and the length of the compound selector to their left. - combinators: SmallVec<[(Combinator, usize); 16]>, - /// The length of the current compount selector. - current_len: usize, -} - -impl Default for SelectorBuilder { - #[inline(always)] - fn default() -> Self { - SelectorBuilder { - simple_selectors: SmallVec::new(), - combinators: SmallVec::new(), - current_len: 0, - } - } + /// The entire sequence of components. We make this large because the result of parsing a + /// selector is fed into a new Arc-ed allocation, so any spilled vec would be a wasted + /// allocation. Also, Components are large enough that we don't have much cache locality + /// benefit from reserving stack space for fewer of them. + components: SmallVec<[Component; 32]>, + last_compound_start: Option, } impl Push> for SelectorBuilder { @@ -71,101 +57,115 @@ impl SelectorBuilder { /// Pushes a simple selector onto the current compound selector. #[inline(always)] pub fn push_simple_selector(&mut self, ss: Component) { - assert!(!ss.is_combinator()); - self.simple_selectors.push(ss); - self.current_len += 1; + debug_assert!(!ss.is_combinator()); + self.components.push(ss); } - /// Completes the current compound selector and starts a new one, delimited - /// by the given combinator. + /// Completes the current compound selector and starts a new one, delimited by the given + /// combinator. #[inline(always)] pub fn push_combinator(&mut self, c: Combinator) { - self.combinators.push((c, self.current_len)); - self.current_len = 0; + self.reverse_last_compound(); + self.components.push(Component::Combinator(c)); + self.last_compound_start = Some(self.components.len()); + } + + fn reverse_last_compound(&mut self) { + let start = self.last_compound_start.unwrap_or(0); + self.components[start..].reverse(); } /// Returns true if combinators have ever been pushed to this builder. #[inline(always)] pub fn has_combinators(&self) -> bool { - !self.combinators.is_empty() + self.last_compound_start.is_some() } /// Consumes the builder, producing a Selector. #[inline(always)] - pub fn build(&mut self) -> ThinArc> { + pub fn build(&mut self, parse_relative: ParseRelative) -> ThinArc> { // Compute the specificity and flags. - let sf = specificity_and_flags(self.simple_selectors.iter()); - self.build_with_specificity_and_flags(sf) + let sf = specificity_and_flags(self.components.iter()); + self.build_with_specificity_and_flags(sf, parse_relative) } - /// Builds with an explicit SpecificityAndFlags. This is separated from build() so - /// that unit tests can pass an explicit specificity. + /// Builds with an explicit SpecificityAndFlags. This is separated from build() so that unit + /// tests can pass an explicit specificity. #[inline(always)] pub(crate) fn build_with_specificity_and_flags( &mut self, - spec: SpecificityAndFlags, + mut spec: SpecificityAndFlags, + parse_relative: ParseRelative, ) -> ThinArc> { - // Create the Arc using an iterator that drains our buffers. - // Panic-safety: if SelectorBuilderIter is not iterated to the end, some simple selectors - // will safely leak. - let raw_simple_selectors = unsafe { - let simple_selectors_len = self.simple_selectors.len(); - self.simple_selectors.set_len(0); - std::slice::from_raw_parts(self.simple_selectors.as_ptr(), simple_selectors_len) - }; - let (rest, current) = split_from_end(raw_simple_selectors, self.current_len); - let iter = SelectorBuilderIter { - current_simple_selectors: current.iter(), - rest_of_simple_selectors: rest, - combinators: self.combinators.drain(..).rev(), + let implicit_parent = parse_relative.needs_implicit_parent_selector() && + !spec.flags.contains(SelectorFlags::HAS_PARENT); + + let parent_selector_and_combinator; + let implicit_parent = if implicit_parent { + spec.flags.insert(SelectorFlags::HAS_PARENT); + parent_selector_and_combinator = [ + Component::Combinator(Combinator::Descendant), + Component::ParentSelector, + ]; + &parent_selector_and_combinator[..] + } else { + &[] }; - Arc::from_header_and_iter(spec, iter) + // As an optimization, for a selector without combinators, we can just keep the order + // as-is. + if self.last_compound_start.is_none() { + return Arc::from_header_and_iter(spec, ExactChain(self.components.drain(..), implicit_parent.iter().cloned())); + } + + self.reverse_last_compound(); + Arc::from_header_and_iter(spec, ExactChain(self.components.drain(..).rev(), implicit_parent.iter().cloned())) } } -struct SelectorBuilderIter<'a, Impl: SelectorImpl> { - current_simple_selectors: slice::Iter<'a, Component>, - rest_of_simple_selectors: &'a [Component], - combinators: iter::Rev>, + +impl Default for SelectorBuilder { + #[inline(always)] + fn default() -> Self { + SelectorBuilder { + components: SmallVec::new(), + last_compound_start: None, + } + } } -impl<'a, Impl: SelectorImpl> ExactSizeIterator for SelectorBuilderIter<'a, Impl> { +// This is effectively a Chain<>, but Chain isn't an ExactSizeIterator, see +// https://github.com/rust-lang/rust/issues/34433 +struct ExactChain(A, B); + +impl ExactSizeIterator for ExactChain +where + A: ExactSizeIterator, + B: ExactSizeIterator, +{ fn len(&self) -> usize { - self.current_simple_selectors.len() + - self.rest_of_simple_selectors.len() + - self.combinators.len() + self.0.len() + self.1.len() } } -impl<'a, Impl: SelectorImpl> Iterator for SelectorBuilderIter<'a, Impl> { - type Item = Component; +impl Iterator for ExactChain +where + A: ExactSizeIterator, + B: ExactSizeIterator, +{ + type Item = Item; + #[inline(always)] fn next(&mut self) -> Option { - if let Some(simple_selector_ref) = self.current_simple_selectors.next() { - // Move a simple selector out of this slice iterator. - // This is safe because we’ve called SmallVec::set_len(0) above, - // so SmallVec::drop won’t drop this simple selector. - unsafe { Some(ptr::read(simple_selector_ref)) } - } else { - self.combinators.next().map(|(combinator, len)| { - let (rest, current) = split_from_end(self.rest_of_simple_selectors, len); - self.rest_of_simple_selectors = rest; - self.current_simple_selectors = current.iter(); - Component::Combinator(combinator) - }) - } + self.0.next().or_else(|| self.1.next()) } fn size_hint(&self) -> (usize, Option) { - (self.len(), Some(self.len())) + let len = self.len(); + (len, Some(len)) } } -fn split_from_end(s: &[T], at: usize) -> (&[T], &[T]) { - s.split_at(s.len() - at) -} - /// Flags that indicate at which point of parsing a selector are we. #[derive(Clone, Copy, Default, Eq, PartialEq, ToShmem)] pub(crate) struct SelectorFlags(u8); diff --git a/servo/components/selectors/parser.rs b/servo/components/selectors/parser.rs index 379c9e959061..37b2b00ca56f 100644 --- a/servo/components/selectors/parser.rs +++ b/servo/components/selectors/parser.rs @@ -445,6 +445,13 @@ pub enum ParseRelative { No, } +impl ParseRelative { + #[inline] + pub(crate) fn needs_implicit_parent_selector(self) -> bool { + matches!(self, Self::ForNesting) + } +} + impl SelectorList { /// Returns a selector list with a single `&` pub fn ampersand() -> Self { @@ -952,7 +959,7 @@ impl Selector { } } let spec = SpecificityAndFlags { specificity, flags }; - Selector(builder.build_with_specificity_and_flags(spec)) + Selector(builder.build_with_specificity_and_flags(spec, ParseRelative::No)) } #[inline] @@ -980,9 +987,6 @@ impl Selector { } let result = SelectorList::from_iter(orig.iter().map(|s| { - if !s.has_parent_selector() { - return s.clone(); - } s.replace_parent_selector(parent) })); @@ -1050,82 +1054,53 @@ impl Selector { flags: &mut SelectorFlags, flags_to_propagate: SelectorFlags, ) -> Selector { - if !orig.has_parent_selector() { - return orig.clone(); - } let new_selector = orig.replace_parent_selector(parent); *specificity += Specificity::from(new_selector.specificity() - orig.specificity()); flags.insert(new_selector.flags().intersection(flags_to_propagate)); new_selector } - let mut items = if !self.has_parent_selector() { - // Implicit `&` plus descendant combinator. - let iter = self.iter_raw_match_order(); - let len = iter.len() + 2; - specificity += Specificity::from(parent_specificity_and_flags.specificity); - flags.insert( - parent_specificity_and_flags - .flags - .intersection(SelectorFlags::for_nesting()), - ); - let iter = iter - .cloned() - .chain(std::iter::once(Component::Combinator( - Combinator::Descendant, - ))) - .chain(std::iter::once(Component::Is(parent.clone()))); - UniqueArc::from_header_and_iter_with_size(Default::default(), iter, len) - } else { - let iter = self.iter_raw_match_order().map(|component| { - use self::Component::*; - match *component { - LocalName(..) | - ID(..) | - Class(..) | - AttributeInNoNamespaceExists { .. } | - AttributeInNoNamespace { .. } | - AttributeOther(..) | - ExplicitUniversalType | - ExplicitAnyNamespace | - ExplicitNoNamespace | - DefaultNamespace(..) | - Namespace(..) | - Root | - Empty | - Scope | - Nth(..) | - NonTSPseudoClass(..) | - PseudoElement(..) | - Combinator(..) | - Host(None) | - Part(..) | - Invalid(..) | - RelativeSelectorAnchor => component.clone(), - ParentSelector => { - specificity += Specificity::from(parent_specificity_and_flags.specificity); - flags.insert( - parent_specificity_and_flags - .flags - .intersection(SelectorFlags::for_nesting()), - ); - Is(parent.clone()) - }, - Negation(ref selectors) => { - Negation( - replace_parent_on_selector_list( - selectors.slice(), - parent, - &mut specificity, - &mut flags, - /* propagate_specificity = */ true, - SelectorFlags::for_nesting(), - ) - .unwrap_or_else(|| selectors.clone()), - ) - }, - Is(ref selectors) => { - Is(replace_parent_on_selector_list( + if !self.has_parent_selector() { + return self.clone(); + } + + let iter = self.iter_raw_match_order().map(|component| { + use self::Component::*; + match *component { + LocalName(..) | + ID(..) | + Class(..) | + AttributeInNoNamespaceExists { .. } | + AttributeInNoNamespace { .. } | + AttributeOther(..) | + ExplicitUniversalType | + ExplicitAnyNamespace | + ExplicitNoNamespace | + DefaultNamespace(..) | + Namespace(..) | + Root | + Empty | + Scope | + Nth(..) | + NonTSPseudoClass(..) | + PseudoElement(..) | + Combinator(..) | + Host(None) | + Part(..) | + Invalid(..) | + RelativeSelectorAnchor => component.clone(), + ParentSelector => { + specificity += Specificity::from(parent_specificity_and_flags.specificity); + flags.insert( + parent_specificity_and_flags + .flags + .intersection(SelectorFlags::for_nesting()), + ); + Is(parent.clone()) + }, + Negation(ref selectors) => { + Negation( + replace_parent_on_selector_list( selectors.slice(), parent, &mut specificity, @@ -1133,64 +1108,75 @@ impl Selector { /* propagate_specificity = */ true, SelectorFlags::for_nesting(), ) - .unwrap_or_else(|| selectors.clone())) - }, - Where(ref selectors) => { - Where( - replace_parent_on_selector_list( - selectors.slice(), - parent, - &mut specificity, - &mut flags, - /* propagate_specificity = */ false, - SelectorFlags::for_nesting(), - ) - .unwrap_or_else(|| selectors.clone()), - ) - }, - Has(ref selectors) => Has(replace_parent_on_relative_selector_list( - selectors, + .unwrap_or_else(|| selectors.clone()), + ) + }, + Is(ref selectors) => { + Is(replace_parent_on_selector_list( + selectors.slice(), parent, &mut specificity, &mut flags, + /* propagate_specificity = */ true, SelectorFlags::for_nesting(), ) - .into_boxed_slice()), - - Host(Some(ref selector)) => Host(Some(replace_parent_on_selector( - selector, - parent, - &mut specificity, - &mut flags, - SelectorFlags::for_nesting() - SelectorFlags::HAS_NON_FEATURELESS_COMPONENT, - ))), - NthOf(ref data) => { - let selectors = replace_parent_on_selector_list( - data.selectors(), + .unwrap_or_else(|| selectors.clone())) + }, + Where(ref selectors) => { + Where( + replace_parent_on_selector_list( + selectors.slice(), parent, &mut specificity, &mut flags, - /* propagate_specificity = */ true, + /* propagate_specificity = */ false, SelectorFlags::for_nesting(), - ); - NthOf(match selectors { - Some(s) => { - NthOfSelectorData::new(data.nth_data(), s.slice().iter().cloned()) - }, - None => data.clone(), - }) - }, - Slotted(ref selector) => Slotted(replace_parent_on_selector( - selector, + ) + .unwrap_or_else(|| selectors.clone()), + ) + }, + Has(ref selectors) => Has(replace_parent_on_relative_selector_list( + selectors, + parent, + &mut specificity, + &mut flags, + SelectorFlags::for_nesting(), + ) + .into_boxed_slice()), + + Host(Some(ref selector)) => Host(Some(replace_parent_on_selector( + selector, + parent, + &mut specificity, + &mut flags, + SelectorFlags::for_nesting() - SelectorFlags::HAS_NON_FEATURELESS_COMPONENT, + ))), + NthOf(ref data) => { + let selectors = replace_parent_on_selector_list( + data.selectors(), parent, &mut specificity, &mut flags, + /* propagate_specificity = */ true, SelectorFlags::for_nesting(), - )), - } - }); - UniqueArc::from_header_and_iter(Default::default(), iter) - }; + ); + NthOf(match selectors { + Some(s) => { + NthOfSelectorData::new(data.nth_data(), s.slice().iter().cloned()) + }, + None => data.clone(), + }) + }, + Slotted(ref selector) => Slotted(replace_parent_on_selector( + selector, + parent, + &mut specificity, + &mut flags, + SelectorFlags::for_nesting(), + )), + } + }); + let mut items = UniqueArc::from_header_and_iter(Default::default(), iter); *items.header_mut() = SpecificityAndFlags { specificity: specificity.into(), flags, @@ -2668,7 +2654,7 @@ where builder.push_combinator(combinator); } - return Ok(Selector(builder.build())); + return Ok(Selector(builder.build(parse_relative))); } fn try_parse_combinator<'i, 't, P, Impl>(input: &mut CssParser<'i, 't>) -> Result { @@ -4400,7 +4386,7 @@ pub mod tests { parse("#foo:has(:is(.bar, div .baz).bar)").unwrap() ); - let child = parse("#foo").unwrap(); + let child = parse_relative_expected("#foo", ParseRelative::ForNesting, Some("& #foo")).unwrap(); assert_eq!( child.replace_parent_selector(&parent), parse(":is(.bar, div .baz) #foo").unwrap() diff --git a/testing/web-platform/meta/css/css-nesting/conditional-rules.html.ini b/testing/web-platform/meta/css/css-nesting/conditional-rules.html.ini index 1275f8492c5f..0b05529f105e 100644 --- a/testing/web-platform/meta/css/css-nesting/conditional-rules.html.ini +++ b/testing/web-platform/meta/css/css-nesting/conditional-rules.html.ini @@ -1,3 +1 @@ prefs: [layout.css.at-scope.enabled:true] -[conditional-rules.html] - expected: FAIL diff --git a/testing/web-platform/meta/css/css-nesting/parsing.html.ini b/testing/web-platform/meta/css/css-nesting/parsing.html.ini deleted file mode 100644 index 6944c7601d29..000000000000 --- a/testing/web-platform/meta/css/css-nesting/parsing.html.ini +++ /dev/null @@ -1,15 +0,0 @@ -[parsing.html] - [.foo { + .bar, .foo, > .baz { color: green; }}] - expected: FAIL - - [.foo { .foo { color: green; }}] - expected: FAIL - - [.foo { .foo, .foo & { color: green; }}] - expected: FAIL - - [.foo { :is(.bar, .baz) { color: green; }}] - expected: FAIL - - [.foo { .foo, .bar { color: green; }}] - expected: FAIL