Bug 1340683 - stylo: Implement the :-moz-any pseudo-class #15966

mbrubeck · 2017-03-15T19:24:40Z

Adds support for the non-standard :-moz-any selector.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix https://bugzilla.mozilla.org/show_bug.cgi?id=1340683
These changes do not require tests because they are gecko-only

This change is

highfive · 2017-03-15T19:24:43Z

Heads up! This PR modifies the following files:

@bholley: components/style/gecko/wrapper.rs, components/style/gecko/selector_parser.rs
@emilio: components/style/gecko/wrapper.rs, components/style/gecko/selector_parser.rs

highfive · 2017-03-15T19:24:44Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

mbrubeck · 2017-03-15T19:25:16Z

r? @SimonSapin

mbrubeck · 2017-03-15T19:38:48Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04585b67f0646e335d87e48584f4abe7fd6d9a64

mbrubeck · 2017-03-15T19:47:15Z

This still fails the layout/style/test/test_any_dynamic.html mochitest; it doesn't handle dynamic updates correctly. I need to update affects_siblings and other selector methods to take :-moz-any into account.

emilio · 2017-03-15T19:51:32Z

components/selectors/parser.rs

@@ -120,7 +120,7 @@ pub trait Parser {
    }
 }

-#[derive(PartialEq, Clone, Debug)]
+#[derive(PartialEq, Eq, Hash, Clone, Debug)]


Is this change needed just due to the bounds in SelectorImpls definition?

If so, I believe we can remove them since we switched to conditional compilation in style and should have a concrete type now.

Also, in pseudo-classes it doesn't really matter I think, since we only store PseudoElements in HashMaps.

No, it's also required by #[derive(Clone, Eq, Hash, PartialEq)]
for struct ComplexSelector.

emilio · 2017-03-15T19:51:32Z

components/style/gecko/wrapper.rs

@@ -661,7 +661,8 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
            NonTSPseudoClass::MozTableBorderNonzero |
            NonTSPseudoClass::MozBrowserFrame => unsafe {
                Gecko_MatchesElement(pseudo_class.to_gecko_pseudoclasstype().unwrap(), self.0)
-            }
+            },
+            NonTSPseudoClass::MozAny(ref selectors) => matches(&selectors.0, self, None),


This doesn't pass the ElementFlags, nor the StyleRelations here. I believe it should be ok, because we unconditionally insert the appropriate flag for all the custom non-tree-structural pseudo-classes, and because of the limit on only-descendant combinators.

Can you verify that something like http://searchfox.org/mozilla-central/source/layout/reftests/css-selectors/state-dependent-in-any.html (perhaps tweaking manually with :hover or :active, or something we actually implement instead of :valid) works?

Can you verify that something like http://searchfox.org/mozilla-central/source/layout/reftests/css-selectors/state-dependent-in-any.html (perhaps tweaking manually with :hover or :active, or something we actually implement instead of :valid) works?

Selectors like :-moz-any(:hover) do not seem to update correctly on state changes; I'm not certain why, since as you said AFFECTED_BY_STATE is always set.

Also, things like :-moz-any(:first-child) are valid and should set HAS_EDGE_CHILD_SELECTOR and AFFECTED_BY_CHILD_INDEX, so I think we do need to pass the relations and flags here.

Ah, part of the problem is that pseudo_class_state_flag was not implemented correctly for MozAny.

emilio · 2017-03-15T19:57:13Z

Oh, I had already started reviewing before you commented :)

Upon reflection, I believe the dynamic changes will not pass until https://bugzilla.mozilla.org/show_bug.cgi?id=1338982 is fixed (I'm going to fix it in https://bugzilla.mozilla.org/show_bug.cgi?id=1345950, but I wanted #15890 to land first)

bzbarsky · 2017-03-15T20:11:07Z

components/style/gecko/selector_parser.rs

