if (OwningEffectNode() && OwningEffectNode()->render_surface_reason ==I think we added IsUnbounded to RenderSurfaceImpl in one of the previous patches (which also checks trees in viz)
return gfx::Rect(layer->bounds());I think this says if any of my ancestor effect nodes are unbounded, then *my* layer rect is the size of that unbounded ancestor.
I don't really know the consequences here, but presumably what we want to say is if any of effects that only apply to me cause me to be unbounded or something like that. I guess this is to prevent any of the layers from clipping, maybe it's ok.
if (node.id == kContentsRootPropertyNodeId) {Should this be before the render surface check? I'm not sure if root can be unbounded
builder.SetVisibility(EVisibility::kVisible);Why this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
GetDocument().UpdateStyleAndLayoutForNode(this,this reminds me of content-visibility: auto ancestors. Also reminds me of maybe requiring that the element is at least partially in the viewport like position anchor
if (auto* html_element = DynamicTo<HTMLElement>(GetNode());You probably want this to be stacked (updating IsStacked) since I don't think we want to support any element interleaving, which will give you the layer type for free
if (object.IsActiveUnboundedElementOrDescendant()) {Does the "OrDescendant" part mean this object is a descendant of an unbounded element? (another a bit more awkward reading is that an unbounded element is a descendant of this object)
for (const LayoutObject* curr = &layer.GetLayoutObject(); curr;This is fine but you can probably want the parents of the paint layer (PaintLayer::Parent) tree since unbounded will be a paint layer
context.inside_active_unbounded);Presumably this needs some invalidation when the ancestor is no longer unbounded since I think we can prune the tree walk... but I'm guessing we can just force the subtree update if the value changes. Not sure if that happens somewhere already
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
if (OwningEffectNode() && OwningEffectNode()->render_surface_reason ==I think we added IsUnbounded to RenderSurfaceImpl in one of the previous patches (which also checks trees in viz)
Good catch - done.
return gfx::Rect(layer->bounds());I think this says if any of my ancestor effect nodes are unbounded, then *my* layer rect is the size of that unbounded ancestor.
I don't really know the consequences here, but presumably what we want to say is if any of effects that only apply to me cause me to be unbounded or something like that. I guess this is to prevent any of the layers from clipping, maybe it's ok.
Yeah, I think you're right - this would break clipped contained content. I've tweaked this to remove the early return, and instead redirect the ancestor clip target to the unbounded elements effect node.
Should this be before the render surface check? I'm not sure if root can be unbounded
Done.
builder.SetVisibility(EVisibility::kVisible);Why this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
This counteracts the [UA stylesheet rule](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=981;drc=cea189ccafc8b0e6be51005148649525ad2cc9fb). I used visibility rather than `display:none` so that the layout information is already computed. I debated that though. But this seems like it'll be faster and might make transitions easier?
GetDocument().UpdateStyleAndLayoutForNode(this,this reminds me of content-visibility: auto ancestors. Also reminds me of maybe requiring that the element is at least partially in the viewport like position anchor
Help me understand what the first sentence is trying to tell me. You're worried what happens if the unbounded element sits in an as-yet unrendered content-visibility:auto tree? If so, yeah you're probably right that this is a problem. Ok for me to fix this as a followup?
On the second sentence, why do you think that should be a requirement? I could see use cases where the very bottom of an input is at the edge of the viewport, but you still want a picker below it (totally outside the viewport). Perhaps we should discuss this on the explainer - I've added an open question.
if (auto* html_element = DynamicTo<HTMLElement>(GetNode());You probably want this to be stacked (updating IsStacked) since I don't think we want to support any element interleaving, which will give you the layer type for free
Thanks for the pointer! Done.
if (object.IsActiveUnboundedElementOrDescendant()) {Does the "OrDescendant" part mean this object is a descendant of an unbounded element? (another a bit more awkward reading is that an unbounded element is a descendant of this object)
I renamed the method - LMK if that makes it more clear. I agree this was confusing.
for (const LayoutObject* curr = &layer.GetLayoutObject(); curr;This is fine but you can probably want the parents of the paint layer (PaintLayer::Parent) tree since unbounded will be a paint layer
Done
Presumably this needs some invalidation when the ancestor is no longer unbounded since I think we can prune the tree walk... but I'm guessing we can just force the subtree update if the value changes. Not sure if that happens somewhere already
See HTMLElement::SetUnboundedElementActive(), where I've changed to call SubtreePaintPropertyUpdateReason(). I think based on the docs for that, that it will do the right thing?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
builder.SetVisibility(EVisibility::kVisible);Mason FreedWhy this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
This counteracts the [UA stylesheet rule](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=981;drc=cea189ccafc8b0e6be51005148649525ad2cc9fb). I used visibility rather than `display:none` so that the layout information is already computed. I debated that though. But this seems like it'll be faster and might make transitions easier?
Drive-by:
Why is this done via StyleAdjuster?
Could this be a UA rule instead?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
+arthursonzogni for just unbounded_element_browsertest.cc
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
builder.SetVisibility(EVisibility::kVisible);Mason FreedWhy this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
Rune LillesveenThis counteracts the [UA stylesheet rule](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=981;drc=cea189ccafc8b0e6be51005148649525ad2cc9fb). I used visibility rather than `display:none` so that the layout information is already computed. I debated that though. But this seems like it'll be faster and might make transitions easier?
Drive-by:
Why is this done via StyleAdjuster?
Could this be a UA rule instead?
You mean via an internal pseudo class? I could do that, if you'd prefer it. (It just takes more work - that's the only reason this is here.)
builder.SetVisibility(EVisibility::kVisible);Mason FreedWhy this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
Rune LillesveenThis counteracts the [UA stylesheet rule](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=981;drc=cea189ccafc8b0e6be51005148649525ad2cc9fb). I used visibility rather than `display:none` so that the layout information is already computed. I debated that though. But this seems like it'll be faster and might make transitions easier?
Mason FreedDrive-by:
Why is this done via StyleAdjuster?
Could this be a UA rule instead?
You mean via an internal pseudo class? I could do that, if you'd prefer it. (It just takes more work - that's the only reason this is here.)
StyleAdjuster was never meant to be a convenient way to implement UA rules, and after sesse@ made it possible to cache ComputedStyles in the MPC with style adjustments applied, the exception you're adding to StyleAdjuster::GetElementTypeCacheKey() takes a slight performance hit. Nothing that in itself is noticeable, but papercuts ...
I won't block this CL, but UA rule should be considered.
if (auto* html_element = DynamicTo<HTMLElement>(element)) {Add a comment saying this is necessary because it does an element-dependent adjustment of visibility.
builder.SetVisibility(EVisibility::kVisible);Mason FreedWhy this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
Rune LillesveenThis counteracts the [UA stylesheet rule](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=981;drc=cea189ccafc8b0e6be51005148649525ad2cc9fb). I used visibility rather than `display:none` so that the layout information is already computed. I debated that though. But this seems like it'll be faster and might make transitions easier?
Mason FreedDrive-by:
Why is this done via StyleAdjuster?
Could this be a UA rule instead?
Rune LillesveenYou mean via an internal pseudo class? I could do that, if you'd prefer it. (It just takes more work - that's the only reason this is here.)
StyleAdjuster was never meant to be a convenient way to implement UA rules, and after sesse@ made it possible to cache ComputedStyles in the MPC with style adjustments applied, the exception you're adding to StyleAdjuster::GetElementTypeCacheKey() takes a slight performance hit. Nothing that in itself is noticeable, but papercuts ...
I won't block this CL, but UA rule should be considered.
Ok, fair enough! I'll switch this to a UA rule via an internal pseudo class. Thanks for the feedback! I'll comment again when done.
| Code-Review | +1 |
// Copyright 2026 The Chromium AuthorsLGTM, but I only read this test file. I will defer to @vmp...@chromium.org for the whole review.
content::WaitForHitTestData(primary_main_frame_host());nit: Omit content::
// TODO(crbug.com/508672616): Not yet implemented on Android.
#if BUILDFLAG(IS_ANDROID)
#define MAYBE_AncestorClipping DISABLED_AncestorClipping
#define MAYBE_InputEventRouting DISABLED_InputEventRouting
#define MAYBE_PopoverInsideUnbounded DISABLED_PopoverInsideUnbounded
#define MAYBE_CompositorPopupAllocation DISABLED_CompositorPopupAllocation
#define MAYBE_CompositorPopupAllocationHighDPI \
DISABLED_CompositorPopupAllocationHighDPI
#else
#define MAYBE_AncestorClipping AncestorClipping
#define MAYBE_InputEventRouting InputEventRouting
#define MAYBE_PopoverInsideUnbounded PopoverInsideUnbounded
#define MAYBE_CompositorPopupAllocation CompositorPopupAllocation
#define MAYBE_CompositorPopupAllocationHighDPI CompositorPopupAllocationHighDPI
#endif
Optional.
This could be in the test fixture constructor and/or setup function;
```
// TODO(crbug.com/508672616): Not yet implemented on Android.
if (BUILDFLAG(IS_ANDROID)) {
GTEST_SKIP();
}
```
(Or individual test cases if partially supported)
std::string setup_script = R"(
(async () => {
document.body.innerHTML = `
<div id="container" style="width:50px; height:50px; overflow:hidden; position:relative;">
<div id="child" style="width:100px; height:100px; position:absolute; top:0; left:0;" unbounded></div>
</div>
`;
const child = document.getElementById('child');
child.addEventListener('mousedown', () => { window.__clicked = true; });
await child.showUnboundedElement();
return true;
})();
)";nit:
https://paste.googleplex.com/5021859476275200?raw
(Sorry, I can't copy-paste due to Chrome DLP bugs, freezing my tab)
(async () => {
document.body.innerHTML = '<div id="child" style="width:100px; height:100px;" unbounded></div>';
const div = document.getElementById('child');
div.addEventListener('mousemove', (e) => {
window.__mouse_x = e.clientX;
window.__mouse_y = e.clientY;
});
await div.showUnboundedElement();
return true;
})();
)";ditto
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
LGTM, but I only read this test file. I will defer to @vmp...@chromium.org for the whole review.
Thanks! I will need another +1 due to file changes though.
content::WaitForHitTestData(primary_main_frame_host());Mason Freednit: Omit content::
Done
// TODO(crbug.com/508672616): Not yet implemented on Android.
#if BUILDFLAG(IS_ANDROID)
#define MAYBE_AncestorClipping DISABLED_AncestorClipping
#define MAYBE_InputEventRouting DISABLED_InputEventRouting
#define MAYBE_PopoverInsideUnbounded DISABLED_PopoverInsideUnbounded
#define MAYBE_CompositorPopupAllocation DISABLED_CompositorPopupAllocation
#define MAYBE_CompositorPopupAllocationHighDPI \
DISABLED_CompositorPopupAllocationHighDPI
#else
#define MAYBE_AncestorClipping AncestorClipping
#define MAYBE_InputEventRouting InputEventRouting
#define MAYBE_PopoverInsideUnbounded PopoverInsideUnbounded
#define MAYBE_CompositorPopupAllocation CompositorPopupAllocation
#define MAYBE_CompositorPopupAllocationHighDPI CompositorPopupAllocationHighDPI
#endif
Optional.
This could be in the test fixture constructor and/or setup function;```
// TODO(crbug.com/508672616): Not yet implemented on Android.
if (BUILDFLAG(IS_ANDROID)) {
GTEST_SKIP();
}
```
(Or individual test cases if partially supported)
So much better! Thanks! Done.
std::string setup_script = R"(
(async () => {
document.body.innerHTML = `
<div id="container" style="width:50px; height:50px; overflow:hidden; position:relative;">
<div id="child" style="width:100px; height:100px; position:absolute; top:0; left:0;" unbounded></div>
</div>
`;
const child = document.getElementById('child');
child.addEventListener('mousedown', () => { window.__clicked = true; });
await child.showUnboundedElement();
return true;
})();
)";nit:
- Wrap to fit 80 columns.
- EvalJs automatically wait if the expression evaluates to a promise.
https://paste.googleplex.com/5021859476275200?raw(Sorry, I can't copy-paste due to Chrome DLP bugs, freezing my tab)
Ahh nice - thanks!
(async () => {
document.body.innerHTML = '<div id="child" style="width:100px; height:100px;" unbounded></div>';
const div = document.getElementById('child');
div.addEventListener('mousemove', (e) => {
window.__mouse_x = e.clientX;
window.__mouse_y = e.clientY;
});
await div.showUnboundedElement();
return true;
})();
)";Mason Freedditto
Done
builder.SetVisibility(EVisibility::kVisible);Mason FreedWhy this change? I'm not sure how that's different from something like display: none in the ancestor or opacity 0 or something
Rune LillesveenThis counteracts the [UA stylesheet rule](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/resources/html.css;l=981;drc=cea189ccafc8b0e6be51005148649525ad2cc9fb). I used visibility rather than `display:none` so that the layout information is already computed. I debated that though. But this seems like it'll be faster and might make transitions easier?
Mason FreedDrive-by:
Why is this done via StyleAdjuster?
Could this be a UA rule instead?
Rune LillesveenYou mean via an internal pseudo class? I could do that, if you'd prefer it. (It just takes more work - that's the only reason this is here.)
Mason FreedStyleAdjuster was never meant to be a convenient way to implement UA rules, and after sesse@ made it possible to cache ComputedStyles in the MPC with style adjustments applied, the exception you're adding to StyleAdjuster::GetElementTypeCacheKey() takes a slight performance hit. Nothing that in itself is noticeable, but papercuts ...
I won't block this CL, but UA rule should be considered.
Ok, fair enough! I'll switch this to a UA rule via an internal pseudo class. Thanks for the feedback! I'll comment again when done.
Done
if (auto* html_element = DynamicTo<HTMLElement>(element)) {Add a comment saying this is necessary because it does an element-dependent adjustment of visibility.
Turns out we don't need this at all, with the pseudo class.
GetDocument().UpdateStyleAndLayoutForNode(this,Mason Freedthis reminds me of content-visibility: auto ancestors. Also reminds me of maybe requiring that the element is at least partially in the viewport like position anchor
Help me understand what the first sentence is trying to tell me. You're worried what happens if the unbounded element sits in an as-yet unrendered content-visibility:auto tree? If so, yeah you're probably right that this is a problem. Ok for me to fix this as a followup?
On the second sentence, why do you think that should be a requirement? I could see use cases where the very bottom of an input is at the edge of the viewport, but you still want a picker below it (totally outside the viewport). Perhaps we should discuss this on the explainer - I've added an open question.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
// TODO(crbug.com/508672616): Not yet implemented on Android.
#if BUILDFLAG(IS_ANDROID)
#define MAYBE_AncestorClipping DISABLED_AncestorClipping
#define MAYBE_InputEventRouting DISABLED_InputEventRouting
#define MAYBE_PopoverInsideUnbounded DISABLED_PopoverInsideUnbounded
#define MAYBE_CompositorPopupAllocation DISABLED_CompositorPopupAllocation
#define MAYBE_CompositorPopupAllocationHighDPI \
DISABLED_CompositorPopupAllocationHighDPI
#else
#define MAYBE_AncestorClipping AncestorClipping
#define MAYBE_InputEventRouting InputEventRouting
#define MAYBE_PopoverInsideUnbounded PopoverInsideUnbounded
#define MAYBE_CompositorPopupAllocation CompositorPopupAllocation
#define MAYBE_CompositorPopupAllocationHighDPI CompositorPopupAllocationHighDPI
#endif
Mason FreedOptional.
This could be in the test fixture constructor and/or setup function;```
// TODO(crbug.com/508672616): Not yet implemented on Android.
if (BUILDFLAG(IS_ANDROID)) {
GTEST_SKIP();
}
```
(Or individual test cases if partially supported)
So much better! Thanks! Done.
| Code-Review | +1 |
lgtm, with some minor nits. Generally, I only commented in one spot to leave a comment but some of the more subtle hunks could also use comments optionally
if (IsUnbounded()) {minor nit, just add another conjunct below?
GetDocument().UpdateStyleAndLayoutForNode(this,Mason Freedthis reminds me of content-visibility: auto ancestors. Also reminds me of maybe requiring that the element is at least partially in the viewport like position anchor
Mason FreedHelp me understand what the first sentence is trying to tell me. You're worried what happens if the unbounded element sits in an as-yet unrendered content-visibility:auto tree? If so, yeah you're probably right that this is a problem. Ok for me to fix this as a followup?
On the second sentence, why do you think that should be a requirement? I could see use cases where the very bottom of an input is at the edge of the viewport, but you still want a picker below it (totally outside the viewport). Perhaps we should discuss this on the explainer - I've added an open question.
I'm going to fix this in a followup.
None of these were meant to be requirements. Follow up is fine :)
the content visibility comment is that these types of calls are usually also need changes in display lock context to ensure that we keep things unlocked while it's unbounded or something along those lines. consider a context menu in a content-visibility: auto element that is offscreen. The offscreenness of the element means that we don't update rendering, but unbounded is likely to escape strict containment and paint on screen anyway (like a top layer element). This should mean that we keep the content visibility auto unlocked for the duration of unboundedness, similar to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/display_lock/display_lock_context.cc;l=1147-1167;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666
which notifies if there is a top layer element in the subtree, which subsequently causes the context to stay unlocked:
bool IsInclusiveDescendantOfUnboundedElement() const {Thanks for the rename, this is much clearer imho
if (auto* html_element = DynamicTo<HTMLElement>(object_.GetNode());minor nit: comment
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
lgtm, with some minor nits. Generally, I only commented in one spot to leave a comment but some of the more subtle hunks could also use comments optionally
Thanks. I added more comments where it looks like they would help. Feel free to point out any that I missed and I'll add comments as a followup.
minor nit, just add another conjunct below?
That's not minor, I totally agree. Done.
GetDocument().UpdateStyleAndLayoutForNode(this,Mason Freedthis reminds me of content-visibility: auto ancestors. Also reminds me of maybe requiring that the element is at least partially in the viewport like position anchor
Mason FreedHelp me understand what the first sentence is trying to tell me. You're worried what happens if the unbounded element sits in an as-yet unrendered content-visibility:auto tree? If so, yeah you're probably right that this is a problem. Ok for me to fix this as a followup?
On the second sentence, why do you think that should be a requirement? I could see use cases where the very bottom of an input is at the edge of the viewport, but you still want a picker below it (totally outside the viewport). Perhaps we should discuss this on the explainer - I've added an open question.
Vladimir LevinI'm going to fix this in a followup.
None of these were meant to be requirements. Follow up is fine :)
the content visibility comment is that these types of calls are usually also need changes in display lock context to ensure that we keep things unlocked while it's unbounded or something along those lines. consider a context menu in a content-visibility: auto element that is offscreen. The offscreenness of the element means that we don't update rendering, but unbounded is likely to escape strict containment and paint on screen anyway (like a top layer element). This should mean that we keep the content visibility auto unlocked for the duration of unboundedness, similar to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/display_lock/display_lock_context.cc;l=1147-1167;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666
which notifies if there is a top layer element in the subtree, which subsequently causes the context to stay unlocked:
Thanks for this! Will address as a followup.
bool IsInclusiveDescendantOfUnboundedElement() const {Thanks for the rename, this is much clearer imho
😊
if (auto* html_element = DynamicTo<HTMLElement>(object_.GetNode());Mason Freedminor nit: comment
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
30 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/paint_property_tree_builder.cc
Insertions: 2, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/paint/paint_layer_clipper.cc
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/layout/layout_object.h
Insertions: 2, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/paint/paint_layer_painter.cc
Insertions: 2, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: cc/layers/render_surface_impl.cc
Insertions: 1, Deletions: 4.
The diff is too large to show. Please review the diff.
```
Unbounded Element: Clipping bypass and visibility [8/N]
This change implements the core rendering bypass for active unbounded
elements and their descendants. It introduces a bitfield on LayoutObject
to track the unbounded hierarchy and overrides ancestor clips during
paint property tree construction. Visibility is managed via
ComputedStyle adjustment during style resolution.
In the paint property tree, active unbounded elements are attached
directly to the root clip node. Background clip rect calculations
are updated to use local border box properties, bypassing ancestor
clips. Additionally, CullRect::Infinite() is forced during painting
to prevent culling. The compositor is updated to skip clipping on
RenderSurfaceImpl and LayerVisibleRect for unbounded elements.
Finally, active elements are deactivated when the unbounded attribute
is removed, and browser tests are added to verify clipping bypass,
input event routing, and dynamic bounds synchronization.
See go/unbounded-element for the design doc.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |