[layout] Reinsert LayoutObjects which undergo a in-flow state change. [chromium/src : main]
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 7 comments![]( "Open in Gerrit")
[layout] Reinsert LayoutObjects which undergo a positioned-state change.Ian KilpatrickAlso floats.
So "out-of-flow-state" would be more correct, although "out-of-flow" is often taken as either abspos and fixedpos. How about "in-flow state" then?
Maybe also reflect this in the runtime feature flag name, but I won't insist.
Done
[layout] Reinsert LayoutObjects which undergo a positioned-state change.Ian KilpatrickAlso floats.
So "out-of-flow-state" would be more correct, although "out-of-flow" is often taken as either abspos and fixedpos. How about "in-flow state" then?
Maybe also reflect this in the runtime feature flag name, but I won't insist.
Done
position:absolute).Ian Kilpatrickor float:left to float:none
Done
LayoutTreeBuilderTraversal::NextSiblingLayoutObject(*this));Ian KilpatrickThis can be more expensive than the current implementation in the presence of display:none and display:contents subtrees (assuming the current code only has to work on the box tree?).
The pathological cases are unlikely, though.
Yeah - I needed this as layout_object->NextSibling() may get destroyed now as part of the LayoutObject::Remove(), and detecting when this occurs is very error prone.
if (!FirstChild() && IsMergeableAnonymousBlock(this)) {Ian Kilpatricknit: Grammar/Clarity. "was created" -> "were created" (matching "we") or "it was created". "remove ourselves" -> "remove this object".
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
Acknowledged
SetScrollAnchorDisablingStyleChangedOnAncestor();Ian KilpatrickIf you happen to know why this (and the below) is necessary, please add a comment.
I see that this CL disables the corresponding code in StyleDidChange()...
But I wish we knew what this is about, and why it's still needed, now that the layout object is taken out and re-inserted.
Done
Related details
- Philip Rogers
- Rune Lillesveen
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
2 comments
LayoutTreeBuilderTraversal::NextSiblingLayoutObject(*this));Ian KilpatrickThis can be more expensive than the current implementation in the presence of display:none and display:contents subtrees (assuming the current code only has to work on the box tree?).
The pathological cases are unlikely, though.
Yeah - I needed this as layout_object->NextSibling() may get destroyed now as part of the LayoutObject::Remove(), and detecting when this occurs is very error prone.
So anonymous checks of current parent and sibling boxes are not enough?
I'm fine with leaving as is.
Related details
- Ian Kilpatrick
- Philip Rogers
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Commit-Queue | +2 |
2 comments
thanks for the review - lets see if it sticks!
LayoutTreeBuilderTraversal::NextSiblingLayoutObject(*this));Ian KilpatrickThis can be more expensive than the current implementation in the presence of display:none and display:contents subtrees (assuming the current code only has to work on the box tree?).
The pathological cases are unlikely, though.
Rune LillesveenYeah - I needed this as layout_object->NextSibling() may get destroyed now as part of the LayoutObject::Remove(), and detecting when this occurs is very error prone.
So anonymous checks of current parent and sibling boxes are not enough?
I'm fine with leaving as is.
So anonymous checks of current parent and sibling boxes are not enough?
It might be - but its complex. One example is:
```
<div>
<!-- anon -->
<div style="display: inline-block;"></div>
<div id="target" style="position: absolute;"></div>
<div style="position: absolute;"></div>
<!-- anon-end -->
<div></div>
</div>
<script>
document.body.offsetTop;
target.style.position = 'static';
</script>
```
will end up as:
```
<div>
<!-- anon -->
<div style="display: inline-block;"></div>
<!-- anon-end -->
<div id="target" style="position: static;"></div>
<div style="position: absolute;"></div>
<div></div>
</div>
e.g. before_child would remain the same, but has a different parent due to the removal.
I'd prefer to leave as is at the moment, then can optimize later with anon checks + using existing parent/before_child.
Related details
- Philip Rogers
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
[layout] Reinsert LayoutObjects which undergo a in-flow state change.
This patch reinserts layout-objects into the layout-tree when their
out-of-flow state changes. (E.g. a change from position:static to
position:absolute or float:left to float:none).
When their out-of-flow state changes we'll remove it from the
layout-object tree, update the style, then reinsert into the tree. This
is similar to layout-tree reattachment, except the layout-object (and
related objects/caches) are kept around.
This simplifies our logic when style changes in this way. Previously
we'd need to mutate the layout tree during LayoutObject::SetStyle which
was complex and error prone, and a source of bugs.
See removal patch for complexity we can remove:
https://chromium-review.googlesource.com/c/chromium/src/+/7263470
All behaviour changes should be behind the flag:
LayoutReinsertOnPositionStateChangeEnabled
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/layout/layout_block_flow.cc
- M third_party/blink/renderer/core/layout/layout_box.cc
- M third_party/blink/renderer/core/layout/layout_inline.cc
- 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/style/computed_style.cc
- M third_party/blink/renderer/core/style/computed_style.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/paint/invalidation/position/position-change-keeping-geometry-expected.txt
- M third_party/blink/web_tests/paint/invalidation/position/positioned-document-element-expected.txt
- M third_party/blink/web_tests/platform/mac/paint/invalidation/position/position-change-keeping-geometry-expected.txt
Code-Review: +1 by Morten Stenshorne, +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
luci-bi...@appspot.gserviceaccount.com has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bro (Gerrit)
Bro added 1 comment![]( "Open in Gerrit")
- Ian Kilpatrick
- Morten Stenshorne
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bro (Gerrit)
Bro added 1 comment![]( "Open in Gerrit")
Chrome
Chrome Crash (Prod) (Gerrit)
Chrome Crash (Prod) has created a revert of this change![]( "Open in Gerrit")
Related details
- Ian Kilpatrick
- Morten Stenshorne
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
LayoutReinsertOnPositionStateChangeEnabledWhen relanding, please change this to LayoutReinsertOnInFlowStateChange , which is the new name of the flag.
Related details
- Ian Kilpatrick
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |