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
Trigger reflow on document.elementsFromPoint #15768
Trigger reflow on document.elementsFromPoint #15768
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon. |
Heads up! This PR modifies the following files:
|
89fa95c
to
3e146b3
Compare
3e146b3
to
7426d90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
r=emilio with those comments addressed, thanks for fixing that!
<title></title> | ||
<link rel="match" href="document_elementsFromPoint_ref.html"> | ||
<script> | ||
var elements = document.elementsFromPoint(10, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of writing the test like this, since this is testing a crash, let's do something in the lines of:
<!doctype html>
<meta charset="utf-8">
<title>document.elementsFromPoint does not crash due to a missing display list</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
test(function() {
document.elementsFromPoint(10, 10);
}, "doesn't crash");
</script>
components/layout/query.rs
Outdated
.rev() | ||
.map(|metadata| metadata.node.to_untrusted_node_address()) | ||
.collect() | ||
fn nodes_from_point(&self) -> Vec<UntrustedNodeAddress> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this nodes_from_point_response
, so it's clear it's not intended to be called directly without querying first.
@@ -0,0 +1,3 @@ | |||
<!doctype html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just for reference, there's a blank.html
file used to avoid having a lot of files like this, though this is no longer relevant with my previous comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, good to know. Thank you!
…test to regular test checking only the fixed crash
@bors-servo r=emilio |
@ferjm: 🔑 Insufficient privileges: Not in reviewers |
@emilio I already addressed your feedback, but I don't seem have permissions to ask bors-servo to merge. |
@bors-servo r=emilio |
📌 Commit 31d833f has been approved by |
…r=emilio Trigger reflow on document.elementsFromPoint As [suggested](#15592 (comment)) by @jdm `Document::nodes_from_point` now triggers a reflow. I added a new reftest that panics with `ERROR:servo: Tried to hit test without a DisplayList` if this patch is not applied. - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15592. - [X] There are tests for these changes <!-- 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/15768) <!-- Reviewable:end -->
☀️ 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 |
Thanks for fixing this again! :) |
As suggested by @jdm
Document::nodes_from_point
now triggers a reflow.I added a new reftest that panics with
ERROR:servo: Tried to hit test without a DisplayList
if this patch is not applied../mach build -d
does not report any errors./mach test-tidy
does not report any errorsdocument.elementsFromPoint
is causing a black screen. #15592.This change is