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

Trigger reflow on document.elementsFromPoint #15768

Merged

Conversation

@ferjm
Copy link
Member

commented Feb 28, 2017

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 errors
  • These changes fix #15592.
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 28, 2017

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.

@highfive

This comment has been minimized.

Copy link

commented Feb 28, 2017

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/document.rs, components/script/dom/window.rs, components/script_layout_interface/message.rs, components/script_layout_interface/rpc.rs
  • @fitzgen: components/script/dom/document.rs, components/script/dom/window.rs, components/script_layout_interface/message.rs, components/script_layout_interface/rpc.rs
  • @emilio: components/layout/query.rs, components/layout/lib.rs
@highfive

This comment has been minimized.

Copy link

commented Feb 28, 2017

warning Warning warning

  • These commits include an empty title element (<title></title>). Consider adding appropriate metadata.
@ferjm ferjm force-pushed the ferjm:issue-15592-document-elementsFromPoint branch 2 times, most recently from 89fa95c to 3e146b3 Mar 1, 2017
@ferjm ferjm force-pushed the ferjm:issue-15592-document-elementsFromPoint branch from 3e146b3 to 7426d90 Mar 2, 2017
@emilio
emilio approved these changes Mar 2, 2017
Copy link
Member

left a comment

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);

This comment has been minimized.

Copy link
@emilio

emilio Mar 2, 2017

Member

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>
.rev()
.map(|metadata| metadata.node.to_untrusted_node_address())
.collect()
fn nodes_from_point(&self) -> Vec<UntrustedNodeAddress> {

This comment has been minimized.

Copy link
@emilio

emilio Mar 2, 2017

Member

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>

This comment has been minimized.

Copy link
@emilio

emilio Mar 2, 2017

Member

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.

This comment has been minimized.

Copy link
@ferjm

ferjm Mar 2, 2017

Author Member

I see, good to know. Thank you!

…test to regular test checking only the fixed crash
@ferjm

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

@ferjm: 🔑 Insufficient privileges: Not in reviewers

@highfive highfive assigned emilio and unassigned jdm Mar 2, 2017
@ferjm

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@emilio I already addressed your feedback, but I don't seem have permissions to ask bors-servo to merge.

@KiChjang

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

📌 Commit 31d833f has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

⌛️ Testing commit 31d833f with merge fa32d50...

bors-servo added a commit that referenced this pull request Mar 2, 2017
…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 -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

@bors-servo bors-servo merged commit 31d833f into servo:master Mar 2, 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
@ferjm ferjm deleted the ferjm:issue-15592-document-elementsFromPoint branch Mar 2, 2017
@emilio

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

Thanks for fixing this again! :)

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.