Implement Document#elementsFromPoint

[rilut]

Fixes #9859.

I'm trying to implement Document#elementsFromPoint, which I need to reuse the get_nodes_under_mouse and mouse_over function which have been removed a days ago in #9715. So I added it back while I'm not sure if my implementation is correct. Any advice will be greatly appreciated.

---

This change is

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!
• These commits modify layout and script code, but no tests are modified. Please consider adding a test!

[emilio]

Good job! I've left a few comments :)

Let's see if there's any test for that,

@bors-servo: try

-S-awaiting-review +S-needs-code-changes

---

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

---

components/layout/query.rs, line 101 [r1] (raw file):
This conditional can be removed, since using .iter().map(..)will return an empty vector in this case regardless.

---

components/script/dom/document.rs, line 1498 [r1] (raw file):
I think this name is misleading, and should be called nodes_from_point, or return just the elements.

---

components/script/dom/document.rs, line 2610 [r1] (raw file):
Doing something like:

self.elements_from_point().iter().flat_map(|addr| {
    let node = node::from_untrusted_node_address(js_runtime, addr);
    node.downcast::<Element>()
}).collect();

Should be enough, the parent of a node that is not an element should already be in the list after it.

Your code might unexpectedly panic depending on which point are you using as input.

---

components/script/dom/document.rs, line 2624 [r1] (raw file):
Why do we need the clone and the unwrap here?

Can't we do:

if elements.last() != Some(root_element) {
    elements.push(root_element);
}

Or Some(&root_element) since last() returns a reference.

---

Comments from the review on Reviewable.io

[bors-servo]

⌛ Trying commit 0e639cc with merge d77aa2d...

[bors-servo]

💔 Test failed - status-appveyor

[jdm]

Looks like the non-windows platforms had no unexpected test results.

[jdm]

We are apparently ignoring the tests in tests/wpt/web-platform-tests/cssom-view/. Could you add that directory to tests/wpt/include.ini?

[rilut]

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

---

components/script/dom/document.rs, line 1498 [r1] (raw file):
But nodes_from_point is already used. Should we rename the other function? To what name?

---

Comments from the review on Reviewable.io

[emilio]

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

components/script/dom/document.rs, line 1498 [r1] (raw file):
Is it? I don't see any nodes_from_point in the impl Document... There shouldn't be any name conflict.

---

Comments from the review on Reviewable.io

[rilut]

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

components/script/dom/document.rs, line 1498 [r1] (raw file):
Oh right, there wouldn't be any name conflict

---

Comments from the review on Reviewable.io

[emilio]

Left a few more comments, let's see how the cssom tests go!

@bors-servo: try

-S-awaiting-review +S-needs-code-changes

---

Reviewed 2 of 4 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/document.rs, line 2612 [r2] (raw file):
I think this is still not quite correct, you're asuming every node address returned here is an element. I think that might not be always true, that's why I suggested the flat_map (which "removes" the None values) instead of the unwrap().

I don't know if we include text nodes in the nodes_from_point traversal, but that'd be just one of the possible examples that could cause a crash.

---

tests/wpt/include.ini, line 58 [r2] (raw file):
Thanks for doing this! I don't think we pass every test in here so the expectations should be updated... I'll make a try run now :)

---

Comments from the review on Reviewable.io

[bors-servo]

⌛ Trying commit 2c8b247 with merge a53c40a...

[bors-servo]

💔 Test failed - mac-rel-wpt

[emilio]

CRASH [expected OK] /cssom-view/elementsFromPoint.html

Heh, that's it, we can't assume every node returned by nodes_from_point is an element.

[rilut]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/script/dom/document.rs, line 2612 [r2] (raw file):
Right, but I couldn't use flat_map because the trait core::iter::Iterator is not implemented for the type dom::bindings::js::Root<dom::element::Element>

---

Comments from the review on Reviewable.io

[rilut]

components/script/dom/document.rs, line 2612 [r2] (raw file):
Never mind. Suppose

let mut elements: Vec<_> = self.nodes_from_point(point).iter()
    .flat_map(|&untrusted_node_address| {
        let node = node::from_untrusted_node_address(js_runtime, untrusted_node_address);
        let element=node.downcast::<Element>();
        element
}).collect();

How do I make node live long enough? clone?

---

Comments from the review on Reviewable.io

[jdm]

I think the problem is the unanswered question.

[rilut]

@nox thanks, not really. It's what @jdm said

[KiChjang]

@bors-servo r=emilio

[bors-servo]

📌 Commit 07584b9 has been approved by emilio

[bors-servo]

⌛ Testing commit 07584b9 with merge d7f4327...

[bors-servo]

💔 Test failed - status-appveyor

[KiChjang]

@bors-servo retry

• #10251

[bors-servo]

⌛ Testing commit 07584b9 with merge c363817...

[Ms2ger]

@bors-servo retry

[bors-servo]

⌛ Testing commit 07584b9 with merge 241518a...

[bors-servo]

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

[rilut]

Thanks, Servo team 🎉.

I'll proceed with #.10093