[APC] Align outer bounding box mapping with visible rect [chromium/src : main]
Aaron Leventhal (Gerrit)
Aaron Leventhal voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andrew Verge
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 5 comments![]( "Open in Gerrit")
This is great, thank you!
+pdr/szager, wasn't sure who's more familiar with GeometryMapper so bugging you both. :)
bool apply_overflow_clip = true,
bool apply_viewport_transform = false,
bool apply_viewport_clip = true);nit: This is getting into enum bitflags territory. :)
does_intersect = true;I glanced through the call-sites to confirm this should be the return value. One of the comments says that this API returns true if no clipping was applied so this seems reasonable to me. But leaving a comment so a more familiar owner can validate.
// Normalize to a (0,0) origin in the outer document's viewport space so it
// matches the coordinate system returned by MapToVisualRectInAncestorSpace().This won't suffice for OOPIFs since `MapToVisualRectInAncestorSpace` does mapping into the remote frame's viewport space. But IIUC this will be the OOPIF's viewport's coordinate space.
Also looks like we can add this check in a separate CL? Keep this one focused on validating the relationship between outer_bounding_box and visible_bounding_box.
// rect, ensuring both passes share identical transform logic.nit: transform and scroll offset logic
Related details
- Aaron Leventhal
- Andrew Verge
- Philip Rogers
- Stefan Zager
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 added 1 comment![]( "Open in Gerrit")
still respecting ancestor transforms, scroll offsets, and filters.Mapping while respecting transforms and scroll offsets, but without clipping, sounds like GeometryMapper::SourceToDestinationProjection (see ObjectToViewTransform for an example of this in use). Can you just use SourceToDestinationProjection?
Related details
- Aaron Leventhal
- Andrew Verge
- Stefan Zager
Andrew Verge (Gerrit)
Andrew Verge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Verge
| Code-Review | +1 |
1 comment
I am pretty new to this space so thanks for letting me review this! Overall +1, but deferring to other domain experts for approval.
Related details
- Aaron Leventhal
- Stefan Zager
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal removed Stefan Zager from this change![]( "Open in Gerrit")
- Stefan Zager
Related details
- Aaron Leventhal
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 4 comments![]( "Open in Gerrit")
still respecting ancestor transforms, scroll offsets, and filters.Mapping while respecting transforms and scroll offsets, but without clipping, sounds like GeometryMapper::SourceToDestinationProjection (see ObjectToViewTransform for an example of this in use). Can you just use SourceToDestinationProjection?
Hi pdr, unfortunately, tons of tests fail our DCHECKs with that approach. See
https://chromium-review.googlesource.com/c/chromium/src/+/7214166?tab=checks
Maybe you have another suggestion? What I like about the approach here, although it does add some complexity to layout, is that it's clearly following the same code path and just not applying clipping. That means there won't be any surprises from divergent math.
bool apply_overflow_clip = true,
bool apply_viewport_transform = false,
bool apply_viewport_clip = true);nit: This is getting into enum bitflags territory. :)
I put this part of the change in https://chromium-review.googlesource.com/c/chromium/src/+/7213198
Note that it adds 35 lines of code. Not sure whether it's worth it.
// Normalize to a (0,0) origin in the outer document's viewport space so it
// matches the coordinate system returned by MapToVisualRectInAncestorSpace().This won't suffice for OOPIFs since `MapToVisualRectInAncestorSpace` does mapping into the remote frame's viewport space. But IIUC this will be the OOPIF's viewport's coordinate space.
Also looks like we can add this check in a separate CL? Keep this one focused on validating the relationship between outer_bounding_box and visible_bounding_box.
Moved to separate CL: https://chromium-review.googlesource.com/c/chromium/src/+/7213442
// rect, ensuring both passes share identical transform logic.nit: transform and scroll offset logic
- Khushal Sagar
- 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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
still respecting ancestor transforms, scroll offsets, and filters.Aaron LeventhalMapping while respecting transforms and scroll offsets, but without clipping, sounds like GeometryMapper::SourceToDestinationProjection (see ObjectToViewTransform for an example of this in use). Can you just use SourceToDestinationProjection?
Hi pdr, unfortunately, tons of tests fail our DCHECKs with that approach. See
https://chromium-review.googlesource.com/c/chromium/src/+/7214166?tab=checksMaybe you have another suggestion? What I like about the approach here, although it does add some complexity to layout, is that it's clearly following the same code path and just not applying clipping. That means there won't be any surprises from divergent math.
I'm sorry but I tried to get this test to run but couldn't get it to. I may have made a mistake fixing the merge conflicts.
I'd like to understand the difference between this patch and SourceToDestinationProjection. What is being handled in one but not the other, etc.
Can you describe how the result from SourceToDestinationProjection differs from the result here? For example, https://chromium-review.googlesource.com/c/chromium/src/+/7214166 says that virtual/content-extraction/compositing/geometry/layer-due-to-layer-children-deep.html fails with:
```
DCHECK failed: outer_box_in_viewport_coords.Contains(visible_box_in_viewport_coords). Visible box must lie within outer box. Visible: 31,43 300x294, Outer: 31,43 289x218
STDERR: For: 0x10400521560:LayoutBlockFlow (relative positioned, children-inline) DIV id="greatgrandchild"
```
Can you print Outer with this patch? I'm curious what the difference is.
There is a small difference between https://chromium-review.googlesource.com/c/chromium/src/+/7214166 and ObjectToViewTransform in the non-GeometryMapper codepath, where the patch does:
```
object.MapLocalToAncestor(nullptr, transform_state, 0);
transform_state.Flatten();
unclipped_box = transform_state.LastPlanarQuad().BoundingBox();
```
But ObjectToViewTransform does:
```
gfx::Transform target_to_view_transform = ObjectToViewTransform(*target);
target_rect_ = target_to_view_transform.MapRect(target_rect_);
```
I'm not sure if this is relevant or not.
Related details
- Aaron Leventhal
- Khushal Sagar
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
still respecting ancestor transforms, scroll offsets, and filters.Aaron LeventhalMapping while respecting transforms and scroll offsets, but without clipping, sounds like GeometryMapper::SourceToDestinationProjection (see ObjectToViewTransform for an example of this in use). Can you just use SourceToDestinationProjection?
Philip RogersHi pdr, unfortunately, tons of tests fail our DCHECKs with that approach. See
https://chromium-review.googlesource.com/c/chromium/src/+/7214166?tab=checksMaybe you have another suggestion? What I like about the approach here, although it does add some complexity to layout, is that it's clearly following the same code path and just not applying clipping. That means there won't be any surprises from divergent math.
I'm sorry but I tried to get this test to run but couldn't get it to. I may have made a mistake fixing the merge conflicts.
I'd like to understand the difference between this patch and SourceToDestinationProjection. What is being handled in one but not the other, etc.
Can you describe how the result from SourceToDestinationProjection differs from the result here? For example, https://chromium-review.googlesource.com/c/chromium/src/+/7214166 says that virtual/content-extraction/compositing/geometry/layer-due-to-layer-children-deep.html fails with:
```
DCHECK failed: outer_box_in_viewport_coords.Contains(visible_box_in_viewport_coords). Visible box must lie within outer box. Visible: 31,43 300x294, Outer: 31,43 289x218
STDERR: For: 0x10400521560:LayoutBlockFlow (relative positioned, children-inline) DIV id="greatgrandchild"
```Can you print Outer with this patch? I'm curious what the difference is.
There is a small difference between https://chromium-review.googlesource.com/c/chromium/src/+/7214166 and ObjectToViewTransform in the non-GeometryMapper codepath, where the patch does:
```
object.MapLocalToAncestor(nullptr, transform_state, 0);
transform_state.Flatten();
unclipped_box = transform_state.LastPlanarQuad().BoundingBox();
```But ObjectToViewTransform does:
```
gfx::Transform target_to_view_transform = ObjectToViewTransform(*target);
target_rect_ = target_to_view_transform.MapRect(target_rect_);
```
I'm not sure if this is relevant or not.
You'll need to update your source to run the tests as I just landed it.
I've written up a comparison of outer bounding boxes here:
https://docs.google.com/document/d/1sVSTn_4BPGXafXm-MhBABbNngfcFeu3vNfGXbLv63CI/edit?tab=t.0
Thanks for telling me about the missing MapRect() call. I've fixed that, but it did not help. In any case it's more similar to the previous code, so I updated the CL with that change.
Related details
- Khushal Sagar
- Philip Rogers
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
nullptr, unclipped, kSkipAncestorAndViewportClips));Can you also check the result when using geometry mapper?
```
unclipped = local_rect;
EXPECT_TRUE(target->MapToVisualRectInAncestorSpace(
&GetLayoutView(), unclipped, static_cast<VisualRectFlags>(kSkipAncestorAndViewportClips | kUseGeometryMapper)));
EXPECT_EQ(unclipped, PhysicalRect(-30, -35, 40, 40));
```
(similarly for the others)
Related details
- Aaron Leventhal
- Andrew Verge
- Khushal Sagar
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal restored this change![]( "Open in Gerrit")
Related details
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 2 comments![]( "Open in Gerrit")
I tried switching outer box mapping to GeometryMapper::SourceToDestinationProjection following the ObjectToViewTransform pattern (GeometryMapper fast-path when possible, otherwise fallback) in CL:7214166. Unfortunately it still triggers the outer.Contains(visible) DCHECK in a number of tests, and it doesn’t look like a quick fit as a general replacement for “MapToVisualRectInAncestorSpace but without clipping”.
A few representative problem cases, which I've added to the virtual/content-extraction VirtualTestSuite.
1. Page scale / scaling (including fixed-position + iframe interactions)
These appear to be transform-tree scale / pageScaleFactor scenarios where
transform-only projection diverges from the visual-rect
mapping pipeline (which is what visible_bounding_box uses).
Example: compositing/geometry/fixed-position-composited-page-scale.html
(also ...-page-scale-scroll.html, ...-iframe-...-page-
scale.html, ...-transform-...-page-scale.html, and
compositing/scaling/tiled-layer-recursion.html).
2. Fragmentation (documented limitation in the suggested pattern).
The ObjectToViewTransform helper that demonstrates this pattern is documented
with: “NOTE: This doesn't work if object has multiple block fragments.” Even if
we start from a single local bounding rect, mapping via FirstFragment() /
PaintOffset() is inherently fragment-dependent, so it can under-map relative to
MapToVisualRectInAncestorSpace() which is built to handle visual-
rect mapping through layout/paint state.
Example: compositing/gestures/gesture-tapHighlight-block-in-multicol.html
(and generally multicol / fragmentation/* coverage).
3. Viewport/visual viewport / scrollbars (often combined with scaling).
A bunch of failures cluster around viewport sizing and scrolling behavior,
suggesting “viewport space” as computed by the visual-
rect mapping codepath isn’t consistently reproducible via projection.
Example: css3/viewport-percentage-lengths/viewport-percentage-lengths-scaled-
normal-scrollbars.html (also ...scaled-overlay-
scrollbars.html, fast/dom/viewport/viewport-dimensions-*.html).
Apologies if I'm misunderstanding and this is simple. Do you want me to try to
address these issues?
Or perhaps the new flag in this CL is still worth considering, in that using a
single mapping pipeline makes outer.Contains(visible) true by definition.
Can you also check the result when using geometry mapper?
```
unclipped = local_rect;
EXPECT_TRUE(target->MapToVisualRectInAncestorSpace(
&GetLayoutView(), unclipped, static_cast<VisualRectFlags>(kSkipAncestorAndViewportClips | kUseGeometryMapper)));
EXPECT_EQ(unclipped, PhysicalRect(-30, -35, 40, 40));
```
(similarly for the others)
Good suggestion. This was a good thing to check especially with the new fast path code for null ancestor as viewport. It led to a failure and then fix.
Related details
- 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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
// Only supported for DCHECK-enabled builds.As per the AI review agent, `AIPageContentBuildOnLoadForTesting` is also only enabled for DCHECK builds. This comment should probably be duplicated for `AIPageContentCheckGeometry` rather than moved.
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
still respecting ancestor transforms, scroll offsets, and filters.I think the geometry mapper codepath should be able to handle #1 and #3 correctly, and is generally more stable infra. I am not sure #2 is handled correctly in either codepath. In any case, let's continue with this patch for now, and you can consider switching to the fast-path in the future.
transform_state.Flatten();Should we just return true above this, rather than doing any of this function?
This test demonstrates the issue:
```
TEST_P(VisualRectMappingTest, RegressionTest_SkipAncestorClipExpansion) {
SetBodyInnerHTML(R"HTML(
<style>
body { margin: 0; }
#grandparent {
position: relative;
left: 100px; top: 100px;
}
#parent {
width: 100px; height: 100px;
transform: rotate(45deg);
overflow: hidden;
}
#child {
width: 20px; height: 20px;
background: green;
transform: rotate(-45deg);
}
</style>
<div id="grandparent">
<div id="parent">
<div id="child"></div>
</div>
</div>
)HTML");
auto* grandparent = GetLayoutObjectByElementId("grandparent");
auto* child = GetLayoutObjectByElementId("child");UpdateAllLifecyclePhasesForTest();
PhysicalRect local_rect(0, 0, 20, 20);
// Slow path.
PhysicalRect slow_result = local_rect;
child->MapToVisualRectInAncestorSpace(To<LayoutBoxModelObject>(grandparent), slow_result, kSkipAncestorAndViewportClips);
// Fast path (GeometryMapper).
PhysicalRect fast_result = local_rect;
child->MapToVisualRectInAncestorSpace(To<LayoutBoxModelObject>(grandparent), fast_result, static_cast<VisualRectFlags>(kSkipAncestorAndViewportClips | kUseGeometryMapper));
EXPECT_NEAR(slow_result.Width().ToFloat(), 20.0f, 0.1f);
EXPECT_NEAR(slow_result.Height().ToFloat(), 20.0f, 0.1f);
EXPECT_NEAR(fast_result.Width().ToFloat(), 20.0f, 0.1f);
EXPECT_NEAR(fast_result.Height().ToFloat(), 20.0f, 0.1f);
EXPECT_EQ(slow_result, fast_result);
}
```
Related details
- Aaron Leventhal
Aaron Leventhal (Gerrit)
Aaron Leventhal added 4 comments![]( "Open in Gerrit")
bool apply_overflow_clip = true,
bool apply_viewport_transform = false,
bool apply_viewport_clip = true);Aaron Leventhalnit: This is getting into enum bitflags territory. :)
I put this part of the change in https://chromium-review.googlesource.com/c/chromium/src/+/7213198
Note that it adds 35 lines of code. Not sure whether it's worth it.
Done
Done
I glanced through the call-sites to confirm this should be the return value. One of the comments says that this API returns true if no clipping was applied so this seems reasonable to me. But leaving a comment so a more familiar owner can validate.
Done. Looks like it was answered above essentially. Not only return true, but make this function a noop
// Only supported for DCHECK-enabled builds.As per the AI review agent, `AIPageContentBuildOnLoadForTesting` is also only enabled for DCHECK builds. This comment should probably be duplicated for `AIPageContentCheckGeometry` rather than moved.
- Khushal Sagar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |