Make scroll timelines support single-axis scroll containers [chromium/src : main]
Free Debreuil (Gerrit)
Free Debreuil voted and added 3 comments![]( "Open in Gerrit")
Votes added by Free Debreuil
| Auto-Submit | +1 |
3 comments
PTAL. The resolved physical direction is now stored in the timeline snapshot so changes to it invalidate the timeline and get pushed to the compositor, which now caches the direction (pending/active). Without it in the snapshot comparison, a change that leaves the offsets numerically identical (e.g. ltr -> rtl on an inline-axis timeline) would leave the compositor with a stale direction.
if (reference_type_ == ReferenceType::kNearestAncestor) {Free DebreuilRather than writing special code to find the nearest ancestor here, please share this with how we find the timeline scroll ancestor, but without the axis restriction so that it indeed finds the nearest one.
Good point, thanks. I moved the walking of ancestor scroll containers for both
cases into `ComputeLayoutObjectNoLayout()`. It now finds either the
`LayoutObject` used to resolve the writing direction, or the `LayoutObject`
used as the timeline source.
Both view timelines and scroll timelines using `scroll(nearest)` go through
the ancestor scroll container walk path.
if (Element* document_element =Free DebreuilI don't think we should need both of these conditions, can authors set the writing mode of the layout view? If so, maybe always use that? What do we do for similar properties like overscroll-behavior or scroll-behavior?
Gerrit is indicating this is untested, do we have a test covering getting the writing mode from the viewport?
Looking into this more, I think it makes sense to use the writing mode of the `LayoutView`. There [are somewhat complex rules to decide](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/style_resolver.cc?q=PropagateStyleToViewport) which writing mode to assign to the `LayoutView`. It takes the writing mode and direction from the <body> element when one exists (unless propagation is blocked, e.g. by containment), otherwise from the <html> element ([spec says the same](https://drafts.csswg.org/css-writing-modes-4/#principal-flow)). Using the value from the `LayoutView` means we automatically get the result of these rules.
[overscroll-behavior](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/style_resolver.cc;drc=c319c9a597db1ae3718e42f9cace8de9f2823ed4;l=3540) and [scroll-behavior](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/style_resolver.cc;drc=c319c9a597db1ae3718e42f9cace8de9f2823ed4;l=3642) also have custom rules, which are implemented in the same method linked above. `overscroll-behavior` uses the [viewport-defining element](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc?q=ViewportDefiningElement), so <body> if <html> is `overflow: visible`, otherwise <html>. `scroll-behavior` uses <html> and ignores <body>.
I added a WPT to cover getting the writing mode from the viewport. Please let me know if any changes are 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 3 comments![]( "Open in Gerrit")
// Only set while a scroll container matches.This comment is incredibly confusing. Rather than saying what it's not, state what it is. This is the physical direction of scrolling on the resolved axis. If you want to include how that axis is resolved, you could state logical axes are resolved on the nearest scroll container to the subject.
// to a physical axis. Depending on `reference_type_`, it is the objectSince the reference type of source doesn't affect the lookup, we don't need to even mention it here. You can document that the lookup determines whether we find the nearest scroll container or the nearest scroll container in the observed axis. However, I I think it would be simpler to pass the ScrollableAxes we care about directly to the shared helper method rather than adding a new enum.
// representation. Defaults to ScrollDown when the direction is unresolved;When would direction be unresolved when we're converting to a cc scroll direction?
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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Free Debreuil
| Auto-Submit | +1 |
PTAL.
// Only set while a scroll container matches.This comment is incredibly confusing. Rather than saying what it's not, state what it is. This is the physical direction of scrolling on the resolved axis. If you want to include how that axis is resolved, you could state logical axes are resolved on the nearest scroll container to the subject.
Good point thanks - updated. This is now stated directly, and added a small comment mentioning how logical axes are resolved.
// to a physical axis. Depending on `reference_type_`, it is the objectSince the reference type of source doesn't affect the lookup, we don't need to even mention it here. You can document that the lookup determines whether we find the nearest scroll container or the nearest scroll container in the observed axis. However, I I think it would be simpler to pass the ScrollableAxes we care about directly to the shared helper method rather than adding a new enum.
Good point - updated.
The method to pass in the scrollable axes, and removed the enum. The resolution of the nearest scroll container for the `kNearestAncestor` path now happens outside of `ComputeLayoutObjectNoLayout` which is easier to reason about.
Please let me know if additional changes are needed.
if (reference_type_ == ReferenceType::kNearestAncestor) {Free DebreuilRather than writing special code to find the nearest ancestor here, please share this with how we find the timeline scroll ancestor, but without the axis restriction so that it indeed finds the nearest one.
Good point, thanks. I moved the walking of ancestor scroll containers for both
cases into `ComputeLayoutObjectNoLayout()`. It now finds either the
`LayoutObject` used to resolve the writing direction, or the `LayoutObject`
used as the timeline source.Both view timelines and scroll timelines using `scroll(nearest)` go through
the ancestor scroll container walk path.
Done
if (Element* document_element =Free DebreuilI don't think we should need both of these conditions, can authors set the writing mode of the layout view? If so, maybe always use that? What do we do for similar properties like overscroll-behavior or scroll-behavior?
Gerrit is indicating this is untested, do we have a test covering getting the writing mode from the viewport?
Looking into this more, I think it makes sense to use the writing mode of the `LayoutView`. There [are somewhat complex rules to decide](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/style_resolver.cc?q=PropagateStyleToViewport) which writing mode to assign to the `LayoutView`. It takes the writing mode and direction from the <body> element when one exists (unless propagation is blocked, e.g. by containment), otherwise from the <html> element ([spec says the same](https://drafts.csswg.org/css-writing-modes-4/#principal-flow)). Using the value from the `LayoutView` means we automatically get the result of these rules.
[overscroll-behavior](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/style_resolver.cc;drc=c319c9a597db1ae3718e42f9cace8de9f2823ed4;l=3540) and [scroll-behavior](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/style_resolver.cc;drc=c319c9a597db1ae3718e42f9cace8de9f2823ed4;l=3642) also have custom rules, which are implemented in the same method linked above. `overscroll-behavior` uses the [viewport-defining element](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document.cc?q=ViewportDefiningElement), so <body> if <html> is `overflow: visible`, otherwise <html>. `scroll-behavior` uses <html> and ignores <body>.
I added a WPT to cover getting the writing mode from the viewport. Please let me know if any changes are needed.
Done
// representation. Defaults to ScrollDown when the direction is unresolved;When would direction be unresolved when we're converting to a cc scroll direction?
The value could be unresolved if it was not set on the Blink timeline snapshot, for example [if the timeline is invalid](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/scroll_timeline.cc;drc=826c14a312e39eaee24cf7c82ec85c950f054cd7;l=85).
I think the cleaner approach would be to allow the value to be `std::nullopt`, and propagate that through to the compositor `ScrollTimeline`. This follows [the existing pattern of other fields there](https://source.chromium.org/chromium/chromium/src/+/main:cc/animation/scroll_timeline.h;drc=04a531fe414c309d8d87952617ee2aac4e3343fd;l=176). I've updated the patch to this approach. Please let me know if any changes are 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. |