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. |