Implement Overscroll Effects on Non-root Scrollers [chromium/src : main]
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
PTAL :) I think some changes will be needed, but sending out a first pass for feedback.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 2 comments![]( "Open in Gerrit")
void ObserveScrollUpdate(cc::ElementId element_id,Free DebreuilBlink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `element_id` can be omitted from the function declaration in the header file as its type `cc::ElementId` makes its purpose clear. Please consider applying this to other modified function declarations in this file where applicable.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
// Returns the ElementId of the currently latched scroller, or invalid id.Free Debreuilnit: Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters. This getter could be named `LatchedScrollerElementId()`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 8 comments![]( "Open in Gerrit")
if (!is_root && !scroll_node->is_composited) {Add a comment here explaining why.
auto& elastic_overscroll = host_impl_->active_tree()Can you include the type here instead of using auto so that the reader doesn't have to check what type elastic_overscroll() returns to know what structure it is or what operations are valid on it?
It's a bit of a fuzzy area when using auto is good or bad but general guidlines are at https://google.github.io/styleguide/cppguide.html#Type_deduction
if (!is_root && !scroll_node->is_composited) {If the goal is to never stretch a non-composited scroller, then i think you don't need to handle it on line 92, and could convert that to a CHECK. You also need to make sure to clear out any elastic overscrolls on scrollers which become non-composited during a commit in the ScrollTree::operator=.
layer_tree_impl()->OverscrollElasticityTransformNode()) {I have a few concerns with simply trying to undo the local content scale. Is this a problem with root elastic overscroll today?
If not, is this something we could do in a followup patch to make sure we get the details right?
If it is, could we move this fix to an earlier patch to the rest of this change?
There was a prior attempt to avoid rerastering in https://issues.chromium.org/40228454 . In particular, I think we rely on ignoring subtle changes to transform nodes where screen_space_transform_is_animating is true as we don't trigger updates to the raster scale unless we are significantly off.
DirtyTransformForElasticOverscroll(id, layer_tree_impl, property_trees);Rather than iterating over the maps at draw time, we should set the appropriate bits when the elastic overscroll is changed (e.g. when calling ScrollTree::SetElasticOverscroll). This should result in much less book keeping - e.g. we won't have to keep the previous values around.
// Find an active entry that is actually overscrolled and finish that one.This is concerning. How do we get into this state and can we do this earlier when we have the correct id?
ObserveScrollUpdate(element_id, event_delta,In this and many other cases we're doing repeated lookups in the entry map for the element. Could we make these methods take an entry instead of an element id?
helper_->SetStretchAmount(element_id, new_stretch_amount);I think it would be cleaner to set is_animating = true here rather than doing a second pass through the list after.
Related details
- Free Debreuil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
DirtyTransformForElasticOverscroll(id, layer_tree_impl, property_trees);Rather than iterating over the maps at draw time, we should set the appropriate bits when the elastic overscroll is changed (e.g. when calling ScrollTree::SetElasticOverscroll). This should result in much less book keeping - e.g. we won't have to keep the previous values around.
Nice, yes that significantly simplifies it. The most recent patch set has this change. We have to be careful to re-apply the overscroll values to the transform when promoting the pending tree to active. (I have added this)
Please let me know if there are any other cases I am not considering.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
layer_tree_impl()->OverscrollElasticityTransformNode()) {I have a few concerns with simply trying to undo the local content scale. Is this a problem with root elastic overscroll today?
If not, is this something we could do in a followup patch to make sure we get the details right?
If it is, could we move this fix to an earlier patch to the rest of this change?
There was a prior attempt to avoid rerastering in https://issues.chromium.org/40228454 . In particular, I think we rely on ignoring subtle changes to transform nodes where screen_space_transform_is_animating is true as we don't trigger updates to the raster scale unless we are significantly off.
Yes - I am looking into this now! On main within the `PictureLayerImpl` the tiles seem to be created once, then the `cc::Tile::raster_transform` limits the maximum scaling. Looking into that area - and will compare with my version to find root cause of difference. Thanks 😊
Free Debreuil (Gerrit)
Free Debreuil added 8 comments![]( "Open in Gerrit")
PTAL :)
if (!is_root && !scroll_node->is_composited) {Add a comment here explaining why.
added comment.
auto& elastic_overscroll = host_impl_->active_tree()Can you include the type here instead of using auto so that the reader doesn't have to check what type elastic_overscroll() returns to know what structure it is or what operations are valid on it?
It's a bit of a fuzzy area when using auto is good or bad but general guidlines are at https://google.github.io/styleguide/cppguide.html#Type_deduction
Updated with explicit type, thanks for the reference!
if (!is_root && !scroll_node->is_composited) {If the goal is to never stretch a non-composited scroller, then i think you don't need to handle it on line 92, and could convert that to a CHECK. You also need to make sure to clear out any elastic overscrolls on scrollers which become non-composited during a commit in the ScrollTree::operator=.
updated.
layer_tree_impl()->OverscrollElasticityTransformNode()) {Free DebreuilI have a few concerns with simply trying to undo the local content scale. Is this a problem with root elastic overscroll today?
If not, is this something we could do in a followup patch to make sure we get the details right?
If it is, could we move this fix to an earlier patch to the rest of this change?
There was a prior attempt to avoid rerastering in https://issues.chromium.org/40228454 . In particular, I think we rely on ignoring subtle changes to transform nodes where screen_space_transform_is_animating is true as we don't trigger updates to the raster scale unless we are significantly off.
Yes - I am looking into this now! On main within the `PictureLayerImpl` the tiles seem to be created once, then the `cc::Tile::raster_transform` limits the maximum scaling. Looking into that area - and will compare with my version to find root cause of difference. Thanks 😊
Resolved by setting `transform_node->has_potential_animation` to true when the animation of the node with elastic overscroll starts.
Possibly there is an edge case here. I added a TODO, but could also look into before submitting.
```
// TODO (crbug.com/41102897): Look into potential edge case where clearing
// this on a transform with an animation could result in this value being
// cleared to false un-intentionally. This would only be an issue for
// non-root scrollers.
```
DirtyTransformForElasticOverscroll(id, layer_tree_impl, property_trees);Free DebreuilRather than iterating over the maps at draw time, we should set the appropriate bits when the elastic overscroll is changed (e.g. when calling ScrollTree::SetElasticOverscroll). This should result in much less book keeping - e.g. we won't have to keep the previous values around.
Nice, yes that significantly simplifies it. The most recent patch set has this change. We have to be careful to re-apply the overscroll values to the transform when promoting the pending tree to active. (I have added this)
Please let me know if there are any other cases I am not considering.
Marked as resolved.
ObserveScrollUpdate(element_id, event_delta,In this and many other cases we're doing repeated lookups in the entry map for the element. Could we make these methods take an entry instead of an element id?
Looking through the methods in this class, most use cases require both the `ElementId` and `OverscrollEntry`. To cleanly pass in just the entry, I think we would need to store the `ElementId` on the `OverscrollEntry`.
It might be straightforward to convert the storage of the `OverscrollEntries` from a `base::flat_map` to a `base::flat_set` and add a custom equality / comparator that allows lookup based on `ElementId` instead of `OverscrollEntry`?
helper_->SetStretchAmount(element_id, new_stretch_amount);I think it would be cleaner to set is_animating = true here rather than doing a second pass through the list after.
Updated thanks!
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
// Find an active entry that is actually overscrolled and finish that one.This is concerning. How do we get into this state and can we do this earlier when we have the correct id?
Any thoughts on this fallback code? Do you have a repro case? Can we detect when the element id goes away and do the cleanup then?
ObserveScrollUpdate(element_id, event_delta,Free DebreuilIn this and many other cases we're doing repeated lookups in the entry map for the element. Could we make these methods take an entry instead of an element id?
Looking through the methods in this class, most use cases require both the `ElementId` and `OverscrollEntry`. To cleanly pass in just the entry, I think we would need to store the `ElementId` on the `OverscrollEntry`.
It might be straightforward to convert the storage of the `OverscrollEntries` from a `base::flat_map` to a `base::flat_set` and add a custom equality / comparator that allows lookup based on `ElementId` instead of `OverscrollEntry`?
In the short term I wouldn't worry about having the ElementId as both the key and in the stored member. It's true this could just be a flat_set but we could always do this as a followup or alternately you could pass both the element id and the entry through.
Related details
- Free Debreuil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
// Find an active entry that is actually overscrolled and finish that one.Robert FlackThis is concerning. How do we get into this state and can we do this earlier when we have the correct id?
Any thoughts on this fallback code? Do you have a repro case? Can we detect when the element id goes away and do the cleanup then?
I am looking into this one currently - I can repro in Chrome by scrolling multiple times in different scrollers w/ overscroll - possibly it would be good to make a test that simulates this as well.
From what I can tell so far, in `InputHandlerProxy` during the gesture lifecycle (`HandleGestureScrollBegin`, `Update`, etc..) the `CurrentlyScrollingNode` (from the `ScrollTree`) seems to be being cleared - which results in the issue. I'm looking into exactly what is clearing it now : )
We could possibly detect during the `HandleGestureScrollUpdate` if the `CurrentlyScrollingNode` is cleared - and do something then if needed.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
// Find an active entry that is actually overscrolled and finish that one.Robert FlackThis is concerning. How do we get into this state and can we do this earlier when we have the correct id?
Free DebreuilAny thoughts on this fallback code? Do you have a repro case? Can we detect when the element id goes away and do the cleanup then?
I am looking into this one currently - I can repro in Chrome by scrolling multiple times in different scrollers w/ overscroll - possibly it would be good to make a test that simulates this as well.
From what I can tell so far, in `InputHandlerProxy` during the gesture lifecycle (`HandleGestureScrollBegin`, `Update`, etc..) the `CurrentlyScrollingNode` (from the `ScrollTree`) seems to be being cleared - which results in the issue. I'm looking into exactly what is clearing it now : )
We could possibly detect during the `HandleGestureScrollUpdate` if the `CurrentlyScrollingNode` is cleared - and do something then if needed.
Yeah that would be what I would expect, that if we clear the currently scrolling node then we effectively "end" the user scrolling on that node.
Related details
- Free Debreuil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
// Find an active entry that is actually overscrolled and finish that one.Robert FlackThis is concerning. How do we get into this state and can we do this earlier when we have the correct id?
Free DebreuilAny thoughts on this fallback code? Do you have a repro case? Can we detect when the element id goes away and do the cleanup then?
Robert FlackI am looking into this one currently - I can repro in Chrome by scrolling multiple times in different scrollers w/ overscroll - possibly it would be good to make a test that simulates this as well.
From what I can tell so far, in `InputHandlerProxy` during the gesture lifecycle (`HandleGestureScrollBegin`, `Update`, etc..) the `CurrentlyScrollingNode` (from the `ScrollTree`) seems to be being cleared - which results in the issue. I'm looking into exactly what is clearing it now : )
We could possibly detect during the `HandleGestureScrollUpdate` if the `CurrentlyScrollingNode` is cleared - and do something then if needed.
Yeah that would be what I would expect, that if we clear the currently scrolling node then we effectively "end" the user scrolling on that node.
Looking into this more, it was being caused by a call to `InputHandler::ElasticOverscrollAnimationFinished` from a scroller that already had finished it's GSB GSU GSE lifecycle. This was not an issue before, as only a single (root) scroller could animate at once.
I just piped through the `ElementId` of the overscroll animation that is finishing, and filter calling `InputHandler::ScrollEnd` if that id matches the `CurrentlyScrollingNode()`.
The "fallback" has been removed, it is just a straight `CHECK` now.
Please let me know if there's any issues with this approach.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Free Debreuil
| Commit-Queue | +1 |
1 comment
ObserveScrollUpdate(element_id, event_delta,Free DebreuilIn this and many other cases we're doing repeated lookups in the entry map for the element. Could we make these methods take an entry instead of an element id?
Robert FlackLooking through the methods in this class, most use cases require both the `ElementId` and `OverscrollEntry`. To cleanly pass in just the entry, I think we would need to store the `ElementId` on the `OverscrollEntry`.
It might be straightforward to convert the storage of the `OverscrollEntries` from a `base::flat_map` to a `base::flat_set` and add a custom equality / comparator that allows lookup based on `ElementId` instead of `OverscrollEntry`?
In the short term I wouldn't worry about having the ElementId as both the key and in the stored member. It's true this could just be a flat_set but we could always do this as a followup or alternately you could pass both the element id and the entry through.
I have updated the `OverscrollEntry` to store the `ElementId` of the target scroller, then pass just that into the relevant methods. Yes, we could do the `flat_set` or related optimization in a follow up if needed.
Please let me know if there are any additional issues. :)
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
PTAL :) I have addressed feedback - please let me know if any additional changes are needed.
Related details
- Robert Flack
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 6 comments![]( "Open in Gerrit")
if (const ScrollNode* current = CurrentlyScrollingNode()) {nit: Switch this for an early return to reduce nesting here.
if (execute_scroll_end && !IsAnimatingForSnap(current->element_id)) {Does this mean that if you start scrolling another scroller before the elastic overscroll finishes we won't be executing scroll end for nested scrollers? Can this result in them not correctly snapping to a mandatory snap area? This seems like it will be a bug.
void ApplyStretchAmount(LayerTreeHostImpl& host_impl,This method seems mis-named. It's not actually applying the stretch, just marking that the node needs an update.
#if BUILDFLAG(IS_ANDROID)nit: It should be documented why we are doing this (i.e. that it prevents re-rastering based on the constantly updating transform), and why on Android only (i.e. because it is only on android where we have a stretch that affects the raster scale). That said, I think it should be perfectly safe to set this even when we aren't applying a scale given we are still animating the transform frame to frame.
transform_node->has_potential_animation = !stretch_amount.IsZero();I also wouldn't be opposed
transform_node->SetTransformChanged(DamageReason::kUntracked);This seems like it should be kCompositorScroll?
Related details
- Free Debreuil
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
transform_node->has_potential_animation = !stretch_amount.IsZero();I also wouldn't be opposed
Oops, what i meant to say here is i wouldn't be opposed to always setting this true. Though resetting it to false for zero stretch will ensure that we eventually raster at 1x in case the initial raster happened during a stretched moment somehow.
Related details
- Free Debreuil
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 6 comments![]( "Open in Gerrit")
if (const ScrollNode* current = CurrentlyScrollingNode()) {nit: Switch this for an early return to reduce nesting here.
Marked as resolved.
if (execute_scroll_end && !IsAnimatingForSnap(current->element_id)) {Does this mean that if you start scrolling another scroller before the elastic overscroll finishes we won't be executing scroll end for nested scrollers? Can this result in them not correctly snapping to a mandatory snap area? This seems like it will be a bug.
Nice catch, yes this is an issue. I have added a test for this in `layer_tree_host_unittest_scroll.cc` called `LayerTreeHostScrollTestSnapAfterInterruptedOverscroll`. Have added a `flat_map` to track the deferred end values.
This method seems mis-named. It's not actually applying the stretch, just marking that the node needs an update.
Marked as resolved.
nit: It should be documented why we are doing this (i.e. that it prevents re-rastering based on the constantly updating transform), and why on Android only (i.e. because it is only on android where we have a stretch that affects the raster scale). That said, I think it should be perfectly safe to set this even when we aren't applying a scale given we are still animating the transform frame to frame.
Resolved. I have kept it as an Android only path to minimize possible behaviour changes. No problem to update if needed.
transform_node->has_potential_animation = !stretch_amount.IsZero();I also wouldn't be opposed
Nice, yes that should be relatively safe - likely better than having `has_potential_animation` turned off accidentally.
I have set the root node case to set directly as a special case, as that is the current behaviour on main - and I think animations do not touch that directly?
Thanks!
transform_node->SetTransformChanged(DamageReason::kUntracked);This seems like it should be kCompositorScroll?
Have updated to use `kCompositorScroll`.
For reference, in the original code path, it was setting to `DamageReason::kUntracked` on the non-Android path.
https://source.chromium.org/chromium/chromium/src/+/main:cc/trees/draw_property_utils.cc;drc=747e8c8f36453eb7cf97a55f53a5937121365208;l=1531
Related details
- Robert Flack
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
class LayerTreeHostTestElasticOverscroll_ScaledAnimation@vmp...@chromium.org I added a test that applies the overscroll effect to an animating scroller and ensures that re-tiling does not happen - which should ensure we do not get performance regressions for this case.
Related details
- Robert Flack
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
transform_node->has_potential_animation = !stretch_amount.IsZero();Free DebreuilI also wouldn't be opposed
Nice, yes that should be relatively safe - likely better than having `has_potential_animation` turned off accidentally.
I have set the root node case to set directly as a special case, as that is the current behaviour on main - and I think animations do not touch that directly?
Thanks!
Sorry to go back on this, I was just looking into the potential implications of this and it looks like this has non-local effects on descendants. E.g. if you look at screen_space_transform_is_animating, it will be true if any ancestor is animating transform. I think this means we should keep the conservative option of only setting it true when we have a non-zero stretch.
Otherwise, we may keep non ideal raster scales in descendants where we didn't before.
struct OverscrollEntry {nit: With this having virtual methods and overrides let's upgrade it to a class.
Related details
- Free Debreuil
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 2 comments![]( "Open in Gerrit")
transform_node->has_potential_animation = !stretch_amount.IsZero();Free DebreuilI also wouldn't be opposed
Robert FlackNice, yes that should be relatively safe - likely better than having `has_potential_animation` turned off accidentally.
I have set the root node case to set directly as a special case, as that is the current behaviour on main - and I think animations do not touch that directly?
Thanks!
Sorry to go back on this, I was just looking into the potential implications of this and it looks like this has non-local effects on descendants. E.g. if you look at screen_space_transform_is_animating, it will be true if any ancestor is animating transform. I think this means we should keep the conservative option of only setting it true when we have a non-zero stretch.
Otherwise, we may keep non ideal raster scales in descendants where we didn't before.
Absolutely - updated! The original issue is tracked directly in a unit test with a TODO (`layer_tree_host_impl.cc`, `LayerTreeHostTestElasticOverscroll_ScaledAnimation`).
struct OverscrollEntry {nit: With this having virtual methods and overrides let's upgrade it to a class.
Updated - made fields public as there aren't invariants requiring protection. Happy to introduce getters/setters if preferred by the style guide.
Related details
- Robert Flack
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin voted![]( "Open in Gerrit")
- Free Debreuil
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Free Debreuil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Implement Overscroll Effects on Non-root Scrollers
Extend `ElasticOverscrollController` and `ScrollElasticityHelper` to
support per-scroller overscroll effects, rather than just the root.
Overscroll state is now tracked per `ElementId`.
Application of overscroll effect moved to
`TransformTree::UpdateLocalTransform` instead of
`DrawPropertyUtils::UpdateElasticOverscroll`, to allow combining of
overscroll effect with scroller's content transform.
BYPASS_LARGE_CHANGE_WARNING: Code needs to be submitted together
to be testable.
- M cc/input/compositor_input_interfaces.h
- M cc/input/input_handler.cc
- M cc/input/input_handler.h
- M cc/input/scroll_elasticity_helper.cc
- M cc/input/scroll_elasticity_helper.h
- M cc/trees/draw_property_utils.cc
- M cc/trees/layer_tree_host_impl.cc
- M cc/trees/layer_tree_host_impl.h
- M cc/trees/layer_tree_host_unittest.cc
- M cc/trees/layer_tree_host_unittest_scroll.cc
- M cc/trees/layer_tree_impl.cc
- M cc/trees/layer_tree_impl.h
- M cc/trees/layer_tree_impl_unittest.cc
- M cc/trees/property_tree.cc
- M cc/trees/property_tree.h
- M cc/trees/property_tree_unittest.cc
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller.cc
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller.h
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller_bezier.cc
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller_bezier.h
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller_bezier_unittest.cc
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller_exponential.cc
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller_exponential.h
- M third_party/blink/renderer/platform/widget/input/elastic_overscroll_controller_exponential_unittest.cc
- M third_party/blink/renderer/platform/widget/input/input_handler_proxy.cc
- M third_party/blink/renderer/platform/widget/input/input_handler_proxy.h
- M third_party/blink/renderer/platform/widget/input/widget_input_handler_manager.cc
Code-Review: +1 by Robert Flack, +1 by Vladimir Levin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |