Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upProperly handle scroll offsets in hit testing #16865
Conversation
highfive
commented
May 15, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
May 15, 2017
|
|
|
There is still a lot of code duplication in DisplayList, but I hope to fix that in a later PR. |
|
|
jonathandturner
commented
May 16, 2017
|
Initial results look great. Seems to have fixed the issue I was seeing. |
|
@jonathandturner That's great to hear! I've updated the PR to fix the merge conflicts. |
|
Looks good to me, thanks for fixing this! Only a few nits, and this is ready to land IMO. |
| } | ||
|
|
||
| fn full_offset_for_scroll_root(&mut self, id: &ClipId) -> Point2D<f32> { | ||
| match self.calculated_total_offsets.get(id) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
emilio
May 16, 2017
Member
Though this may be cleaner and slightly more efficient using the entry API.
This comment has been minimized.
This comment has been minimized.
mrobinson
May 16, 2017
Author
Member
I cannot use the entry API here, because I'm recursing causing a double mutable borrow. if let is a lot cleaner though.
| let mut point = *point - stacking_context.bounds.origin; | ||
| if stacking_context.scroll_policy == ScrollPolicy::Fixed { | ||
| let old_offset = offset_lookup.calculated_total_offsets.get(&clip_id).cloned(); | ||
| offset_lookup.calculated_total_offsets.insert(clip_id, Point2D::zero()); |
This comment has been minimized.
This comment has been minimized.
emilio
May 16, 2017
Member
When can a clip_id correspond to different StackingContext? Perhaps it would be nice to describe how Clips and StackingContext relate to each other, either here or in webrender's CLIPPING.md.
In any case, this looks correct to me.
This comment has been minimized.
This comment has been minimized.
mrobinson
May 16, 2017
Author
Member
We chatted about this on IRC, but the plan is that I will document everything after the last major API change. Hopefully that will happen in the next few weeks.
Scroll roots are no longer nested containers holding items, so instead we need to track the offsets of each, carefully handling fixed position items and stacking contexts that create new reference frames. Additionally, we remove the complexity of the pre-computed page scroll offset, instead opting to send script scrolls to the layout task in order to more quickly have a ScrollState there that matches the script's idea of the scroll world. Fixes #16405.
|
@bors-servo r=emilio |
|
|
Properly handle scroll offsets in hit testing Scroll roots are no longer nested containers holding items, so instead we need to track the offsets of each, carefully handling fixed position items and stacking contexts that create new reference frames. Additionally, we remove the complexity of the pre-computed page scroll offset, instead opting to send script scrolls to the layout task in order to more quickly have a ScrollState there that matches the script's idea of the scroll world. Fixes #16405. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16405. <!-- Either: --> - [ ] There are tests for these changes OR - [x] These changes do not require tests because it is currently impossible to test interactive scrolling with Servo's test infrastructure. Existing tests verify the script part though. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/16865) <!-- Reviewable:end -->
|
|
mrobinson commentedMay 15, 2017
•
edited by larsbergstrom
Scroll roots are no longer nested containers holding items, so instead
we need to track the offsets of each, carefully handling fixed position
items and stacking contexts that create new reference frames.
Additionally, we remove the complexity of the pre-computed page scroll
offset, instead opting to send script scrolls to the layout task in
order to more quickly have a ScrollState there that matches the
script's idea of the scroll world.
Fixes #16405.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is