[anchor] Keep anchored elements within their fragmentation context. [chromium/src : main]
0 views
Skip to first unread message
AI Code Reviewer (Gerrit)
Aug 27, 2025, 1:58:47 AM (14 days ago) Aug 27
to Morten Stenshorne, Ian Kilpatrick, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Morten Stenshorne
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/layout/fragment_builder.h
Line 207, Patchset 27 (Latest):
static void PropagateChildAnchors(const PhysicalFragment& child,AI Code Reviewer . unresolved
Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. Please consider omitting parameter names like `child`, `child_offset`, `container_object`, etc., from the function declaration in the header file as their purpose is clear from their types.
***
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
File third_party/blink/renderer/core/layout/out_of_flow_layout_part.h
Line 346, Patchset 27 (Latest):
OffsetInfo CalculateOffset(AI Code Reviewer . unresolved
Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The boolean parameter `is_inside_fragmentation_context` makes call sites less readable. Please consider using an enum class like `enum class FragmentationContextStatus { kInside, kOutside };`.
***
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Attention is currently required from:
- Morten Stenshorne
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Aug 27, 2025, 3:10:09 AM (14 days ago) Aug 27
to Rune Lillesveen, AI Code Reviewer, Ian Kilpatrick, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Ian Kilpatrick and Rune Lillesveen
Morten Stenshorne added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 29 (Latest):
Morten Stenshorne . resolved
Need another stamp because I messed up TestExpectations, and now cleaned it up.
File third_party/blink/renderer/core/layout/fragment_builder.h
Line 207, Patchset 27:
static void PropagateChildAnchors(const PhysicalFragment& child,AI Code Reviewer . resolved
Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. Please consider omitting parameter names like `child`, `child_offset`, `container_object`, etc., from the function declaration in the header file as their purpose is clear from their types.
***_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Morten Stenshorne
What makes them obvious?
File third_party/blink/renderer/core/layout/out_of_flow_layout_part.h
Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. The boolean parameter `is_inside_fragmentation_context` makes call sites less readable. Please consider using an enum class like `enum class FragmentationContextStatus { kInside, kOutside };`.
***_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Attention is currently required from:
- Ian Kilpatrick
- Rune Lillesveen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Aug 27, 2025, 3:15:58 AM (14 days ago) Aug 27
to Morten Stenshorne, Rune Lillesveen, AI Code Reviewer, Ian Kilpatrick, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Ian Kilpatrick and Morten Stenshorne
Rune Lillesveen voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Ian Kilpatrick
- Morten Stenshorne
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Aug 27, 2025, 3:21:36 AM (14 days ago) Aug 27
to Rune Lillesveen, AI Code Reviewer, Ian Kilpatrick, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Ian Kilpatrick
Morten Stenshorne voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Ian Kilpatrick
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Aug 27, 2025, 4:45:50 AM (14 days ago) Aug 27
to Morten Stenshorne, Rune Lillesveen, AI Code Reviewer, Ian Kilpatrick, AyeAye, chromium...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[anchor] Keep anchored elements within their fragmentation context.
Don't use `StitchedAnchorQueries`, or anything at all in
anchor_query_map.cc
If the containing block of an out-of-flow positioned element
participates in a fragmentation context, so does the out-of-flow
positioned element. This was violated for anchor-positioned elements. In
such cases we'd essentially let them "escape" the fragmentation context,
take a peek on the outside, to calculate the bounding box of the anchor,
and then jump back into the fragmentation context. This caused undesired
behavior, made it hard to reason about, and also caused implementation
complexity.
Anchor-positioned elements use the `anchor()` function with inset
properties, such as `top`, `right`, `bottom`, and `left`. These are
relative to the containing block for out-of-flow positioned elements,
and anchor-positioned elements should be no different. Therefore, if the
containing block is fragmented, we're in this "stitched" coordinate
system, where we imagine that all fragments have been stacked on top of
each other. This is also the case for sizing properties, such as
`block-size`. `<div style="columns:2;"><div style="block-size:100px;">`,
for instance, will not create a 100px tall child, visually, since it
will be fragmented into two columns, with 50px of content in each. Yet,
it will be 100px tall if we imagine the fragments to be "stitched"
together. If we now imagine that the `block-size:100px` DIV is
relatively positioned, and put a `position:absolute` child inside with
`top:70px`, it will not start 70px into the first column (its containing
block is fragmented). Nay, it will start 20px into the second column,
skipping the 50px of the first column, then skip the remaining 20px in
the second column. This hopefully explains the weirdness with the
previous behavior.
The fix in PhysicalBoxFragment::MutableForOofFragmentation::Merge()
should have been there in the first place, since we need to handle
anchors with the same name correctly. Previously, we'd be saved by the
duplicate logic in StitchedAnchorQuery::AddAnchorReference(), which
would clean up after this mistake. Now we need to get it right the first
time. anchor-position-multicol-012.html (and some existing, rather
convoluted, tests) would fail without this.
Update tests to match the new behavior. Changed them to reftests, since
offsetLeft & co (used by check-layout-th.js) have poor interop inside
multicol. See https://github.com/w3c/csswg-drafts/issues/11541
This change is behind a runtime enabled feature kill-switch named
CSSAnchorSimplifiedFragmentation.
See https://github.com/w3c/csswg-drafts/issues/12287 for the spec
discussion, and resolution.
Bug: 436305267
Change-Id: I4872d35a040062e7dbccaf736b9eda3b8661b880
Commit-Queue: Morten Stenshorne <mste...@chromium.org>
Reviewed-by: Rune Lillesveen <fut...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1506964}
Files:
- M third_party/blink/renderer/core/layout/anchor_evaluator_impl.cc
- M third_party/blink/renderer/core/layout/anchor_evaluator_impl.h
- M third_party/blink/renderer/core/layout/anchor_query_map.cc
- M third_party/blink/renderer/core/layout/fragment_builder.cc
- M third_party/blink/renderer/core/layout/fragment_builder.h
- M third_party/blink/renderer/core/layout/layout_object.cc
- M third_party/blink/renderer/core/layout/layout_object.h
- M third_party/blink/renderer/core/layout/out_of_flow_layout_part.cc
- M third_party/blink/renderer/core/layout/out_of_flow_layout_part.h
- M third_party/blink/renderer/core/layout/physical_box_fragment.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- D third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-getComputedStyle-002-expected.txt
- D third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-grid-001-expected.txt
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-grid-001-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-grid-001.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-inline-004-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-inline-004.html
- M third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-007.html
- M third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-008.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-012.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-013.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-014.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-015.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-016.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-017.html
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-nested-001-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-anchor-position/anchor-position-multicol-nested-001.html
- M third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-anchor-multicol-display.tentative.html
- M third_party/blink/web_tests/virtual/html-anchor-attribute-disabled/external/wpt/html/semantics/popovers/popover-anchor-multicol-display.tentative-expected.txt
Change size: L
Delta: 29 files changed, 687 insertions(+), 306 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages