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 upAllow gecko to provide the already-applied sticky offset to WR #1977
Conversation
staktrace
force-pushed the
staktrace:sticky
branch
3 times, most recently
from
fc9752b
to
ffa56ab
|
Sorry for taking so long to review this. I have a few comments... |
| @@ -454,6 +457,14 @@ impl ClipScrollNode { | |||
| } | |||
| } | |||
|
|
|||
| fn clamp_to_bounds( | |||
This comment has been minimized.
This comment has been minimized.
mrobinson
Nov 3, 2017
Member
It probably makes more sense for this to be a trait on f32 if possible. The self value is unused here.
| } | ||
| debug_assert!(sticky_offset.y >= 0.0); | ||
| debug_assert!(sticky_offset.y + info.applied_offset.y >= 0.0); | ||
| } | ||
|
|
||
| if sticky_offset.y == 0.0 { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
staktrace
Nov 3, 2017
Author
Contributor
You're right, we do. When I thought it through before I thought this was correct but thinking it through again now you're right. (For posterity, the specific thought-example is the case where gecko paints something with a top-offset already applied, and then with the async scroll we completely unapply that offset and apply a bottom-offset).
| @@ -999,6 +999,9 @@ impl YamlFrameWriter { | |||
|
|
|||
| yaml_node(&mut v, "horizontal-offset-bounds", Yaml::Array(horizontal)); | |||
| yaml_node(&mut v, "vertical-offset-bounds", Yaml::Array(vertical)); | |||
|
|
|||
| f32_node(&mut v, "applied-offset-x", item.applied_offset.x); | |||
This comment has been minimized.
This comment has been minimized.
mrobinson
Nov 3, 2017
Member
Thanks for adding wrench support! I think it would be good to add a test for this as well, since there is a risk of breaking such a particular use case.
This comment has been minimized.
This comment has been minimized.
| // the calling code, stored in info.applied_offset, and the extra amount we | ||
| // computed as a result of scrolling, stored in sticky_offset) needs to be | ||
| // clamped to the provided bounds. | ||
| sticky_offset.y = self.clamp_to_bounds(sticky_offset.y + info.applied_offset.y, &info.vertical_offset_bounds) - info.applied_offset.y; |
This comment has been minimized.
This comment has been minimized.
| /// has been scrolled down, such that the sticky item's position needed to be offset | ||
| /// downwards by `applied_offset.y`. A negative y component corresponds to the upward | ||
| /// offset applied due to bottom-stickiness. The x-axis works analogously. | ||
| pub applied_offset: LayoutVector2D, |
This comment has been minimized.
This comment has been minimized.
mrobinson
Nov 3, 2017
Member
Maybe this should be called something like previously_applied_offset to avoid confusing it with the current offset provided by WebRender?
This comment has been minimized.
This comment has been minimized.
staktrace
Nov 3, 2017
Author
Contributor
I was thinking of that too but wasn't sure how about variable name length. I can change it.
|
Updated. For the clamp function I just made a little lambda inside the main function which I think is cleaner than before. While writing the reftests I also discovered that neither |
|
Travis failures are just timeouts; please ask bors to retry if the updated patch looks good to you. |
|
|
|
Looks good to me. Feel free to rebase and land this. |
|
Rebased ^. Can somebody with permissions stamp this with r=mrobinson please? Thanks! |
|
@bors-servo r=mrobinson Thanks! |
|
|
Allow gecko to provide the already-applied sticky offset to WR Fixes the remaining issue in #1912. r? @mrobinson <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1977) <!-- Reviewable:end -->
|
|
staktrace commentedNov 1, 2017
•
edited by larsbergstrom
Fixes the remaining issue in #1912.
r? @mrobinson
This change is