Upgrade rust-selectors, pass ':empty' tests

[frewsxcv]

servo/rust-selectors#36

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5531

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[frewsxcv]

This unimplemented!() is intentional because I'm not sure how this should be implemented for LayoutNode. If anyone has suggestions, please let me know

[SimonSapin]

Based on reading ThreadSafeLayoutNode::text_content, try something like:

if let Some(text) = TextCast::to_layout_js(&self.node) {
    unsafe {
        !CharacterDataCast::from_layout_js(&text).data_for_layout().is_empty()
    }
} else {
    ElementCast::to_layout_js(&self.node).is_some()
}

[frewsxcv]

Added in the latest force push

[frewsxcv]

@SimonSapin r?

[frewsxcv]

I forced pushed because I ran the entire suite and found I passed two WPT tests (instead of just one)

[Ms2ger]

These two implementations aren't equivalent.

[frewsxcv]

These two implementations aren't equivalent.

In what ways are they different? At least for now, I'm very unfamiliar with any of the layout code (LayoutNode, LayoutJS, etc) so it's not entirely obvious to me how they differ.

[Ms2ger]

<style>
#x { background: red; }
#x:empty { background: green }
</style>
<p id=x><!--foo--></p>
<script>
console.log(document.getElementById("x").matches(":empty"));
</script>

[SimonSapin]

I don’t see the difference either. As far as I can tell both impls would return false for comment nodes.

[Ms2ger]

textContent on comments gives their data.

[SimonSapin]

Oh, I see. @frewsxcv, I think you want self.is_text() in there, similar to self.is_element().

[frewsxcv]

Ah, I understand, thanks for the clarification. Let me know how that latest force push looks.

[Ms2ger]

r+ if you add that test

[frewsxcv]

Sounds reasonable. I'll add

[frewsxcv]

I'd like to add a regression test for this, but I think it is blocked by #6663 since it affects the WPT test that I would be modifying

[pcwalton]

@Ms2ger Can I help get this moving? The r+ here is blocked on a WPT upgrade which is blocked on some other bugs. Serialization of 2D canvas/WebGL messages is blocked on this, and e10s is blocked on that.

[frewsxcv]

Considering this test:

• https://github.com/w3c/web-platform-tests/blob/e0fd0c9f36f44450d848c3e42f49d838e0e09a60/dom/nodes/selectors.js#L209
• https://github.com/w3c/web-platform-tests/blob/e0fd0c9f36f44450d848c3e42f49d838e0e09a60/dom/nodes/ParentNode-querySelector-All-content.html#L221

I don't think I need to add a new test case for this since it already exists (but currently doesn't work).

Since I haven't explicitly stated, I am not actively working on #6663.

[Ms2ger]

Just add a new test under tests/wpt/mozilla, then, and make sure to test both css and matches().

[frewsxcv]

Added a couple tests, let me know how they look

[Ms2ger]

Thanks! One nit, though: in reftests, any red being visible idiomatically implies failure, so use blue or another colour?

[frewsxcv]

Changed the colors to orange and purple

[Ms2ger]

@bors-servo r+

[bors-servo]

📌 Commit b1b03a3 has been approved by Ms2ger

[bors-servo]

⌛ Testing commit b1b03a3 with merge 488f3b6...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3