Unbounded Element: Include ink overflow in window bounds calculation [chromium/src : main]
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
I'm gonna route this one to pdr. I don't know if there's a reliable way to get the ink overflow rect in blink that precisely matches what cc would produce. View transitions does a roundabout way where we use a placeholder rect and then a signal back from cc indicating the precise rect
FWIW I also don't know if we want a precise rect here -- what would be the behavior if we are too small or too large on our calculation?
Related details
- Mason Freed
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
I'm gonna route this one to pdr. I don't know if there's a reliable way to get the ink overflow rect in blink that precisely matches what cc would produce. View transitions does a roundabout way where we use a placeholder rect and then a signal back from cc indicating the precise rect
FWIW I also don't know if we want a precise rect here -- what would be the behavior if we are too small or too large on our calculation?
Yeah, if there's a better way to do this, that'd be great! I do think it's "ok" to be too large here, since the background is transparent anyway. (I have a future patch that just expands the unbounded window size to the union of old/new sizes, to speed up window moves. That should prove the point, once it works.)
Related details
- Philip Rogers
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 7 comments![]( "Open in Gerrit")
}We shouldn't continue if the lifecycle isn't updated, as everything below is stale and potentially dangerous to touch.
gfx::Rect AbsoluteBoundingBoxRectIncludingInkOverflow(No need to have or pass a MapCoordinatesFlag.
// Returns the absolute bounding box rect including ink overflow (such as CSSI'm a little reluctant to add this easy-to-use API because of the issues below. Maybe this can be made more unbounded specific?
LocalToAbsoluteRect(overflow, flags | kTraverseDocumentBoundaries));Do we need LocalToAncestorVisualRect? E.g.:
```
<!doctype html>
<div style="filter: blur(10px);">
<div unbounded>
</div>
```
return AbsoluteBoundingBoxRect(flags);Why do we use kTraverseDocumentBoundaries above but not here?
object.AbsoluteBoundingBoxRectIncludingInkOverflow();AbsoluteBoundingBoxRectIncludingInkOverflow uses kTraverseDocumentBoundaries so we need to ensure this re-runs when ancestors change, even crossing frame boundaries. This seems hard.
There can also be affected by transform animations and scrolls above and below the unbounded element. This also seems hard.
Do we need a different approach here? For example, maybe we should tag the unbounded element's painting, and then handle all of this on the compositor thread (or later). Or maybe we need something like intersection observer which updates out-of-band.
<!DOCTYPE html>Can you use wpt_internal for these tests, since this may be a real API in the future? You can use a reference of two green divs rather than a png file.
Related details
- 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. |
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
We shouldn't continue if the lifecycle isn't updated, as everything below is stale and potentially dangerous to touch.
Done, good catch.
gfx::Rect AbsoluteBoundingBoxRectIncludingInkOverflow(No need to have or pass a MapCoordinatesFlag.
Done
// Returns the absolute bounding box rect including ink overflow (such as CSSI'm a little reluctant to add this easy-to-use API because of the issues below. Maybe this can be made more unbounded specific?
Done - it now has "unbounded" in the name and comment, and DCHECKs that the feature is enabled.
LocalToAbsoluteRect(overflow, flags | kTraverseDocumentBoundaries));Do we need LocalToAncestorVisualRect? E.g.:
```
<!doctype html>
<div style="filter: blur(10px);">
<div unbounded>
</div>
```
So at the moment, that unbounded element will escape that blur or a similar transform. (In LayerTreeHostImpl, the render pass for the unbounded element gets added to the unbounded_render_passes, which isn't connected to the parent's target, so it isn't affected by the parent effects.) I'm still wrestling with the best behavior here. I've been modeling this somewhat after top layer elements, which escape everything. That's the easiest implementation, but it's still an open question.
As for `LocalToAncestorVisualRect`, doesn't that walk up the tree and collect clips and expansions due to ancestor effects? If so, we don't want that, at least for now, since we're escaping those.
Why do we use kTraverseDocumentBoundaries above but not here?
I've removed kTraverseDocumentBoundaries completely, since I've rebased this patch onto the iframe one, which uses view->FrameToViewport(bounds).
object.AbsoluteBoundingBoxRectIncludingInkOverflow();AbsoluteBoundingBoxRectIncludingInkOverflow uses kTraverseDocumentBoundaries so we need to ensure this re-runs when ancestors change, even crossing frame boundaries. This seems hard.
There can also be affected by transform animations and scrolls above and below the unbounded element. This also seems hard.
Do we need a different approach here? For example, maybe we should tag the unbounded element's painting, and then handle all of this on the compositor thread (or later). Or maybe we need something like intersection observer which updates out-of-band.
Ohhh. This is a good point, and you're right it's not handled now.
The `kTraverseDocumentBoundaries` part is now gone, since we handle this via `view->FrameToViewport(bounds)`. (I rebased this patch onto the iframe one.) But the part about compositor scroll effects not invalidating this still exists. And probably compositor-driven transform animations don't update the unbounded element position.
Here's my proposed approach:
- go with the current approach in this CL, to get the basic (static) clip expansion behavior working. That's breaking some webium use cases now.
- add a simple test case now for this issue, and mark it failing.
- file a bug to fix it, with some suggested fixes.
- fix that bug as a followup.
{Edit: I'm actually unable to come up with a failing test reliably. I'm going to use crbug.com/524931672 for this, since it should catch any issues.}
Sound ok to you?
Related details
- Philip Rogers
- 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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Anything I can do to help with the review here?
Related details
- Philip Rogers
- 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. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 2 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
2 comments
object.AbsoluteBoundingBoxRectIncludingInkOverflow();Mason FreedAbsoluteBoundingBoxRectIncludingInkOverflow uses kTraverseDocumentBoundaries so we need to ensure this re-runs when ancestors change, even crossing frame boundaries. This seems hard.
There can also be affected by transform animations and scrolls above and below the unbounded element. This also seems hard.
Do we need a different approach here? For example, maybe we should tag the unbounded element's painting, and then handle all of this on the compositor thread (or later). Or maybe we need something like intersection observer which updates out-of-band.
Ohhh. This is a good point, and you're right it's not handled now.
The `kTraverseDocumentBoundaries` part is now gone, since we handle this via `view->FrameToViewport(bounds)`. (I rebased this patch onto the iframe one.) But the part about compositor scroll effects not invalidating this still exists. And probably compositor-driven transform animations don't update the unbounded element position.
Here's my proposed approach:
- go with the current approach in this CL, to get the basic (static) clip expansion behavior working. That's breaking some webium use cases now.
- add a simple test case now for this issue, and mark it failing.
- file a bug to fix it, with some suggested fixes.
- fix that bug as a followup.
{Edit: I'm actually unable to come up with a failing test reliably. I'm going to use crbug.com/524931672 for this, since it should catch any issues.}
Sound ok to you?
- Mason Freed
- 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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
+arthursonzogni for just this:
content/browser/renderer_host/unbounded_element_browsertest.cc
Related details
- Arthur Sonzogni
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
Message
**[Early Review]** This is an automated early review generated by an LLM. It is intended to help you catch obvious issues early and **potentially save a round of code review**.
If you find any suggestion irrelevant, please feel free to *ignore* or *close* it.
_I am going to do a manual code review when I wake up._
Please see suggestions below.
2 comments
**[Early Review]**
LocalFrameView::UpdateAllLifecyclePhasesExceptPaint returns void, so it cannot be used in a boolean expression. This will cause a compilation error. You should call the method on a non-null view and handle any post-update checks (such as verifying the document is still active) separately.
return ToEnclosingRect(LocalToAbsoluteRect(overflow));**[Early Review]**
ToEnclosingRect is in the gfx namespace. Since there is no using declaration for it in this file (as seen by the use of gfx::ToEnclosingRect on line 1965), calling it without the gfx:: prefix will cause a compilation error. Use gfx::ToEnclosingRect instead.
Related details
- Arthur Sonzogni
- Mason Freed
- 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. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
**[Early Review]**
LocalFrameView::UpdateAllLifecyclePhasesExceptPaint returns void, so it cannot be used in a boolean expression. This will cause a compilation error. You should call the method on a non-null view and handle any post-update checks (such as verifying the document is still active) separately.
Uhh - the AI is smoking something.
```
bool LocalFrameView::UpdateAllLifecyclePhasesExceptPaint(
DocumentUpdateReason reason) {
return GetFrame().LocalFrameRoot().View()->UpdateLifecyclePhases(
DocumentLifecycle::kPrePaintClean, reason);
}
```
return ToEnclosingRect(LocalToAbsoluteRect(overflow));**[Early Review]**
ToEnclosingRect is in the gfx namespace. Since there is no using declaration for it in this file (as seen by the use of gfx::ToEnclosingRect on line 1965), calling it without the gfx:: prefix will cause a compilation error. Use gfx::ToEnclosingRect instead.
It seems odd to point out compilation errors, since, you know, the bots are green. That, and there's a call to gfx::ToEnclosingRect just 2 lines above in the existing code.
Related details
- Arthur Sonzogni
- 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. |