[Blink] Implement single-axis scroll containers for sticky position [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
PhysicalAxes handled_axes = kPhysicalAxesBoth) const;Free DebreuilThis method should be returning the 1D data.
Free DebreuilPosting here for reference (from chat):
```
It might make sense to making the return structure like:
SomethingPair {
horizontal_;
veritcal_;
}
Then setting the sticky constraints per-axis.
```
Implemented as `struct StickyConstraintsData`. Please let me know if there are any issues with this approach.
GetMutableForPainting().FirstFragment().SetStickyConstraints(new_constraint);Free DebreuilAnd setting here should only be setting each of the axis separately rather than mutating the to set the old data.
It works at the moment, but its fragile, e.g. what happens if a style change means that we'll set a different axis - but not clear the old data?
Free DebreuilFrom my understanding, the old axes will need to be cleared out by querying the `ComputedStyle`. That is the intention of this code - but possibly it could be structured to explicitly clear the old axes to be clearer.
```
// Only copy over axes from the `old_constraint` that are still valid from the
// `ComputedStyle`. This handles edge cases where fragment descendants are
// reused when the scroll container parent changes axes handled.
const PhysicalAxes valid_axes = StickyConstrainedAxes(StyleRef());
```
I have updated the logic to create a new constraints object, instead of mutating the old one. Please let me know if additional changes or restructuring would make sense to do.
Related details
- Ian Kilpatrick
- Steve Kobes
- 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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 4 comments![]( "Open in Gerrit")
SetStickyConstraints(StickyConstraintsData{});I think you may want to clear the per-axis sticky constraints here.
E.g. just clear one if changed now.
struct CORE_EXPORT StickyPositionScrollingConstraints finalWhat does it look like if we make StickyPositionScrollingConstraints
STACK_ALLOCATED()
and store the PerAxisData directly on the FragmentData?
It'd likely simplify the SetStickyContraints logic.
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal) != kPhysicalAxesNone) {```suggestion
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal)) {
```
I'm fine with above if that works for you (and elsewhere)
sticky_descendant->SetStickyConstraints(std::move(data))) {So the order of calling UpdateAllStickyConstraints isn't guaranteed.
E.g. a layer with a deeper depth may get called before/after the other layer.
It'd likely make the logic within SetStickyConstraints simpler if we just store the PerAxisData directly instead of having the wrapping StickyPositionConstraints object.
Related details
- Free Debreuil
- Steve Kobes
- Vladimir Levin
Free Debreuil (Gerrit)
Free Debreuil voted and added 6 comments![]( "Open in Gerrit")
Votes added by Free Debreuil
| Auto-Submit | +1 |
PTAL : )
I think you may want to clear the per-axis sticky constraints here.
E.g. just clear one if changed now.
Good point - that is much cleaner and less error prone - and is in line with the existing behavior as well. I've updated it so that it clears per-axis as needed.
GetMutableForPainting().FirstFragment().SetStickyConstraints(new_constraint);Free DebreuilAnd setting here should only be setting each of the axis separately rather than mutating the to set the old data.
It works at the moment, but its fragile, e.g. what happens if a style change means that we'll set a different axis - but not clear the old data?
Free DebreuilFrom my understanding, the old axes will need to be cleared out by querying the `ComputedStyle`. That is the intention of this code - but possibly it could be structured to explicitly clear the old axes to be clearer.
```
// Only copy over axes from the `old_constraint` that are still valid from the
// `ComputedStyle`. This handles edge cases where fragment descendants are
// reused when the scroll container parent changes axes handled.
const PhysicalAxes valid_axes = StickyConstrainedAxes(StyleRef());
```
I have updated the logic to create a new constraints object, instead of mutating the old one. Please let me know if additional changes or restructuring would make sense to do.
`SetStickyConstraints` is now very simple - as the clearing logic has been moved to `LayoutBoxModelObject::StyleDidChange`.
struct CORE_EXPORT StickyPositionScrollingConstraints finalWhat does it look like if we make StickyPositionScrollingConstraints
STACK_ALLOCATED()and store the PerAxisData directly on the FragmentData?
It'd likely simplify the SetStickyContraints logic.
That works nicely I think - I've updated it so that the `StickyPositionScrollingConstraints` (now STACK_ALLOCATED) is created transiently from the `PerAxisData` (now stored directly on the `FragmentData`) to preserve existing ergonomics.
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal) != kPhysicalAxesNone) {```suggestion
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal)) {
```I'm fine with above if that works for you (and elsewhere)
I think that would be a good syntax - but `PhysicalAxes` is a `using` of `base::StrongAlias`, which does not implement the `bool` operator.
Primitives such as `std::bitset` have an `any()` syntax which is nice (e.g. `(scroll_axes & kPhysicalAxesHorizontal).any()`) - but maybe non-trivial to add.
I've kept the existing syntax - no problem to switch to a different syntax that's clearer if needed.
sticky_descendant->SetStickyConstraints(std::move(data))) {So the order of calling UpdateAllStickyConstraints isn't guaranteed.
E.g. a layer with a deeper depth may get called before/after the other layer.
It'd likely make the logic within SetStickyConstraints simpler if we just store the PerAxisData directly instead of having the wrapping StickyPositionConstraints object.
Nice - yes that is a good solution - the `StickyConstraintsData` is now assigned directly to the `PerAxisData`.
Please let me know if there's any issues with the implementation.
Related details
- Ian Kilpatrick
- Steve Kobes
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 5 comments![]( "Open in Gerrit")
Thanks for the changes!
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal) != kPhysicalAxesNone) {Free Debreuil```suggestion
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal)) {
```I'm fine with above if that works for you (and elsewhere)
I think that would be a good syntax - but `PhysicalAxes` is a `using` of `base::StrongAlias`, which does not implement the `bool` operator.
Primitives such as `std::bitset` have an `any()` syntax which is nice (e.g. `(scroll_axes & kPhysicalAxesHorizontal).any()`) - but maybe non-trivial to add.
I've kept the existing syntax - no problem to switch to a different syntax that's clearer if needed.
bool SetStickyConstraints(StickyConstraintsData constraints) {Now that we have a ClearStickyConstraints, is it valid to pass a "empty" constraints here (e.g. add DCHECK?)
To<LayoutBoxModelObject>(object).StickyConstraints());would a HasStickyConstraints make these callsites easier?
up to yout.
auto layout_constraint = box_model.StickyConstraints();.nit const ?
Related details
- Free Debreuil
- Steve Kobes
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Free Debreuil (Gerrit)
Free Debreuil voted and added 5 comments![]( "Open in Gerrit")
Votes added by Free Debreuil
| Auto-Submit | +1 |
5 comments
Thank you for reviewing! Please let me know if additional changes are needed.
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal) != kPhysicalAxesNone) {Free Debreuil```suggestion
if (x_data_ && (scroll_axes & kPhysicalAxesHorizontal)) {
```I'm fine with above if that works for you (and elsewhere)
Ian KilpatrickI think that would be a good syntax - but `PhysicalAxes` is a `using` of `base::StrongAlias`, which does not implement the `bool` operator.
Primitives such as `std::bitset` have an `any()` syntax which is nice (e.g. `(scroll_axes & kPhysicalAxesHorizontal).any()`) - but maybe non-trivial to add.
I've kept the existing syntax - no problem to switch to a different syntax that's clearer if needed.
Hmmm..... can we do something like:
https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/common/unique_ids.h;l=27-34?q=%22public%20base::StrongAlias%22&ss=chromium
?
Good point with the subclass. I thought that wouldn't be possible, since we weren't injecting the inherited type into `StrongAlias` with CRTP (so I assumed there would be operators that would result in the `StrongAlias` type instead of the inherited type).
But it looks like there are no methods / operators in there that return the `StrongAlias` type - so inheritance is a nice approach.
Have updated the definition and usage sites. Thanks!
bool SetStickyConstraints(StickyConstraintsData constraints) {Now that we have a ClearStickyConstraints, is it valid to pass a "empty" constraints here (e.g. add DCHECK?)
Yes good point - have updated this with an additional `DCHECK`.
To<LayoutBoxModelObject>(object).StickyConstraints());would a HasStickyConstraints make these callsites easier?
up to yout.
Yes - that is better - have updated thanks!
auto layout_constraint = box_model.StickyConstraints();Free Debreuil.nit const ?
Updated thank you.
Related details
- Ian Kilpatrick
- Steve Kobes
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 4 comments![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Code-Review | +1 |
4 comments
lgtm - Thanks for your patience! This looks great.
CHECK(walk == &scroll_container_layer || is_fixed_to_view);.nit scope so that `walk` doesn't leak into the rest of this long function.
```
bool is_fixed_to_view = false;
{
const PaintLayer* walk = Layer();
... //etc
}
```
if (!axes_to_clear || !rare_data_) {
return has_changed;
}```suggestion
if (!rare_data_) {
return false;
}
```
(no need to short-circuit axes_to_clear)
return To<LayoutBoxModelObject>(object).HasStickyConstraints();This could arguably be:
```
if (const auto* box_model = DynamicTo<LayoutBoxModelObject>(object)) {
return box_model->HasStickyConstraints();
}
return false;
```
(avoiding the double convert check).
Related details
- Free Debreuil
- Steve Kobes
- 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. |
Free Debreuil (Gerrit)
Free Debreuil voted and added 4 comments![]( "Open in Gerrit")
Votes added by Free Debreuil
| Auto-Submit | +1 |
4 comments
CHECK(walk == &scroll_container_layer || is_fixed_to_view);.nit scope so that `walk` doesn't leak into the rest of this long function.
```
bool is_fixed_to_view = false;
{
const PaintLayer* walk = Layer();
... //etc
}
```
Good point updated thank you.
if (!axes_to_clear || !rare_data_) {
return has_changed;
}```suggestion
if (!rare_data_) {
return false;
}
```(no need to short-circuit axes_to_clear)
Yes that that is better - updated thank you.
return To<LayoutBoxModelObject>(object).HasStickyConstraints();This could arguably be:
```
if (const auto* box_model = DynamicTo<LayoutBoxModelObject>(object)) {
return box_model->HasStickyConstraints();
}
return false;
```(avoiding the double convert check).
- Steve Kobes
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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/58138.
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
- Steve Kobes
- Vladimir Levin
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
74 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/paint/fragment_data.h
Insertions: 1, Deletions: 1.
@@ -85,7 +85,7 @@
AssertIsFirst();
bool has_changed = false;
- if (!axes_to_clear || !rare_data_) {
+ if (!rare_data_) {
return has_changed;
}
if ((axes_to_clear & kPhysicalAxesHorizontal) &&
```
```
The name of the file: third_party/blink/renderer/core/layout/layout_box_model_object.cc
Insertions: 10, Deletions: 8.
@@ -534,15 +534,17 @@
DCHECK(StyleRef().HasStickyConstrainedPosition());
bool is_fixed_to_view = false;
- // Walk up layout / paint layer tree to find if a fixed to view element exists
- // between this element and `scroll_container_layer`.
- const PaintLayer* walk = Layer();
- while (walk && walk != &scroll_container_layer && !is_fixed_to_view) {
- walk = walk->ContainingScrollContainerLayer(&is_fixed_to_view);
+ {
+ // Walk up layout / paint layer tree to find if a fixed to view element
+ // exists between this element and `scroll_container_layer`.
+ const PaintLayer* walk = Layer();
+ while (walk && walk != &scroll_container_layer && !is_fixed_to_view) {
+ walk = walk->ContainingScrollContainerLayer(&is_fixed_to_view);
+ }
+ // We should reach `scroll_container_layer` unless we hit a fixed to view
+ // ancestor (in which case we stop early).
+ CHECK(walk == &scroll_container_layer || is_fixed_to_view);
}
- // We should reach `scroll_container_layer` unless we hit a fixed to view
- // ancestor (in which case we stop early).
- CHECK(walk == &scroll_container_layer || is_fixed_to_view);
// Skip anonymous containing blocks except for anonymous fieldset content box.
LayoutBlock* sticky_container = StickyContainer();
```
```
The name of the file: third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
Insertions: 3, Deletions: 4.
@@ -539,11 +539,10 @@
}
static bool NeedsStickyTranslation(const LayoutObject& object) {
- if (!object.IsBoxModelObject()) {
- return false;
+ if (const auto* box_model = DynamicTo<LayoutBoxModelObject>(object)) {
+ return box_model->HasStickyConstraints();
}
-
- return To<LayoutBoxModelObject>(object).HasStickyConstraints();
+ return false;
}
static bool NeedsAnchorPositionScrollTranslation(const LayoutObject& object) {
```
Change information
[Blink] Implement single-axis scroll containers for sticky position
Adds support for independent horizontal and vertical sticky positioning
overflow behaviors. Elements can now stick on one axis while keeping the
other axis visible or clipped.
Introduces the `StickyConstraintsData` struct to process partial,
per-axis updates and merge constraints cleanly when the X and Y axes
track different scroll container parents. This feature is gated by the
`SingleAxisScrollContainersEnabled` runtime flag.
Also updates the compositor reason finder to safely drop elements back
to main-thread scrolling if multiple scroll containers are tracked, as
the compositor currently assumes a single scroll parent.
- M third_party/blink/renderer/core/animation/view_timeline.cc
- M third_party/blink/renderer/core/layout/fragment_builder.cc
- M third_party/blink/renderer/core/layout/geometry/axis.h
- M third_party/blink/renderer/core/layout/layout_box_model_object.cc
- M third_party/blink/renderer/core/layout/layout_box_model_object.h
- M third_party/blink/renderer/core/layout/layout_box_model_object_test.cc
- M third_party/blink/renderer/core/layout/physical_fragment_test.cc
- M third_party/blink/renderer/core/layout/svg/layout_svg_foreign_object.cc
- M third_party/blink/renderer/core/page/scrolling/sticky_position_scrolling_constraints.cc
- M third_party/blink/renderer/core/page/scrolling/sticky_position_scrolling_constraints.h
- M third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.cc
- M third_party/blink/renderer/core/paint/fragment_data.cc
- M third_party/blink/renderer/core/paint/fragment_data.h
- M third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
- M third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/external/wpt/css/css-position/sticky/position-sticky-single-axis-basic.html
- A third_party/blink/web_tests/external/wpt/css/css-position/sticky/position-sticky-single-axis-dynamic.html
- A third_party/blink/web_tests/external/wpt/css/css-position/sticky/position-sticky-single-axis-nested.html
- A third_party/blink/web_tests/virtual/single-axis-scroll-containers/README.md
Code-Review: +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/58138
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |