Fix background painting when scrolling in reversed-flow containers [chromium/src : main]
Eain Chen (Gerrit)
Eain Chen added 1 comment![]( "Open in Gerrit")
if (box.StyleRef().FlexDirection() == EFlexDirection::kRowReverse) {WIP: Add test cases to cover the following new logic.
Related details
- Claire Chambers
- Olga Gerchikov
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. |
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
// be adjusted. The default scroll offset is calculated from the top-left
// (or start) of the content. However, in a reversed container, scrolling
// is calculated from the bottom-right (or end).This comment annoyed me, because this isn't a given - inline flows can go in any direction based on language.
I discovered that your bug also repros for `direction: rtl`. In that case, reversing the flow direction actually *fixes* the bug (because it's reversed twice).
Are we sure the scroll offsets are being used correctly here? This might need to be resolved upstream from here, by calculating absolute offsets as always being the distance from the origin to start. It seems that when the flow direction is inverted we keep using the original origin - your change simply corrects it after the fact for a single property.
Related details
- Eain Chen
- Olga Gerchikov
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. |
Eain Chen (Gerrit)
Eain Chen added 1 comment![]( "Open in Gerrit")
// be adjusted. The default scroll offset is calculated from the top-left
// (or start) of the content. However, in a reversed container, scrolling
// is calculated from the bottom-right (or end).This comment annoyed me, because this isn't a given - inline flows can go in any direction based on language.
I discovered that your bug also repros for `direction: rtl`. In that case, reversing the flow direction actually *fixes* the bug (because it's reversed twice).
Are we sure the scroll offsets are being used correctly here? This might need to be resolved upstream from here, by calculating absolute offsets as always being the distance from the origin to start. It seems that when the flow direction is inverted we keep using the original origin - your change simply corrects it after the fact for a single property.
Thanks, it's hard to describe clearly with these direction words. I'll try to reduce the ambiguity.
I'll also investigate the case of `direction: rtl`. In this bug, when `flex-direction` is reversed, the behavior appears similar, which suggests there may be a shared underlying issue around how scroll offsets are interpreted. I’ll try to apply the correct calculation upstream.
Related details
- Claire Chambers
- Olga Gerchikov
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. |
Eain Chen (Gerrit)
Eain Chen added 1 comment![]( "Open in Gerrit")
// be adjusted. The default scroll offset is calculated from the top-left
// (or start) of the content. However, in a reversed container, scrolling
// is calculated from the bottom-right (or end).Eain ChenThis comment annoyed me, because this isn't a given - inline flows can go in any direction based on language.
I discovered that your bug also repros for `direction: rtl`. In that case, reversing the flow direction actually *fixes* the bug (because it's reversed twice).
Are we sure the scroll offsets are being used correctly here? This might need to be resolved upstream from here, by calculating absolute offsets as always being the distance from the origin to start. It seems that when the flow direction is inverted we keep using the original origin - your change simply corrects it after the fact for a single property.
Thanks, it's hard to describe clearly with these direction words. I'll try to reduce the ambiguity.
I'll also investigate the case of `direction: rtl`. In this bug, when `flex-direction` is reversed, the behavior appears similar, which suggests there may be a shared underlying issue around how scroll offsets are interpreted. I’ll try to apply the correct calculation upstream.
After investigating, the scroll offset originates from `PaintLayerScrollableArea::scroll_offset_`. We also found that there are universal standers based on [https://www.w3.org/TR/cssom-view-1/#scrolling-events], so, we cannot change the core logic to calculate absolute offsets.
To proceed with the fix, I am applying the same offset adjustment logic during the painting stage for `direction: rtl`, as the rendering mechanism is the same. The key to this approach is using the XOR to correctly handle the `rtl` case alongside `flex-direction: row-reverse`.
I did some tests on my fix and on Firefox. They work as expected. The manual tests details are in the CL description.
Related details
- Claire Chambers
- Olga Gerchikov
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. |
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
// be adjusted. The default scroll offset is calculated from the top-left
// (or start) of the content. However, in a reversed container, scrolling
// is calculated from the bottom-right (or end).Eain ChenThis comment annoyed me, because this isn't a given - inline flows can go in any direction based on language.
I discovered that your bug also repros for `direction: rtl`. In that case, reversing the flow direction actually *fixes* the bug (because it's reversed twice).
Are we sure the scroll offsets are being used correctly here? This might need to be resolved upstream from here, by calculating absolute offsets as always being the distance from the origin to start. It seems that when the flow direction is inverted we keep using the original origin - your change simply corrects it after the fact for a single property.
Eain ChenThanks, it's hard to describe clearly with these direction words. I'll try to reduce the ambiguity.
I'll also investigate the case of `direction: rtl`. In this bug, when `flex-direction` is reversed, the behavior appears similar, which suggests there may be a shared underlying issue around how scroll offsets are interpreted. I’ll try to apply the correct calculation upstream.
After investigating, the scroll offset originates from `PaintLayerScrollableArea::scroll_offset_`. We also found that there are universal standers based on [https://www.w3.org/TR/cssom-view-1/#scrolling-events], so, we cannot change the core logic to calculate absolute offsets.
To proceed with the fix, I am applying the same offset adjustment logic during the painting stage for `direction: rtl`, as the rendering mechanism is the same. The key to this approach is using the XOR to correctly handle the `rtl` case alongside `flex-direction: row-reverse`.
I did some tests on my fix and on Firefox. They work as expected. The manual tests details are in the CL description.
Perhaps I should have been a little more specific with my guidance, I suspected that chromium probably already had a way of doing this that didn't require any calculating, and after duing a little perusing I found ScrollableArea::ScrollPosition and ScrollableArea::ScrollOffsetToPosition
These do exactly what you intend without creating any potentially brittle property specific logic.
I built locally with the following change:
```
diff --git a/third_party/blink/renderer/core/paint/box_fragment_painter.cc b/third_party/blink/renderer/core/paint/box_fragment_painter.cc
index f66e439deb98a..2c5c582b0a02a 100644
--- a/third_party/blink/renderer/core/paint/box_fragment_painter.cc
+++ b/third_party/blink/renderer/core/paint/box_fragment_painter.cc
@@ -2218,8 +2218,11 @@ PhysicalRect BoxFragmentPainter::AdjustRectForScrolledContent(
PhysicalRect scrolled_paint_rect = rect;
// Adjust the paint rect to reflect a scrolled content box with borders at
// the ends.
- scrolled_paint_rect.offset -=
- PhysicalOffset(physical.PixelSnappedScrolledContentOffset());
+ if (PaintLayerScrollableArea* scroller =
+ To<LayoutBox>(physical.GetLayoutObject())->GetScrollableArea()) {
+ scrolled_paint_rect.offset -=
+ PhysicalOffset::FromPointFFloor(scroller->ScrollPosition());
+ }
scrolled_paint_rect.size =
physical.ScrollSize() +
PhysicalSize(borders.HorizontalSum(), borders.VerticalSum());
```
And it seems to pass all cases. Under the hood, it uses the scroll origin rather than calculating the max offset.
Do you think this would work? I think it would be better if it's possible to re-use existing functionality.
Related details
- Eain Chen
- Olga Gerchikov
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. |
Eain Chen (Gerrit)
Eain Chen added 1 comment![]( "Open in Gerrit")
Thanks, re-use the ScrollPosition() method looks good for all scenarios. It's much easier.
Related details
- Claire Chambers
- Olga Gerchikov
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. |
Eain Chen (Gerrit)
Eain Chen added 2 comments![]( "Open in Gerrit")
// be adjusted. The default scroll offset is calculated from the top-left
// (or start) of the content. However, in a reversed container, scrolling
// is calculated from the bottom-right (or end).Done
if (box.StyleRef().FlexDirection() == EFlexDirection::kRowReverse) {WIP: Add test cases to cover the following new logic.
Done
Related details
- Claire Chambers
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Gerchikov (Gerrit)
Olga Gerchikov added 1 comment![]( "Open in Gerrit")
.container::-webkit-scrollbar {Please use 'scrollbar-width: none' instead (https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/scrollbar-width). -webkit-scrollbar is not standard: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/::-webkit-scrollbar
Related details
- Claire Chambers
- Eain Chen
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. |
Claire Chambers (Gerrit)
Claire Chambers voted Code-Review+1![]( "Open in Gerrit")
- Eain Chen
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. |