Refine APC Geometry Pipeline [chromium/src : main]
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Commit-Queue | +1 |
2 comments
HashSet<DOMNodeId, IntWithZeroKeyHashTraits<DOMNodeId>>I got errors that base::flat_set/flat_map are not allowed in blink.
namespace {Aaron Leventhalnit: Isn't this already in anonymous namespace? why do need another one?
Done
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Why both hit test rects and fragments are needed:This approach of supporting both codepaths leads to a lot of complexity, which I would like to avoid. Can we avoid having two? What is the usecase for needing the hit test rect for out-of-viewport elements? If there is a need for this, WDYT of doing a second hit test that includes areas outside the viewport, so that you can re-use the new hit-test-region code for both of these usecases?
Related details
- Aaron Leventhal
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Why both hit test rects and fragments are needed:This approach of supporting both codepaths leads to a lot of complexity, which I would like to avoid. Can we avoid having two? What is the usecase for needing the hit test rect for out-of-viewport elements? If there is a need for this, WDYT of doing a second hit test that includes areas outside the viewport, so that you can re-use the new hit-test-region code for both of these usecases?
Hi PDR, thanks for taking a look — the intent behind keeping both paths is:
**Hit-test rects aren’t always available:**
The list-based sweep only emits geometry for nodes it touches while walking the current frame: actionable elements in the viewport. We still have to emit geometry for:
- structural containers that don’t paint on their own
- off-screen nodes when actionable mode is disabled, see performance concerns below
- the accessibility-focused node in non-actionable mode
In all of those cases there simply isn’t a hit-test rect to reuse, so we fall back to the fragment path to keep coverage for the whole tree. Dropping the fragment path would make those nodes lose geometry entirely.
**Running a second “full page” hit test is expensive:**
It’s possibly doable (and we are sketching a flag for experimenting with it), but it changes the performance characteristics because it is a second paint traversal. The fragment path already gives us geometry for every rendered object; the hit-test pass is just an optimization to reuse the exact click footprint (and z-order) for actionable nodes in view. Doing an extra page-wide pass to cover everything would double the cost without buying more coverage than we already get from fragments.
The resulting structure is:
```
if (actionable hit-test produced a rect for this node)
reuse it
else
build from fragments
```
That’s the minimum complexity I found that maintains full coverage with controllable costs.
Related details
- Khushal Sagar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Why both hit test rects and fragments are needed:Aaron LeventhalThis approach of supporting both codepaths leads to a lot of complexity, which I would like to avoid. Can we avoid having two? What is the usecase for needing the hit test rect for out-of-viewport elements? If there is a need for this, WDYT of doing a second hit test that includes areas outside the viewport, so that you can re-use the new hit-test-region code for both of these usecases?
Hi PDR, thanks for taking a look — the intent behind keeping both paths is:
**Hit-test rects aren’t always available:**
The list-based sweep only emits geometry for nodes it touches while walking the current frame: actionable elements in the viewport. We still have to emit geometry for:
- structural containers that don’t paint on their own
- off-screen nodes when actionable mode is disabled, see performance concerns below
- the accessibility-focused node in non-actionable mode
In all of those cases there simply isn’t a hit-test rect to reuse, so we fall back to the fragment path to keep coverage for the whole tree. Dropping the fragment path would make those nodes lose geometry entirely.
**Running a second “full page” hit test is expensive:**
It’s possibly doable (and we are sketching a flag for experimenting with it), but it changes the performance characteristics because it is a second paint traversal. The fragment path already gives us geometry for every rendered object; the hit-test pass is just an optimization to reuse the exact click footprint (and z-order) for actionable nodes in view. Doing an extra page-wide pass to cover everything would double the cost without buying more coverage than we already get from fragments.
The resulting structure is:
```
if (actionable hit-test produced a rect for this node)
reuse it
else
build from fragments
```That’s the minimum complexity I found that maintains full coverage with controllable costs.
Hit-test rects aren’t always available
Can you help me understand the usecase for needing the hit test rect for an out-of-viewport element?
Let's call the two codepaths "hit test rects" and "geometry-mapped rects". We discussed before how approaches other than "hit test rects" will be wrong in common cases, including outlines. What is the solution for this problem with "geometry-mapped rects"?
Running a second “full page” hit test is expensive
Is this true? At an extremely high level, "hit test rects" is O(|fragments|), and "geometry-mapped rects" is O(|fragments| * |tree depth|).
Related details
- Aaron Leventhal
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 4 comments![]( "Open in Gerrit")
Can discuss more when we meet later.
static HitTestGeometry ForRegion(const cc::Region& region) {Is this the API used when the node has multiple fragments?
class HitTestGeometry {what's the coordinate space of the geometry? can you add a comment.
bool ShouldUseHitTestGeometry(Overall feedback, we should use the hit testing algorithm as the source of truth for `visible_bounding_box` instead of any custom logic here. It makes it much easier to ensure what we use here stays aligned with hit testing. This is assuming the hit testing approach satisfies the following:
- Doesn't incorrectly include outline and floating descendants.
- Can provide separate rects for each fragment instead of a union.
Given the above, I'm not following why we need this use to be conditional. The calculation for `visible_bounding_box` should always be able to rely on the hit test geometry. The same way `document_scoped_z_order` relies on it for occlusion.
I was thinking about the case of anonymous layout objects. Since there's no node we likely don't get the hit test callback in those cases? But that means z order has the same issue and we're already ignoring such ContentNodes.
Nodes which are not visited by the hit testing algorithm will have an empty `visible_bounding_box` which is correct by design.
_____
Separately for elements outside the viewport for which we're providing `outer_bounding_box`. If we can derive that from hit testing too, that's great. But could we move that to a separate patch since this data is being used by an experimental feature? Fixing the `visible_bounding_box`, both cases where it currently includes content which isn't painted and the fact that we don't have fragments, will fix what's being used in production.
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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. |