Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

selectors: Add parsing support for ::slotted(). #19541

Merged
merged 1 commit into from Dec 14, 2017

Conversation

@emilio
Copy link
Member

emilio commented Dec 10, 2017

Without turning it on yet, of course.

The reason why I didn't use the general PseudoElement mechanism is because this
pseudo is a bit of its own thing, and I found easier to make ::selectors know
about it (because you need to jump to the assigned slot) than the other way
around.

Also, we need to support ::slotted(..)::before and such, and supporting multiple
pseudo-elements like that breaks some other invariants around the SelectorMap,
and fixing those would require special-casing slotted a lot more in other parts
of the code.

Let me know if you think otherwise.

I also don't like much the boolean tuple return value, but I plan to do some
cleanup in the area in a bit, so it should go away soon, I'd hope.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 10, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/selectors/tree.rs, components/selectors/matching.rs, components/selectors/parser.rs, components/selectors/builder.rs
@emilio

This comment has been minimized.

Copy link
Member Author

emilio commented Dec 10, 2017

@highfive highfive assigned SimonSapin and unassigned Manishearth Dec 10, 2017
@emilio emilio force-pushed the emilio:parse-slotted branch from 3e4af9e to 413517c Dec 12, 2017
@emilio

This comment has been minimized.

Copy link
Member Author

emilio commented Dec 14, 2017

r? @heycam

  • @SimonSapin and I talked today about it and he's fine with the approach, but mentioned that he didn't know enough about Shadow DOM to review it.
@highfive highfive assigned heycam and unassigned SimonSapin Dec 14, 2017
@heycam
heycam approved these changes Dec 14, 2017
Copy link
Member

heycam left a comment

Looks good.

@@ -541,6 +545,21 @@ where
{
match *selector {
Component::Combinator(_) => unreachable!(),
Component::Slotted(ref selectors) => {
context.shared.nesting_level += 1;

This comment has been minimized.

Copy link
@heycam

heycam Dec 14, 2017

Member

Minor thing, but I wonder if we should have an RAII-like thing to handle the incrementing/decrementing.

This comment has been minimized.

Copy link
@emilio

emilio Dec 14, 2017

Author Member

Yeah, I thought about it to, it's slightly tricky, because it requires grabbing a mutable reference to the context, which is not possible, so we should switch the count to be a Cell<usize> instead... Not that it matters a lot, tbh.

assert!(parse("::slotted(div + bar)").is_err());
assert!(parse("::slotted(div) + foo").is_err());
assert!(parse("div ::slotted(div)").is_ok());
assert!(parse("div + slot::slotted(div)").is_ok());

This comment has been minimized.

Copy link
@heycam

heycam Dec 14, 2017

Member

Can you include some tests with a compound selector with more than one simple selector (like div.foo) and also with a list of compound selectors?

This comment has been minimized.

Copy link
@emilio

emilio Dec 14, 2017

Author Member

Sure :)

Without turning it on yet, of course.

The reason why I didn't use the general PseudoElement mechanism is because this
pseudo is a bit of its own thing, and I found easier to make ::selectors know
about it (because you need to jump to the assigned slot) than the other way
around.

Also, we need to support ::slotted(..)::before and such, and supporting multiple
pseudo-elements like that breaks some other invariants around the SelectorMap,
and fixing those would require special-casing slotted a lot more in other parts
of the code.

Let me know if you think otherwise.

I also don't like much the boolean tuple return value, but I plan to do some
cleanup in the area in a bit, so it should go away soon, I'd hope.
@emilio emilio force-pushed the emilio:parse-slotted branch from 413517c to 7886e03 Dec 14, 2017
@emilio

This comment has been minimized.

Copy link
Member Author

emilio commented Dec 14, 2017

@bors-servo r=heycam

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2017

📌 Commit 7886e03 has been approved by heycam

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2017

⌛️ Testing commit 7886e03 with merge f5129ef...

bors-servo added a commit that referenced this pull request Dec 14, 2017
selectors: Add parsing support for ::slotted().

Without turning it on yet, of course.

The reason why I didn't use the general PseudoElement mechanism is because this
pseudo is a bit of its own thing, and I found easier to make ::selectors know
about it (because you need to jump to the assigned slot) than the other way
around.

Also, we need to support ::slotted(..)::before and such, and supporting multiple
pseudo-elements like that breaks some other invariants around the SelectorMap,
and fixing those would require special-casing slotted a lot more in other parts
of the code.

Let me know if you think otherwise.

I also don't like much the boolean tuple return value, but I plan to do some
cleanup in the area in a bit, so it should go away soon, I'd hope.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19541)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2017

@bors-servo bors-servo merged commit 7886e03 into servo:master Dec 14, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.