[scrollbar-gutter] Fix "scrollbar-gutter: stable both" clipping on macOS [chromium/src : main]
David Grogan (Gerrit)
David Grogan added 3 comments![]( "Open in Gerrit")
Hi Stefan and Ian,
Please review this CL that you've consulted on in the past. Its hopefully approaching its final form here.
https://chromium-review.googlesource.com/c/chromium/src/+/7121439 has your original changes from a few months ago in case you want to look.
state.contents_rect = {Example 2, from bottom left of scrollbar-gutter-content-overflowing.html:
```html
<div style="width: 185px; height: 185px; overflow-x: hidden; overflow-y: auto; scrollbar-gutter: stable both-edges; will-change: transform;">
<div style="width: 500px; height: 50px; margin-left: -100px;"></div>
<div style="width: 185px; height: 500px;"></div>
</div>
```
Before this change, state.contents_size was `400x550`. This worked!
After this change, `state.contents_rect = 16,1 400x550` _before_ the `Union`. This **breaks** this example's rendering, with the contents not painting over the left gutter even though it should, because of the -margin.
Then, after we `Union` with `state.container_rect = 1,1 170x185`, we get the final `state.contents_rect = 1,1 415x550`, which DOES work.
state.contents_rect = {Example 1, from bottom middle of -003.html:
```html
<div style="width: 200px; height: 200px; overflow-y: scroll; scrollbar-gutter: stable both-edges; will-change: transform;">
<div style="width: 200px; height: 400px;"></div>
</div>
```
Before this change, state.contents_size was `170x400`. Each scrollbar/gutter was 15px wide. This resulted in the right-most 15px of content being (incorrectly) clipped because this box had an implicit offset of `0,0`.
After this change, `state.contents_rect = 15,0 170x400` _before_ the `Union`. This on its own works for this case.
Then, after we `Union` with `state.container_rect = 0,0 185x200`, we get the final `state.contents_rect = 0,0 185x400`. This still works.
Related details
- Ian Kilpatrick
- Stefan Zager
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. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
state.contents_rect = {Example 2, from bottom left of scrollbar-gutter-content-overflowing.html:
```html
<div style="width: 185px; height: 185px; overflow-x: hidden; overflow-y: auto; scrollbar-gutter: stable both-edges; will-change: transform;">
<div style="width: 500px; height: 50px; margin-left: -100px;"></div>
<div style="width: 185px; height: 500px;"></div>
</div>
```Before this change, state.contents_size was `400x550`. This worked!
After this change, `state.contents_rect = 16,1 400x550` _before_ the `Union`. This **breaks** this example's rendering, with the contents not painting over the left gutter even though it should, because of the -margin.
Then, after we `Union` with `state.container_rect = 1,1 170x185`, we get the final `state.contents_rect = 1,1 415x550`, which DOES work.
Oops, the scroller had `border: 1px solid` when I logged all the geometry for this example. I accidentally omitted that property from the markup here, but it explains the 1s and 16s.
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
box.ScrollableOverflowRect().PixelSnappedOffset() +Is `box.ScrollableOverflowRect().PixelSnappedOffset()` the same as `state.container_rect.location`? If it is, then I think using the latter formulation here makes the code easier to grok. If it's not, then can you add a comment (or replace the current comment) explaining how they are different?
Related details
- David Grogan
- Ian Kilpatrick
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. |
David Grogan (Gerrit)
David Grogan voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Grogan
| Commit-Queue | +1 |
1 comment
box.ScrollableOverflowRect().PixelSnappedOffset() +Is `box.ScrollableOverflowRect().PixelSnappedOffset()` the same as `state.container_rect.location`? If it is, then I think using the latter formulation here makes the code easier to grok. If it's not, then can you add a comment (or replace the current comment) explaining how they are different?
Heh, I got `box.ScrollableOverflowRect().PixelSnappedOffset()` from you.
(And I assume you meant `state.container_rect.origin_`.) They are different, but I don't know the terminology to describe them accurately and succinctly.
I want to say that `container_rect.origin` is the distance from the top left of the container's outer border box to the top left of the inner scroll box, not including any gutters. By contrast, `ScrollableOverflowRect().PixelSnappedOffset()` DOES include the gutters.
So, in the horizontal-tb ltr case,
```
<div style="width: 200px; height: 200px; border: 1px solid; scrollbar-gutter: stable both-edges; overflow: auto;">
<div style="width: 100px; height: 400px;">
```
```
state.container_rect.origin() = 1,1
box.ScrollableOverflowRect().PixelSnappedOffset() = 16,1
```
Then `rtl` changes the scroll origin:
```
<div style="width: 200px; height: 200px; border: 1px solid; direction: rtl; overflow: auto;">
<div style="width: 400px; height: 100px;">
```
```
state.container_rect.origin() = 1,1
box.ScrollableOverflowRect().PixelSnappedOffset() = -199,1
```
So `ScrollableOverflowRect().PixelSnappedOffset()` looks like the distance between the top-left of the container's border box and the top-left of the scrollable overflow rect.
Do you know I should describe these?
(I've been looking at this picture: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/doc/rtl-tb-scroll.png .)
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- David Grogan
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. |
Stefan Zager (Gerrit)
Stefan Zager voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stefan Zager
| Code-Review | +1 |
2 comments
lgtm
box.ScrollableOverflowRect().PixelSnappedOffset() +David GroganIs `box.ScrollableOverflowRect().PixelSnappedOffset()` the same as `state.container_rect.location`? If it is, then I think using the latter formulation here makes the code easier to grok. If it's not, then can you add a comment (or replace the current comment) explaining how they are different?
Heh, I got `box.ScrollableOverflowRect().PixelSnappedOffset()` from you.
(And I assume you meant `state.container_rect.origin_`.) They are different, but I don't know the terminology to describe them accurately and succinctly.
I want to say that `container_rect.origin` is the distance from the top left of the container's outer border box to the top left of the inner scroll box, not including any gutters. By contrast, `ScrollableOverflowRect().PixelSnappedOffset()` DOES include the gutters.
So, in the horizontal-tb ltr case,
```
<div style="width: 200px; height: 200px; border: 1px solid; scrollbar-gutter: stable both-edges; overflow: auto;">
<div style="width: 100px; height: 400px;">
```
```
state.container_rect.origin() = 1,1
box.ScrollableOverflowRect().PixelSnappedOffset() = 16,1
```Then `rtl` changes the scroll origin:
```
<div style="width: 200px; height: 200px; border: 1px solid; direction: rtl; overflow: auto;">
<div style="width: 400px; height: 100px;">
```
```
state.container_rect.origin() = 1,1
box.ScrollableOverflowRect().PixelSnappedOffset() = -199,1
```So `ScrollableOverflowRect().PixelSnappedOffset()` looks like the distance between the top-left of the container's border box and the top-left of the scrollable overflow rect.
Do you know I should describe these?
(I've been looking at this picture: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/doc/rtl-tb-scroll.png .)
OK I understand better now, no need to document this (I don't think it would be all that helpful).
David Grogan (Gerrit)
David Grogan voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Grogan
| Commit-Queue | +2 |
3 comments
state.contents_rect = {Example 1, from bottom middle of -003.html:
```html
<div style="width: 200px; height: 200px; overflow-y: scroll; scrollbar-gutter: stable both-edges; will-change: transform;">
<div style="width: 200px; height: 400px;"></div>
</div>
```Before this change, state.contents_size was `170x400`. Each scrollbar/gutter was 15px wide. This resulted in the right-most 15px of content being (incorrectly) clipped because this box had an implicit offset of `0,0`.
After this change, `state.contents_rect = 15,0 170x400` _before_ the `Union`. This on its own works for this case.
Then, after we `Union` with `state.container_rect = 0,0 185x200`, we get the final `state.contents_rect = 0,0 185x400`. This still works.
Acknowledged
state.contents_rect = {David GroganExample 2, from bottom left of scrollbar-gutter-content-overflowing.html:
```html
<div style="width: 185px; height: 185px; overflow-x: hidden; overflow-y: auto; scrollbar-gutter: stable both-edges; will-change: transform;">
<div style="width: 500px; height: 50px; margin-left: -100px;"></div>
<div style="width: 185px; height: 500px;"></div>
</div>
```Before this change, state.contents_size was `400x550`. This worked!
After this change, `state.contents_rect = 16,1 400x550` _before_ the `Union`. This **breaks** this example's rendering, with the contents not painting over the left gutter even though it should, because of the -margin.
Then, after we `Union` with `state.container_rect = 1,1 170x185`, we get the final `state.contents_rect = 1,1 415x550`, which DOES work.
Oops, the scroller had `border: 1px solid` when I logged all the geometry for this example. I accidentally omitted that property from the markup here, but it explains the 1s and 16s.
Acknowledged
box.ScrollableOverflowRect().PixelSnappedOffset() +David GroganIs `box.ScrollableOverflowRect().PixelSnappedOffset()` the same as `state.container_rect.location`? If it is, then I think using the latter formulation here makes the code easier to grok. If it's not, then can you add a comment (or replace the current comment) explaining how they are different?
Stefan ZagerHeh, I got `box.ScrollableOverflowRect().PixelSnappedOffset()` from you.
(And I assume you meant `state.container_rect.origin_`.) They are different, but I don't know the terminology to describe them accurately and succinctly.
I want to say that `container_rect.origin` is the distance from the top left of the container's outer border box to the top left of the inner scroll box, not including any gutters. By contrast, `ScrollableOverflowRect().PixelSnappedOffset()` DOES include the gutters.
So, in the horizontal-tb ltr case,
```
<div style="width: 200px; height: 200px; border: 1px solid; scrollbar-gutter: stable both-edges; overflow: auto;">
<div style="width: 100px; height: 400px;">
```
```
state.container_rect.origin() = 1,1
box.ScrollableOverflowRect().PixelSnappedOffset() = 16,1
```Then `rtl` changes the scroll origin:
```
<div style="width: 200px; height: 200px; border: 1px solid; direction: rtl; overflow: auto;">
<div style="width: 400px; height: 100px;">
```
```
state.container_rect.origin() = 1,1
box.ScrollableOverflowRect().PixelSnappedOffset() = -199,1
```So `ScrollableOverflowRect().PixelSnappedOffset()` looks like the distance between the top-left of the container's border box and the top-left of the scrollable overflow rect.
Do you know I should describe these?
(I've been looking at this picture: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/doc/rtl-tb-scroll.png .)
OK I understand better now, no need to document this (I don't think it would be all that helpful).
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57111.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
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
[scrollbar-gutter] Fix "scrollbar-gutter: stable both" clipping on macOS
Previously, ScrollPaintPropertyNode only stored contents_size with no
offset, effectively assuming the content always started at the offset
(0,0). With scrollbar gutters appearing on the left, that is no longer
accurate.
Because the code assumed an offset of 0, the contents_rect was
positioned too far to the left relative to the actual content. This
caused the right-most part of the content to fall outside the calculated
layer bounds, resulting in incorrect clipping on the right side.
This CL changes contents_size to contents_rect in
ScrollPaintPropertyNode to allow specifying an explicit offset.
In PaintPropertyTreeBuilder, we now:
1. Calculate the contents_rect offset relative to the container's border
box, correctly accounting for the gutter shift.
2. Union contents_rect with container_rect. This ensures the scrolling
contents layer fully covers the scrollport (including the gutter),
preventing clipping of overflowing content in the gutter area.
- M third_party/blink/renderer/core/frame/visual_viewport.cc
- M third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
- M third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor_test.cc
- M third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.cc
- M third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/renderer/platform/testing/paint_property_test_helpers.h
- A third_party/blink/web_tests/external/wpt/css/css-overflow/scrollbar-gutter-003.html
- M third_party/blink/web_tests/wpt_internal/css/css-overflow/scrollbar-gutter-content-overflowing-ref.html
- M third_party/blink/web_tests/wpt_internal/css/css-overflow/scrollbar-gutter-content-overflowing.html
Code-Review: +1 by Stefan Zager, +1 by Ian Kilpatrick
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57111
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |