-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement viewport stuffs for window #1718 #6875
Conversation
farodin91
commented
Jul 31, 2015
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon. |
Current Problem i get
if |
It should become clear what the problem is if you run |
console.log("ScrollX: "+ window.scrollX); | ||
} | ||
function scroll(){ | ||
window.scroll(0,0); |
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.
Oh, you're overwriting window.scroll with this new function :P
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.
ups
f589e8c
to
a9df5f2
Compare
Minimal support of Scroll working. |
We can use getBoundingClientRect on the document's root element for that, as mentioned on IRC. Also, instead of merging changes from master into your branch, could you rebase your changes instead? It makes it much easier to follow your changes when reviewing the code. |
11c09e6
to
160802d
Compare
getBoundingClientRect doesn't work correct for me. Return (0,0)
|
I'm working on Linux |
What if you use getBoundingClientRect on the document's body element instead? |
Current status the most work. if glutin implement this stuff |
Thanks for doing this! I'll give it a proper review sometime in the next couple days. |
☔ The latest upstream changes (presumably #6784) made this pull request unmergeable. Please resolve the merge conflicts. |
Conflicts: components/compositing/compositor.rs components/msg/compositor_msg.rs components/script/dom/webidls/Window.webidl components/script/dom/window.rs components/script/script_task.rs tests/html/viewport.html
Will Squash at the end |
Conflicts: components/script/dom/window.rs
Conflicts: components/compositing/compositor.rs components/msg/compositor_msg.rs
-S-awaiting-review +S-needs-code-changes Reviewed 1 of 9 files at r14, 11 of 11 files at r16. components/compositing/compositor_task.rs, line 69 [r16] (raw file): Comments from the review on Reviewable.io |
☔ The latest upstream changes (presumably #7401) made this pull request unmergeable. Please resolve the merge conflicts. |
Conflicts: components/script/dom/window.rs
|
||
// https://drafts.csswg.org/cssom-view/#dom-window-devicepixelratio | ||
fn DevicePixelRatio(self) -> Finite<f64> { | ||
let dpr = self.window_size.get() |
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.
This will return the wrong DPR if we are running with an option specifiying DPR. (See device_pixels_per_screen_px in compositor.rs: http://mxr.mozilla.org/servo/source/components/compositing/compositor.rs#1139)
Not sure how much we care though.
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.
This value comes from http://mxr.mozilla.org/servo/source/components/compositing/compositor.rs#779 which is derived from device_pixels_per_screen_px. Why is this wrong?
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.
Aaah I'm having trouble reading. Ignore me and carry on.
Time to squash! Thanks! Reviewed 6 of 6 files at r17. Comments from the review on Reviewable.io |
☔ The latest upstream changes (presumably #7417) made this pull request unmergeable. Please resolve the merge conflicts. |
Conflicts: components/compositing/headless.rs components/script/dom/window.rs
Implement viewport functions for window #1718 @jdm r? closes #6875 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7500) <!-- Reviewable:end -->
Implement viewport functions for window #1718 @jdm r? closes #6875 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7500) <!-- Reviewable:end -->
Implement viewport functions for window #1718 @jdm r? closes #6875 <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7500) <!-- Reviewable:end -->