+        match_ignore_ascii_case! { &name,
+            "-moz-any" => {
+                // https://developer.mozilla.org/en-US/docs/Web/CSS/:any
+                // The selectors may not contain pseudo-elements and the only combinator allowed is


The devmo docs don't seem to match the code. The code has:

// Check that none of the selectors in the list have combinators or // pseudo-elements.

and proceeds to do just that. See http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/style/nsCSSParser.cpp#6511-6518

Fixed to match the Gecko code.

bzbarsky · 2017-03-15T20:18:31Z

Note that we also have tests for this at http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/style/test/test_selectors.html#1184-1209 and the second test there tests that descendant combinators don't parse...

But it looks like lack of support for other things earlier in the test (e.g. :placeholder-shown) causes it to throw exceptions that prevent it from even reaching this part. :(

Maybe we should change should_serialize_to to more gracefully handle cases when the selector is not supported at all. :( @upsuper

mbrubeck · 2017-03-15T20:57:26Z

I need to update affects_siblings and other selector methods to take :-moz-any into account.

Done. I think this is ready for re-review now.

mbrubeck · 2017-03-15T20:58:35Z

Oops, I didn't see all of the review comments because I hadn't refreshed the page. Addressing some more..

mbrubeck · 2017-03-15T23:16:43Z

Updated to correctly calculate ElementStyleFlags, ElementState, and StyleRelations for selectors like :-moz-any(:hover).

mbrubeck · 2017-03-15T23:18:03Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfef2302480ed879637ea957ba3e047631d4f5f6

bors-servo · 2017-03-16T01:13:00Z

☔ The latest upstream changes (presumably #15971) made this pull request unmergeable. Please resolve the merge conflicts.

mbrubeck · 2017-03-16T14:56:33Z

Rebased.

emilio

Looks good. We've talked over IRC and we'll make sure there are bugs on file for the remaining failures in layout/style/test/test_any_dynamic.html, and layout/style/test/test_bug635286.html.

I'm pretty sure they'll be fixed by:

But better getting them on file.

emilio · 2017-03-16T18:11:23Z

components/style/gecko/selector_parser.rs

@@ -207,6 +245,10 @@ impl NonTSPseudoClass {
                match *self {
                    $(NonTSPseudoClass::$name => flag!($state),)*
                    $(NonTSPseudoClass::$s_name(..) => flag!($s_state),)*
+                    NonTSPseudoClass::MozAny(ref selectors) => {
+                        selectors.iter().fold(ElementState::empty(), |state, s|


nit: since the closure wraps to the next line, let's just wrap it with braces.

emilio · 2017-03-16T18:11:23Z

components/style/gecko/selector_parser.rs

@@ -293,6 +336,11 @@ impl<'a> ::selectors::Parser for SelectorParser<'a> {
                        let name = String::from(parser.expect_ident_or_string()?).into_boxed_str();
                        NonTSPseudoClass::$s_name(name)
                    }, )*
+                    "-moz-any" => {
+                        let selectors = parser.parse_comma_separated(|input|
+                            ComplexSelector::parse(self, input))?;


emilio · 2017-03-16T18:11:23Z

components/style/restyle_hints.rs

+/// Returns the union of any `ElementState` flags for components of a `ComplexSelector`
+pub fn complex_selector_to_state(sel: &ComplexSelector<SelectorImpl>) -> ElementState {
+    sel.compound_selector.iter().fold(ElementState::empty(), |state, s|
+        state | selector_to_state(s)


emilio · 2017-03-16T18:11:31Z

@bors-servo r+

bors-servo · 2017-03-16T18:11:32Z

📌 Commit 097222a has been approved by emilio

emilio · 2017-03-16T18:11:47Z

Whoops.

@bors-servo r-

Please address the review nits, then r=me

mbrubeck · 2017-03-16T18:38:59Z

@bors-servo r=emilio

bors-servo · 2017-03-16T18:39:00Z

📌 Commit 097222a has been approved by emilio


        Bug 1340683 - stylo: Implement the :-moz-any pseudo-class

mbrubeck · 2017-03-16T18:45:13Z

@bors-servo r=emilio

bors-servo · 2017-03-16T18:45:14Z

📌 Commit 2872c8b has been approved by emilio

bors-servo · 2017-03-16T23:19:14Z

⌛ Testing commit 2872c8b with merge 0a747e2...


        Auto merge of #15966 - mbrubeck:any, r=emilio

Bug 1340683 - stylo: Implement the :-moz-any pseudo-class Adds support for the non-standard [:-moz-any](https://developer.mozilla.org/en-US/docs/Web/CSS/:any) selector. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix https://bugzilla.mozilla.org/show_bug.cgi?id=1340683 - [x] These changes do not require tests because they are gecko-only  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15966)

bors-servo · 2017-03-16T23:55:47Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: emilio
Pushing 0a747e2 to master...

highfive assigned emilio Mar 15, 2017

highfive added the S-awaiting-review label Mar 15, 2017

highfive assigned SimonSapin and unassigned emilio Mar 15, 2017

mbrubeck changed the title ~~Bug 1340683 - stylo: Implement the :-moz-any pseudo-class~~ WIP: Bug 1340683 - stylo: Implement the :-moz-any pseudo-class Mar 15, 2017

mbrubeck added the S-needs-code-changes label Mar 15, 2017

emilio reviewed Mar 15, 2017

View changes

bzbarsky suggested changes Mar 15, 2017

View changes

mbrubeck force-pushed the mbrubeck:any branch from 435611e to 3058019 Mar 15, 2017

highfive removed the S-needs-code-changes label Mar 15, 2017

mbrubeck force-pushed the mbrubeck:any branch 2 times, most recently from b72298c to 10ad449 Mar 15, 2017

highfive added the S-needs-rebase label Mar 16, 2017

mbrubeck force-pushed the mbrubeck:any branch from 10ad449 to 097222a Mar 16, 2017

emilio assigned emilio and unassigned SimonSapin Mar 16, 2017

mbrubeck changed the title ~~WIP: Bug 1340683 - stylo: Implement the :-moz-any pseudo-class~~ Bug 1340683 - stylo: Implement the :-moz-any pseudo-class Mar 16, 2017

emilio approved these changes Mar 16, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Mar 16, 2017

Bug 1340683 - stylo: Implement the :-moz-any pseudo-class

Loading status checks…

2872c8b

mbrubeck force-pushed the mbrubeck:any branch from 097222a to 2872c8b Mar 16, 2017

highfive added S-awaiting-review and removed S-awaiting-merge labels Mar 16, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 16, 2017

bors-servo merged commit 2872c8b into servo:master Mar 16, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 16, 2017

servo / servo

Join GitHub today

Bug 1340683 - stylo: Implement the :-moz-any pseudo-class #15966

Bug 1340683 - stylo: Implement the :-moz-any pseudo-class #15966

Conversation

mbrubeck commented Mar 15, 2017 • edited by larsbergstrom

highfive commented Mar 15, 2017

highfive commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

This comment has been minimized.

This comment has been minimized.

mbrubeck Mar 15, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Mar 15, 2017

This comment has been minimized.

This comment has been minimized.

bzbarsky commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

mbrubeck commented Mar 15, 2017

bors-servo commented Mar 16, 2017

mbrubeck commented Mar 16, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Mar 16, 2017

bors-servo commented Mar 16, 2017

emilio commented Mar 16, 2017

mbrubeck commented Mar 16, 2017

bors-servo commented Mar 16, 2017

mbrubeck commented Mar 16, 2017

bors-servo commented Mar 16, 2017

bors-servo commented Mar 16, 2017

bors-servo commented Mar 16, 2017

mbrubeck commented Mar 15, 2017 •

edited by larsbergstrom

mbrubeck Mar 15, 2017 •

edited