[gestures] Chain overscroll to overscroll-area-parent pseudos [chromium/src : main]
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
->SetParent(*properties_->Scroll());Just need to figure how to cleanly set this to be the current context's scroll parent without the const cast here.
Related details
- 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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
size_t size() const { return overscroll_parents_.size(); }Blink Style Guide: Naming - Use 'CamelCase' for all function names. Please rename 'size' to 'Size'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This 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)_
Related details
- 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 2 comments![]( "Open in Gerrit")
Philip would you be the right reviewer for this?
->SetParent(*properties_->Scroll());Just need to figure how to cleanly set this to be the current context's scroll parent without the const cast here.
Seems like there are roughly 3 options:
1. Allow setting the scroll parent, maybe make the current scroll context non-const? This is locally a simple change but likely to have lots of cascading consequences.
2. Change the tree walk. In particular, we'd need to walk the overscroll-area-parents right before UpdateScrollAndScrollTranslation() so it seems like we'd have to pass the PrePaintTreeWalk instance into UpdateForChildren and then call Walk on the overscroll-area-parents at the right time, and skip them later in WalkChildren.
3. Add an alias scroll node which initially just points to the current context parent but can be overridden by a descendant later.
Related details
- Philip Rogers
- 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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Koji, would you be okay giving this review a try?
Robert, could you link to any docs that could help Koji in either the change description or the bug? E.g., the explainer.
Related details
- Koji Ishii
- Robert Flack
- 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")
Koji, would you be okay giving this review a try?
Robert, could you link to any docs that could help Koji in either the change description or the bug? E.g., the explainer.
We have an explainer at https://github.com/vmpstr/htmldemos/blob/master/gestures/READMEv2.md
The TLDR is that we want to make the `::overscroll-area-parent` pseudos the parent scrollers to their owning element, followed by chaining scrolling to the element's ancestor scroll container. In this way, a user can scroll the element normally, until they reach the limit at which they will scroll into this overscroll area. If that is at the limit then it finally chains as scroll normally would to the ancestor.
Related details
- Koji Ishii
- Philip Rogers
- Vladimir Levin
Vladimir Levin (Gerrit)
Vladimir Levin added 3 comments![]( "Open in Gerrit")
!builder.OverscrollArea()->GetNames().empty())) {Can you leave a TODO here. I think the plan for the future is to support not having layout containment as well
return kForcedPaintLayer;What's the difference between kNormalPaintLayer and kForcedPaintLayer?
->SetParent(*properties_->Scroll());Robert FlackJust need to figure how to cleanly set this to be the current context's scroll parent without the const cast here.
Seems like there are roughly 3 options:
1. Allow setting the scroll parent, maybe make the current scroll context non-const? This is locally a simple change but likely to have lots of cascading consequences.
2. Change the tree walk. In particular, we'd need to walk the overscroll-area-parents right before UpdateScrollAndScrollTranslation() so it seems like we'd have to pass the PrePaintTreeWalk instance into UpdateForChildren and then call Walk on the overscroll-area-parents at the right time, and skip them later in WalkChildren.
3. Add an alias scroll node which initially just points to the current context parent but can be overridden by a descendant later.
Personally, as a non-owner, I think we can add something like SetParentForOverscroll(...) where it would do the const cast. One thing we need to CHECK is that for example current.scroll already has a parent (it's not root), since setting a parent on the root scroll is going to be bad.
Basically, I think we need to add sufficient CHECKs here to verify that we're in the situation we expect to be in, and if so then const_cast.
We could also just inline all the right checks
Related details
- Koji Ishii
- Philip Rogers
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 3 comments![]( "Open in Gerrit")
Can you leave a TODO here. I think the plan for the future is to support not having layout containment as well
Done
return kForcedPaintLayer;What's the difference between kNormalPaintLayer and kForcedPaintLayer?
It's briefly documented here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_box_model_object.h;l=43;drc=603c83d9fab0e9e06eba701fe1ed92db3ab2cb21
My understanding is that kNormalPaintLayer will result in a self painting layer with the effective side effects of a stacking context, whereas kForcedPaintLayer/kOverflowClipPaintLayer (which is used for non-visible overflow) does not - this is strictly a layer created for ensuring we create the appropriate property nodes.
->SetParent(*properties_->Scroll());Robert FlackJust need to figure how to cleanly set this to be the current context's scroll parent without the const cast here.
Vladimir LevinSeems like there are roughly 3 options:
1. Allow setting the scroll parent, maybe make the current scroll context non-const? This is locally a simple change but likely to have lots of cascading consequences.
2. Change the tree walk. In particular, we'd need to walk the overscroll-area-parents right before UpdateScrollAndScrollTranslation() so it seems like we'd have to pass the PrePaintTreeWalk instance into UpdateForChildren and then call Walk on the overscroll-area-parents at the right time, and skip them later in WalkChildren.
3. Add an alias scroll node which initially just points to the current context parent but can be overridden by a descendant later.
Personally, as a non-owner, I think we can add something like SetParentForOverscroll(...) where it would do the const cast. One thing we need to CHECK is that for example current.scroll already has a parent (it's not root), since setting a parent on the root scroll is going to be bad.
Basically, I think we need to add sufficient CHECKs here to verify that we're in the situation we expect to be in, and if so then const_cast.
We could also just inline all the right checks
I created a special SetOverscrollParent method in ContainingBlockContext to further the discussion.
Related details
- Koji Ishii
- Philip Rogers
- 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")
Robert FlackKoji, would you be okay giving this review a try?
Robert, could you link to any docs that could help Koji in either the change description or the bug? E.g., the explainer.
We have an explainer at https://github.com/vmpstr/htmldemos/blob/master/gestures/READMEv2.md
The TLDR is that we want to make the `::overscroll-area-parent` pseudos the parent scrollers to their owning element, followed by chaining scrolling to the element's ancestor scroll container. In this way, a user can scroll the element normally, until they reach the limit at which they will scroll into this overscroll area. If that is at the limit then it finally chains as scroll normally would to the ancestor.
Seems like Koji is out this week. Ian, would you be able to have a look or know who could review this?
Related details
- Ian Kilpatrick
- Philip Rogers
- 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. |