Implement non-overlay mode for ::overscroll-area-parent [chromium/src : main]
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
Hey Philip, I think I need some guidance on how best to add support for entering multiple scroll translation transforms when we start the scrolling contents. In particular, you can see the ScrollTranslation for the content is inside the ScrollTranslation for the `::overscroll-area-parent` and we end up trying to switch directly to that context for the chunk.
return {};@p...@chromium.org How can we enter multiple scroll translations for a single chunk? The comment above implies that we should call this for each level but that's not happening with the current logic when entering a non-overlay scrollable area inside of multiple `::overscroll-area-parent` pseudos.
Test page:
```html
<!DOCTYPE html>
<style>
#container {
width: 200px;
height: 200px;
border: 1px solid black;
overflow: auto;
padding: 16px;
padding-right: 32px;
box-sizing: border-box;
}
#dismiss {
width: 500%;
height: 500%;
/* Overflows each edge by 100%. */
top: -200%;
left: -200%;
box-sizing: border-box;
background: radial-gradient(rgba(255, 255, 255, 0), rgba(0, 0, 0, 0.7));
pointer-events: none;
}
</style>
<div id="container" overscrollcontainer>
<div id="dismiss"></div>
<div id="content">
<h2>Content</h2>
<p>This is the content area.</p>
<p>There is a dismissable area that can be swiped to from any edge.</p>
<p>You can also scroll the content down...</p>
<p>to view the rest of this text.</p>
</div>
</div>
<button id=button command="toggle-overscroll" commandfor="dismiss">Dismiss</button>
<button id=button2 command="toggle-overscroll" commandfor="dismiss">Dismiss</button>
```
The resulting property trees and paint chunks are as follows:
```
[3431098:3431098:0205/202337.267305:INFO:third_party/blink/renderer/core/paint/paint_property_tree_printer.cc:230] Transform tree:
root 0xa3c004316a0 {"flattensInheritedTransform":false,"in_subtree_of_page_scale":false,"scroll":"0xa3c004317a8"}
VisualViewport Scale Node 0xa3c00563fd0 {"changed":"node-add-remove","flattensInheritedTransform":false,"in_subtree_of_page_scale":false,"directCompositingReasons":"Viewport","compositorElementId":"(576)"}
VisualViewport Translate Node 0xa3c005641c8 {"changed":"node-add-remove","flattensInheritedTransform":false,"directCompositingReasons":"Viewport","scroll":"0xa3c005640d8"}
PaintOffsetTranslation (LayoutView #document) 0xa3c005642d0 {"changed":"node-add-remove"}
ScrollTranslation (LayoutView #document) 0xa3c00564598 {"changed":"node-add-remove","directCompositingReasons":"RootScroller","scroll":"0xa3c005644a8"}
PaintOffsetTranslation (LayoutBlockFlow DIV id='container') 0xa3c005646a0 {"changed":"node-add-remove","translation2d":"[8 8]"}
PaintOffsetTranslation (LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent) 0xa3c00564a70 {"changed":"node-add-remove","translation2d":"[1 1]"}
ScrollTranslation (LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent) 0xa3c00564d38 {"changed":"node-add-remove","translation2d":"[-366 -396]","scroll":"0xa3c00564c48"}
ScrollTranslation (LayoutBlockFlow DIV id='container') 0xa3c00564968 {"changed":"node-add-remove","scroll":"0xa3c00564878"}
[3431098:3431098:0205/202337.287213:INFO:third_party/blink/renderer/core/paint/paint_property_tree_printer.cc:235] Clip tree:
root 0xa3c00431898 {"localTransformSpace":"0xa3c004316a0","rect":"-8.38861e+06,-8.38861e+06 1.67772e+07x1.67772e+07"}
OverflowClip (LayoutView #document) 0xa3c005643d8 {"changed":"node-add-remove","localTransformSpace":"0xa3c005642d0","rect":"0,0 800x600"}
OverflowClip (LayoutBlockFlow DIV id='container') 0xa3c005647a8 {"changed":"node-add-remove","localTransformSpace":"0xa3c005646a0","rect":"1,1 183x198"}
OverflowClip (LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent) 0xa3c00564b78 {"changed":"node-add-remove","localTransformSpace":"0xa3c00564a70","rect":"0,0 183x198"}
[3431098:3431098:0205/202337.293982:INFO:third_party/blink/renderer/core/paint/paint_property_tree_printer.cc:239] Effect tree:
root 0xa3c00431600 {"localTransformSpace":"0xa3c004316a0","outputClip":"0xa3c00431898"}
Caret 0xa3c00544e10 {"changed":"node-add-remove","localTransformSpace":"0xa3c004316a0","outputClip":"(nil)","opacity":0.00100000004749745,"directCompositingReasons":"ActiveOpacityAnimation","compositorElementId":"(138)"}
[3431098:3431098:0205/202337.299675:INFO:third_party/blink/renderer/core/paint/paint_property_tree_printer.cc:243] Scroll tree:
root 0xa3c004317a8 {"containerRect":"-8388608,-8388608 16777215x16777215","contentsRect":"-8388608,-8388608 16777215x16777215"}
VisualViewport Scroll Node 0xa3c005640d8 {"changed":"node-add-remove","containerRect":"0,0 800x600","contentsRect":"0,0 800x600","userScrollable":"both","maxScrollOffsetAffectedByPageScale":"true","compositorElementId":"(580)"}
Scroll (LayoutView #document) 0xa3c005644a8 {"changed":"node-add-remove","containerRect":"0,0 800x600","contentsRect":"0,0 800x600","overflowClipNode":"0xa3c005643d8","userScrollable":"both","compositorElementId":"(836)"}
Scroll (LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent) 0xa3c00564c48 {"changed":"node-add-remove","containerRect":"0,0 183x198","contentsRect":"0,0 915x990","overflowClipNode":"0xa3c00564b78","userScrollable":"both","compositorElementId":"(1028)","snap_container_rect":"0,0 183x198","snap_area_rects":["366,396 183x198","0,0 915x990"]}
Scroll (LayoutBlockFlow DIV id='container') 0xa3c00564878 {"changed":"node-add-remove","containerRect":"1,1 183x198","contentsRect":"1,1 183x363","overflowClipNode":"0xa3c005647a8","userScrollable":"both","compositorElementId":"(964)"}
[3431098:3431098:0205/202337.317354:VERBOSE1:third_party/blink/renderer/platform/graphics/paint/paint_controller.cc:852] PaintController::CommitNewDisplayItems() completed
[3431098:3431098:0205/202337.327462:INFO:third_party/blink/renderer/platform/graphics/paint/paint_controller_debug_data.cc:119] current paint artifact: [
{
"subsequence": "client: 0xa3c00520f50 LayoutView #document",
"chunks": [
{
"chunk": "LayoutView #document 0xa3c00520e38:LayoutView #document:ScrollHitTest:0",
"state": "t:0xa3c005642d0 c:0xa3c00431898 e:0xa3c00431600",
"bounds": "0,0 800x600",
"displayItems": []
},
{
"chunk": "Scrolling background of LayoutView #document 0xa3c004a20b8:Scrolling background of LayoutView #document:DrawingDocumentBackground:0",
"state": "t:0xa3c00564598 c:0xa3c005643d8 e:0xa3c00431600",
"bounds": "0,0 800x600",
"displayItems": [
"0: 0xa3c004a20b8:Scrolling background of LayoutView #document:DrawingDocumentBackground:0",
"1: 0xa3c005219f8:LayoutBlockFlow (inline, children-inline) BUTTON id='button':DrawingBoxDecorationBackground:0",
"2: 0xa3c00521b38:LayoutText #text:DrawingPaintPhaseForeground:0",
"3: 0xa3c00521ba8:LayoutText #text:DrawingPaintPhaseForeground:0",
"4: 0xa3c00521c18:LayoutBlockFlow (inline, children-inline) BUTTON id='button2':DrawingBoxDecorationBackground:0",
"5: 0xa3c00521cb8:LayoutText #text:DrawingPaintPhaseForeground:0"
]
},
{
"subsequence": "client: 0xa3c005211d0 LayoutBlockFlow DIV id='container'",
"chunks": [
{
"chunk": "LayoutBlockFlow DIV id='container' 0xa3c005211d0:LayoutBlockFlow DIV id='container':LayerChunk:0",
"state": "t:0xa3c005646a0 c:0xa3c005643d8 e:0xa3c00431600",
"bounds": "0,0 200x200",
"displayItems": [
"6: 0xa3c00521138:LayoutBlockFlow DIV id='container':DrawingBoxDecorationBackground:0"
]
},
{
"chunk": "LayoutBlockFlow DIV id='container' 0xa3c00521138:LayoutBlockFlow DIV id='container':ScrollHitTest:0",
"state": "t:0xa3c005646a0 c:0xa3c005643d8 e:0xa3c00431600",
"bounds": "0,0 200x200",
"displayItems": []
},
{
"chunk": "VerticalScrollbar 0xa3c0055c340:VerticalScrollbar:ScrollbarVertical:0",
"state": "t:0xa3c005646a0 c:0xa3c005643d8 e:0xa3c00431600",
"bounds": "184,1 15x198",
"displayItems": [
"7: 0xa3c0055c340:VerticalScrollbar:ScrollbarVertical:0"
]
},
{
"chunk": "LayoutBlockFlow DIV id='container' 0xa3c00521138:LayoutBlockFlow DIV id='container':ClipPaintPhaseSelfBlockBackgroundOnly:0",
"state": "t:0xa3c00564968 c:0xa3c005647a8 e:0xa3c00431600",
"bounds": "0,0 185x365",
"displayItems": [
"8: 0xa3c00521548:LayoutText #text:DrawingPaintPhaseForeground:0",
"9: 0xa3c00521658:LayoutText #text:DrawingPaintPhaseForeground:0",
"10: 0xa3c00521658:LayoutText #text:DrawingPaintPhaseForeground:1",
"11: 0xa3c00521768:LayoutText #text:DrawingPaintPhaseForeground:0",
"12: 0xa3c00521768:LayoutText #text:DrawingPaintPhaseForeground:1",
"13: 0xa3c00521768:LayoutText #text:DrawingPaintPhaseForeground:2",
"14: 0xa3c00521768:LayoutText #text:DrawingPaintPhaseForeground:3",
"15: 0xa3c00521878:LayoutText #text:DrawingPaintPhaseForeground:0",
"16: 0xa3c00521878:LayoutText #text:DrawingPaintPhaseForeground:1",
"17: 0xa3c00521988:LayoutText #text:DrawingPaintPhaseForeground:0",
"18: 0xa3c00521988:LayoutText #text:DrawingPaintPhaseForeground:1"
]
},
{
"subsequence": "client: 0xa3c005212c0 LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent",
"chunks": [
{
"chunk": "LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent 0xa3c005212c0:LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent:LayerChunk:0",
"state": "t:0xa3c00564a70 c:0xa3c005647a8 e:0xa3c00431600",
"bounds": "0,0 183x198",
"displayItems": []
},
{
"chunk": "LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent 0xa3c00521228:LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent:ScrollHitTest:0",
"state": "t:0xa3c00564a70 c:0xa3c005647a8 e:0xa3c00431600",
"bounds": "0,0 183x198",
"displayItems": []
},
{
"chunk": "LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent 0xa3c00521228:LayoutBlockFlow (positioned, children-inline) ::internal-overscroll-area-parent:ClipPaintPhaseSelfBlockBackgroundOnly:0",
"state": "t:0xa3c00564d38 c:0xa3c00564b78 e:0xa3c00431600",
"bounds": "0,0 915x990",
"displayItems": []
}
]
},
{
"subsequence": "client: 0xa3c005213b0 LayoutBlockFlow (positioned) DIV id='dismiss'",
"chunks": [
{
"chunk": "LayoutBlockFlow (positioned) DIV id='dismiss' 0xa3c005213b0:LayoutBlockFlow (positioned) DIV id='dismiss':LayerChunk:0",
"state": "t:0xa3c00564d38 c:0xa3c00564b78 e:0xa3c00431600",
"bounds": "0,0 915x990",
"displayItems": [
"19: 0xa3c00521318:LayoutBlockFlow (positioned) DIV id='dismiss':DrawingBoxDecorationBackground:0"
]
}
]
}
]
}
]
},
{
"chunk": "Inner Viewport Scroll Layer 0xa3c004079b8:Inner Viewport Scroll Layer:ForeignLayerViewportScroll:0",
"state": "t:0xa3c005641c8 c:0xa3c00431898 e:0xa3c00431600",
"bounds": "0,0 800x600",
"displayItems": [
"20: 0xa3c004079b8:Inner Viewport Scroll Layer:ForeignLayerViewportScroll:0"
]
}
]
```
Related details
- Philip Rogers
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")
This code is for raster inducing scroll. Normally, scrollers have clips, and so we can't nest without hitting clips, but you are adding that ability. I think we probably need to implement something like the logic in ConversionContext<Result>::SwitchToClip. I wonder if we should temporarily force composited scrolling (CompositingReasonFinder::ShouldForcePreferCompositingToLCDText) for this case, to separate out this tricky change from the overall change?
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. |
Philip Rogers (Gerrit)
Wait, looking closer, is the clip tree hierarchy different from the scroll tree hierarchy? Is that intended?
Robert Flack (Gerrit)
Robert Flack voted and added 1 comment![]( "Open in Gerrit")
Votes added by Robert Flack
| Commit-Queue | +1 |
1 comment
This was not intentional, though does raise the question of whether it should be allowed. For now though, the clipping follows what you would expect. Forcing composited scrolling for these scrollers nicely avoided the dcheck, thanks for the suggestion!
Related details
- Philip Rogers
- 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 added 5 comments![]( "Open in Gerrit")
EInternalOverscrollArea::kAuto) {nit: since these are internal, we might wanna consider changing "auto" to something more representative of "non-overlay" but I don't have any good ideas.
IsPseudo(kPseudoIdOverscrollAreaParent)Interesting. Just clarifying: is this to support multiple overscroll areas being active at the same time, which push the contents "below" them but not "above" them?
Might be worth a brief comment here
// TODO(flackr): Support raster inducing scroll on non-overlay overscrollnit: can you file a bug and reference it here?
if (box->Style()->IsInternalOverscrollArea() || box->IsScrollContainer()) {while here, and looking for micro-optimizations, we might wanna switch the order of this disjunction
Related details
- Philip Rogers
- 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. |
Philip Rogers (Gerrit)
Philip Rogers added 6 comments![]( "Open in Gerrit")
ADD_TRANSFORM(ContentTranslation, NodeId::kContent)I think you need to update FragmentData::ContentsTransform.
// +-[ ScrollTranslation ]Please update this diagram with your new node
Also update third_party/blink/renderer/core/paint/paint_property_tree_printer.cc
const LayoutBox* box = DynamicTo<LayoutBox>(object_);Please add
```
DCHECK(NeedsPaintPropertyUpdate());
DCHECK(NeedsScrollAndScrollTranslation(
object_, full_context_.direct_compositing_reasons));
```
return;Rather than returning, I think we need to clear any existing nodes with OnClearTransform(properties_->ClearContentTranslation());.
OnClearEffect(properties_->ClearScrollCornerEffect());Do you need to clear the new translation here?
return {};I think it might be better to investigate the clip tree / scroll tree issue, because that seems likely to cause you other tricky issues.
Can you add a test like CSSClipAbsPositionDescendant to `third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc` that asserts the hierarchy of the clip and scroll trees are what you want?
Related details
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 10 comments![]( "Open in Gerrit")
There is still an issue with multiple overscroll areas not correctly displaying / hit testing but they do have the correct geometry returned by getBoundingClientRect - see expected test failure.
Interesting. Just clarifying: is this to support multiple overscroll areas being active at the same time, which push the contents "below" them but not "above" them?
Might be worth a brief comment here
Done
// TODO(flackr): Support raster inducing scroll on non-overlay overscrollnit: can you file a bug and reference it here?
Sounds like something I'd ask for, done.
I think you need to update FragmentData::ContentsTransform.
Done
Please update this diagram with your new node
Also update third_party/blink/renderer/core/paint/paint_property_tree_printer.cc
Done
kContent = 10,Robert Flacknit: kContentTranslation
Done
const LayoutBox* box = DynamicTo<LayoutBox>(object_);Please add
```
DCHECK(NeedsPaintPropertyUpdate());
DCHECK(NeedsScrollAndScrollTranslation(
object_, full_context_.direct_compositing_reasons));
```
Done
Rather than returning, I think we need to clear any existing nodes with OnClearTransform(properties_->ClearContentTranslation());.
Done, presumably we need to clear in any of the paths where we don't create it - i.e. the other early return path later?
OnClearEffect(properties_->ClearScrollCornerEffect());Do you need to clear the new translation here?
Done. Yes I think so.
Sorry, when I said it "follows what you would expect", I meant I have updated it to construct the expected tree which matches the scroll tree, what I had in the original output was not expected I agree. I've added testing of the clip tree in OverscrollAreaTrackerPageTest.OverscrollPropertyTrees.
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. |
Vladimir Levin (Gerrit)
Vladimir Levin added 6 comments![]( "Open in Gerrit")
gfx::Vector2d PhysicalBoxFragment::PixelSnappedOverscrollContentOffset() const {I am not understanding how this function is used or what value it returns.
This is adding all of the pixel snapped scrolled content offset (which I think is a scroll offset from each of these things, right?), but it's accumulating it unconditionally regardless of the content drawn. In the other function, we do this only up to the overscroll area parent that the element resides in.
The problem here is that this is called on the container itself, and presumably wants to know the offset of its content, but the offset of its content depends on the content, no?
return IsScrollContainer() ? To<LayoutBox>(*GetLayoutObject())Is this change required or just clean up?
bool IsNonOverlayOverscrollScrollContainer() const {nit: we might want to think of a term for non-overlay that doesn't have a negation in its name
GetPhysicalFragment().PixelSnappedOverscrollContentOffset()) -Continuing my thought from the other comment: I guess because it's used here, and we know that the partially affected objects are all self painting paint layers, we know that we always want the total offset from all of the overscroll area parents.
I don't know where, but we might wanna document this somehow
if (!is_overlay) {Is this clipping the content to the container?
is_overlay ? scroll_translationI'm not sure I follow/remember this (could use a few comments):
- I believe this is setting the overscroll area parent's scroll/transform on the container (assuming one overscroll area parent), *not* on the menu element, right?
- the content transition is then the translation we apply to the content? Why is it itself in overlay and the parent in non-overlay... I don't get that part.
further down also if we're not in overlay, then we also set the content translation's parent to be the overscroll transform. But in that case content translation is itself scroll translation's parent
So we get overscroll_transform -> content_translation -> scroll_translation
is that right? I'm again not sure I follow why this is structured this way.
Related details
- Philip Rogers
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 6 comments![]( "Open in Gerrit")
gfx::Vector2d PhysicalBoxFragment::PixelSnappedOverscrollContentOffset() const {I am not understanding how this function is used or what value it returns.
This is adding all of the pixel snapped scrolled content offset (which I think is a scroll offset from each of these things, right?), but it's accumulating it unconditionally regardless of the content drawn. In the other function, we do this only up to the overscroll area parent that the element resides in.
The problem here is that this is called on the container itself, and presumably wants to know the offset of its content, but the offset of its content depends on the content, no?
In the other function, we do this only up to the overscroll area parent that the element resides in.
This function is only called for the overscrollcontainer to determine how much the content has been shifted. Since `::-internal-overscroll-area-parents` are self painting layers they don't use this offset. Perhaps they need to account for it as well, but I think it's not here - it should be handled the same way as self painting content within a scrolling container.
return IsScrollContainer() ? To<LayoutBox>(*GetLayoutObject())Is this change required or just clean up?
To simplify the caller, we now call this function whether it is a scroll container or not. Otherwise I'd have to check on the caller side whether it is a scroller before calling PixelSnappedScrolledContentOffset.
bool IsNonOverlayOverscrollScrollContainer() const {nit: we might want to think of a term for non-overlay that doesn't have a negation in its name
Agreed, it could be used instead of Auto in the internal css property value as well.
GetPhysicalFragment().PixelSnappedOverscrollContentOffset()) -Continuing my thought from the other comment: I guess because it's used here, and we know that the partially affected objects are all self painting paint layers, we know that we always want the total offset from all of the overscroll area parents.
I don't know where, but we might wanna document this somehow
Acknowledged
if (box->Style()->IsInternalOverscrollArea() || box->IsScrollContainer()) {while here, and looking for micro-optimizations, we might wanna switch the order of this disjunction
Done
if (!is_overlay) {Is this clipping the content to the container?
This is clipping the content to the `::-internal-overscroll-area-parent` clip, see discussion about the clip tree here https://chromium-review.git.corp.google.com/c/chromium/src/+/7545871/comment/2369d91d_a082f465/
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 2 comments![]( "Open in Gerrit")
// +-[ ContentTranslation ]Can you audit all calls of ReplacedContentTransform() and ScrollTranslation() and see if any need to be adjusted to include ContentTranslation()?
return {};- Robert Flack
- Vladimir Levin
Vladimir Levin (Gerrit)
Vladimir Levin added 2 comments![]( "Open in Gerrit")
gfx::Vector2d PhysicalBoxFragment::PixelSnappedOverscrollContentOffset() const {Robert FlackI am not understanding how this function is used or what value it returns.
This is adding all of the pixel snapped scrolled content offset (which I think is a scroll offset from each of these things, right?), but it's accumulating it unconditionally regardless of the content drawn. In the other function, we do this only up to the overscroll area parent that the element resides in.
The problem here is that this is called on the container itself, and presumably wants to know the offset of its content, but the offset of its content depends on the content, no?
In the other function, we do this only up to the overscroll area parent that the element resides in.
This function is only called for the overscrollcontainer to determine how much the content has been shifted. Since `::-internal-overscroll-area-parents` are self painting layers they don't use this offset. Perhaps they need to account for it as well, but I think it's not here - it should be handled the same way as self painting content within a scrolling container.
Yeah, this makes sense. Maybe just a comment is enough to say "This is for content not in overscroll areas"
return IsScrollContainer() ? To<LayoutBox>(*GetLayoutObject())Robert FlackIs this change required or just clean up?
To simplify the caller, we now call this function whether it is a scroll container or not. Otherwise I'd have to check on the caller side whether it is a scroller before calling PixelSnappedScrolledContentOffset.
Acknowledged
Related details
- Robert Flack
Robert Flack (Gerrit)
Robert Flack voted and added 3 comments![]( "Open in Gerrit")
Votes added by Robert Flack
| Commit-Queue | +1 |
3 comments
I have fixed the issue after diagramming it. I've transcribed the tree into comments on the all important SetOverscrollParent method and added a complete test of the transform tree hierarchy to the unit test. It is hit testing and visually correct now with multiple containing overscroll areas.
// +-[ ContentTranslation ]Can you audit all calls of ReplacedContentTransform() and ScrollTranslation() and see if any need to be adjusted to include ContentTranslation()?
I think I have accounted for all of these. Typically everywhere we want to know the actual translation will just use the ScrollTranslation directly.
PhysicalOffset(To<LayoutBox>(object_).ScrollOrigin());It may be possible to add the additional offset into the paint offset here rather than creating a dedicated ContentTranslation node.
Related details
- Philip Rogers
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Robert Flack
| Commit-Queue | +1 |
3 comments
I think this is working as expected now. I had to move the content clip to be outside of the overflow clip so that the overflow follows the same transform chain.
// +-[ ContentTranslation ]Robert FlackCan you audit all calls of ReplacedContentTransform() and ScrollTranslation() and see if any need to be adjusted to include ContentTranslation()?
I think I have accounted for all of these. Typically everywhere we want to know the actual translation will just use the ScrollTranslation directly.
Done
PhysicalOffset(To<LayoutBox>(object_).ScrollOrigin());It may be possible to add the additional offset into the paint offset here rather than creating a dedicated ContentTranslation node.
Actually I think it's necessary to have a node below the paint offset for the overflow clip to ensure that we have the correct parent tree.
Related details
- Philip Rogers
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. |
Vladimir Levin (Gerrit)
Vladimir Levin added 8 comments![]( "Open in Gerrit")
// The ::-internal-overscroll-area is always a direct child of thenit: s/area/area-parent/
is_overlay ? scroll_translationI'm not sure I follow/remember this (could use a few comments):
- I believe this is setting the overscroll area parent's scroll/transform on the container (assuming one overscroll area parent), *not* on the menu element, right?
- the content transition is then the translation we apply to the content? Why is it itself in overlay and the parent in non-overlay... I don't get that part.
further down also if we're not in overlay, then we also set the content translation's parent to be the overscroll transform. But in that case content translation is itself scroll translation's parent
So we get overscroll_transform -> content_translation -> scroll_translation
is that right? I'm again not sure I follow why this is structured this way.
Acknowledged
overscroll_container->GetLayoutBox()->Style()->InternalOverscrollArea() !=nit: we need to, at some point, to create a bunch of short direct functions to "name the state"
if (const auto* transform = properties_->ContentTranslation()) {I think this needs to run even if we don't need paint property update
OnClearTransform(properties_->ClearContentTranslation());not necessary? (you said so)
convert this test to an overlay hit-testing one
// TODO()nit:
- Philip Rogers
- Robert Flack
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
if (properties->ContentTranslation()) {Is the order here correct? I think we need to basically go from lowest to highest in the diagram in third_party/blink/renderer/core/paint/object_paint_properties.h, which would mean moving the ContentTranslation check down below the Perspective check.
kContentTranslation = 10,nit: keep the order the same as the transform tree diagram below? In other words, put kContentTranslation above kPerspective?
if (!NeedsPaintPropertyUpdate()) {Paint properties have a dirty bit (SetNeedsPaintPropertyUpdate()) and any changes to the inputs of this function need to also call SetNeedsPaintPropertyUpdate. For example, PaintLayer::UpdateScrollableArea calls SetNeedsPaintPropertyUpdate when PaintLayer::RequiresScrollableArea changes. Do all changes to the inputs of this function result in SetNeedsPaintPropertyUpdate being called? It's not obvious to me if the InternalOverscrollArea or GetOverscrollAreaTracker or scroll offset changes do or not.
As an example, we use the kPseudoIdOverscrollAreaParent scroll offset below. Scroll offset changes normally invalidate paint properties in PaintLayerScrollableArea::UpdateScrollOffset, but in this case, do we need to change that function to also invalidate another layout object's paint properties too?
Related details
- Robert Flack