From b50a39c2d38e8f9a3815545a0e0676f37c2e0bc7 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] Bug 1639533 - Fix a no-longer valid assumption in pseudo-element matching / invalidation code. r=heycam 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 --- layout/style/crashtests/1639533.html | 15 +++++++ layout/style/crashtests/crashtests.list | 1 + .../style/test/test_pseudoelement_state.html | 18 ++++++++ layout/style/test/test_selectors.html | 7 ++++ servo/components/selectors/matching.rs | 15 ++++--- servo/components/selectors/parser.rs | 41 +++++++++++++++++++ .../style/invalidation/element/invalidator.rs | 28 ++++++++----- .../css/css-scoping/slotted-parsing.html | 4 ++ 8 files changed, 110 insertions(+), 19 deletions(-) create mode 100644 layout/style/crashtests/1639533.html diff --git a/layout/style/crashtests/1639533.html b/layout/style/crashtests/1639533.html new file mode 100644 index 000000000000..5bf2118ffe03 --- /dev/null +++ b/layout/style/crashtests/1639533.html @@ -0,0 +1,15 @@ + + +
+ +
+ diff --git a/layout/style/crashtests/crashtests.list b/layout/style/crashtests/crashtests.list index 6d3c0be1b856..97aba29a14df 100644 --- a/layout/style/crashtests/crashtests.list +++ b/layout/style/crashtests/crashtests.list @@ -319,3 +319,4 @@ load 1599286.html pref(layout.css.motion-path.enabled,true) load 1609786.html pref(layout.css.constructable-stylesheets.enabled,true) load 1616433.html pref(layout.css.constructable-stylesheets.enabled,true) load 1616407.html +load 1639533.html diff --git a/layout/style/test/test_pseudoelement_state.html b/layout/style/test/test_pseudoelement_state.html index d713d1eba072..d2a060c1f3dc 100644 --- a/layout/style/test/test_pseudoelement_state.html +++ b/layout/style/test/test_pseudoelement_state.html @@ -75,6 +75,24 @@ var gTests = [ active_test_style: 'body input::-moz-range-thumb:active { background: lime; }', active_reference_style: 'body input::-moz-range-thumb { background: lime; }' }, + // Do the same as above, but using :is instead. + { markup: '', + pseudoelement: '::-moz-range-thumb', + common_style: 'body input { -moz-appearance: none; } input::-moz-range-thumb { background: black; }', + hover_test_style: 'body input::-moz-range-thumb:is(:hover) { background: green; }', + hover_reference_style: 'body input::-moz-range-thumb { background: green; }', + active_test_style: 'body input::-moz-range-thumb:is(:active) { background: lime; }', + active_reference_style: 'body input::-moz-range-thumb { background: lime; }' }, + + // Do the same as above, but using :not instead. + { markup: '', + pseudoelement: '::-moz-range-thumb', + common_style: 'input { -moz-appearance: none; } input::-moz-range-thumb { background: green } input::-moz-range-thumb:not(:hover) { background: black; }', + hover_test_style: '', + hover_reference_style: 'input::-moz-range-thumb { background: green !important; }', + active_test_style: 'input::-moz-range-thumb:active { background: lime !important; }', + active_reference_style: 'input::-moz-range-thumb { background: lime !important; }' }, + // ::placeholder can't be tested, since the UA style sheet sets it to // be pointer-events:none. ]; diff --git a/layout/style/test/test_selectors.html b/layout/style/test/test_selectors.html index 5cf4dc99de2c..72486caeaee4 100644 --- a/layout/style/test/test_selectors.html +++ b/layout/style/test/test_selectors.html @@ -1308,7 +1308,14 @@ function runTests() { // CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE (none of which are // unprefixed). test_parseable("::-moz-color-swatch:hover"); + test_parseable("::-moz-color-swatch:is(:hover)"); + test_parseable("::-moz-color-swatch:not(:hover)"); + test_parseable("::-moz-color-swatch:where(:hover)"); test_balanced_unparseable("::-moz-color-swatch:not(.foo)"); + test_balanced_unparseable("::-moz-color-swatch:where(.foo)"); + test_balanced_unparseable("::-moz-color-swatch:is(.foo)"); + test_balanced_unparseable("::-moz-color-swatch:where(p, :hover)"); + test_balanced_unparseable("::-moz-color-swatch:is(p, :hover)"); test_balanced_unparseable("::-moz-color-swatch:first-child"); test_balanced_unparseable("::-moz-color-swatch:host"); test_balanced_unparseable("::-moz-color-swatch:host(div)"); diff --git a/servo/components/selectors/matching.rs b/servo/components/selectors/matching.rs index d8d4ad599465..f55833f7fb64 100644 --- a/servo/components/selectors/matching.rs +++ b/servo/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/servo/components/selectors/parser.rs b/servo/components/selectors/parser.rs index 74f4a48de045..e2ad8e30cafd 100644 --- a/servo/components/selectors/parser.rs +++ b/servo/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/servo/components/style/invalidation/element/invalidator.rs b/servo/components/style/invalidation/element/invalidator.rs index 4f5d005c120a..8b387bc0c04a 100644 --- a/servo/components/style/invalidation/element/invalidator.rs +++ b/servo/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. diff --git a/testing/web-platform/tests/css/css-scoping/slotted-parsing.html b/testing/web-platform/tests/css/css-scoping/slotted-parsing.html index 89de0e5b41a3..f0062c86744f 100644 --- a/testing/web-platform/tests/css/css-scoping/slotted-parsing.html +++ b/testing/web-platform/tests/css/css-scoping/slotted-parsing.html @@ -17,6 +17,10 @@ test_invalid_selector("::slotted(*):host"); test_invalid_selector("::slotted(*):host(div)"); test_invalid_selector("::slotted(*):hover"); + test_invalid_selector("::slotted(*):is(:hover)"); + test_invalid_selector("::slotted(*):where(:hover)"); + test_invalid_selector("::slotted(*):is(#id)"); + test_invalid_selector("::slotted(*):where(#id)"); test_invalid_selector("::slotted(*):read-only"); test_invalid_selector("::slotted(*)::slotted(*)"); test_invalid_selector("::slotted(*)::before::slotted(*)");