Repository navigation
docs(rfc): add RFC 420.0000 - Automatic PointerInterceptors on the web - #39
harryterkelsen wants to merge 5 commits into
Conversation
|
|
||
|  | ||
|
|
||
| A single proxy over the whole scene would also block PV2. Placing each platform |
There was a problem hiding this comment.
It the issue because the pointer interceptor doesn't know how to insert below PV2?
There was a problem hiding this comment.
Yeah. So I was trying to show with this example that it's not enough to just put a div over the entire scene to capture the pointer events. The divs needs to be interleaved between platform views in order for the pointer event capture to work correctly.
| widget. Because it is a platform view, it lands in the DOM above earlier | ||
| platform views and receives the click instead. The click then bubbles up to the | ||
| Flutter view, where Flutter's existing pointer handling processes it. This | ||
| proposal uses the same idea, but the engine places plain elements instead of |
There was a problem hiding this comment.
correct me if I am wrong,
PointerInteceptor creates a platformview widget that host a empty div to capture the pointer event, and this proposal is purely on engine side that create an empty div. so the difference is whether framework is involved?
There was a problem hiding this comment.
That's pretty much right. The difference is that instead of the developer having to manually place an empty div to capture pointer events, the framework handles it for them (and also streamlines it a bit since these hit-test regions will be "first-class" entities in the Flutter Web engine, so we don't need to use the Platform View infrastructure just to put a pointer-event-capturing div on the page)
| platform view. | ||
|
|
||
| After that, a `RenderBox` claims its clipped global rect if | ||
| `hitTestSelf` is true at its center. `hitTestSelf` is the method Flutter's hit |
There was a problem hiding this comment.
note the doc for hitTestSelf mention you don't need to implement it if you override hitTest directly, so there is chance that this return false, but still accept hittest
There was a problem hiding this comment.
Good catch. In my current implementation, most of these cases are handled by overriding visitHitTestRegions on those widgets, but I missed a few, so I'll have to handle those also. In the "future work" section, I have a bullet point about doing the "real" hit test check in debug mode, maybe we can promote that from future work to part of this RFC?
| surfaces, text, images and opaque gesture detectors claim, and translucent ones | ||
| don't. | ||
|
|
||
| * `IgnorePointer` and `SliverIgnorePointer` subtrees claim nothing, but the |
There was a problem hiding this comment.
This concerns me a bit that we have to tell what should block case by case.
There was a problem hiding this comment.
We should have a flag in RenderObject says if this is will swallow hittest for subtree.
There was a problem hiding this comment.
I agree. So we can get rid of the case-by-case list of widgets by adding 2 bools on RenderObject:
- a bool which says if the RenderObject blocks children from getting hit tests (AbsorbPointer, IgnorePointer, etc)
- a bool which says "my children can get hit tests outside of my bounds" (Transform, Follower, etc)
These will give us enough to get rid of the special case list of Widgets (and also third-party widgets will be able to give us this information as well).
There was a problem hiding this comment.
I updated the RFC to use this idea instead of the widget list. I made a small tweak from my comment above: the second bool "my children can get hit test outside of my bounds" is on RenderBox. And for the widgets which are skipped because they are not in the scene (Offstage, SliverOffstage, etc), instead of specially listing them we just override visitChildrenInHitTestOrder for them (which is the same thing we do for IndexedStack)
| The walk only needs what is painted over platform views: | ||
|
|
||
| * nothing painted before the first platform view can cover it; | ||
| * once a claim covers every platform view seen so far, later content can't |
There was a problem hiding this comment.
how do we figure this out? if there are corner cases like multiple smaller rect each only cover a part of the platform view, it may be more expansive to calculate whether they completely cover a platform view. we also need to handle when each rect may change location
There was a problem hiding this comment.
For this optimization, we only check if a single hit-test region covers every platform view seen so far. It doesn't try to compute a union. But it turns out this simple check is enough to catch a large number of cases in real world apps (e.g. modal barriers that cover the whole screen)
| * once a claim covers every platform view seen so far, later content can't | ||
| matter until another platform view appears; | ||
| * a box that claims its whole rect makes its children redundant; | ||
| * subtrees whose clip or hit bounds miss every platform view are skipped; |
There was a problem hiding this comment.
are we going through the walk every frame? or are we going to have some kind of incrememtal update model? this seems hard to do if it is the latter
There was a problem hiding this comment.
It walks the render tree on every frame, but each walk usually only hits a few nodes except for in somewhat contrived edge cases (see the appendix). However, it only sends updates to the engine if the hit test regions change.
| platform view remains the worst case, at about 1,100. See | ||
| [Appendix C](#appendix-c-walk-size). | ||
|
|
||
| ### `FlutterView.updateHitTestRegions` |
There was a problem hiding this comment.
instead of a separate flush pipeline, can we do it in a way that renderobject leaves a mark on the layer tree and web engine uses these mark to insert the covering div for the platform view.
There was a problem hiding this comment.
This is actually something I explored a lot (this is pretty much what Appendix B talks about). The main problem is that hit test behavior can change without paint/layer behavior changing. So we would need for widgets which change their hit test behavior to request repaint or signal somehow that the layer tree needs to change. We can't enforce this for third-party widgets.
There was a problem hiding this comment.
if we add a flag suggested in #39 (comment), we could add a markNeedsPaint when the flag changes right?
There was a problem hiding this comment.
The reason I want to do it this way is the avoid a separate tree work to collect the rect which also comes with geometry calculation. and if we have to do some complex calculation, it should happen in engine side if possible so that we can keep that as an implementation detail
There was a problem hiding this comment.
The main reasons I think the Layer-marking approach won't work as well as the separate-walk approach is that in order to work, changing hit-test behavior must force a repaint. So, even if we have the flag for hitTestsChildren do markNeedsPaint when it changes, that doesn't account for every hit-test-behavior-changing change. There are other examples in the framework of hit test behavior changing without markNeedsPaint (e.g. RenderProxyBoxWithHitTestBehavior.behavior, _RenderInkFeatures.absorbHitTest, PlatformViewRenderBox.hitTestBehavior, CustomPaint, etc.) Even if we enforced the "hit test behavior change always induces a repaint" contract in the framework, we wouldn't be able to enforce the contract for third-party widgets. The separate-walk approach solves this by walking the entire tree regardless. The extra advantage of the separate walk is that since it is specifically tailored to this task (automatic PointerInterceptors), we can make optimizations like completely skipping it if there are no platform views, ignoring subtrees before the first platform view, etc. So, the geometry calculations for the rects end up being quite small everywhere except for contrived worse-case scenarios (in my benchmarks the total number of renderobjects visited ended up being ~20-50 for most normal apps).
There was a problem hiding this comment.
wouldn't be able to enforce the contract for third-party widgets
the current proposal still require custom widget to implement the additional API correctly. if we provide a setter or autolisten hittest state, i don't see it will be worse than the current proposal
There was a problem hiding this comment.
Custom widgets will only need to implement the additional API if they don't use default hit testing (e.g. if they override hitTest instead of hitTestSelf and/or hitTestChildren). Third-party widgets will only need to implement the custom API if they do custom hit-testing or paint their children out of order (and even when they do that, we have a check which runs in debug mode which can detect it automatically). Even if we go the "hit test information in the layer tree" route, we will need to have these APIs so we can get the correct hit test information from the render objects. The difference is that now those render objects will need to repaint when the hit test information changes
- Describe how the proposal differs from PointerInterceptor: the framework places the interceptors, and the engine adds plain elements instead of platform views. - Explain why proxies have to be interleaved with platform views. - Say that the pass runs every frame, from scratch, and only notifies the engine when the regions change. - Replace the list of special-cased render object types with two render object getters (whether children can be hit, and whether they can be hit outside the parent's bounds), plus visitChildrenInPaintOrder for children that aren't painted. - Add a section on matching the real hit test, and move the debug hit-test check from later work into the proposal. - Clarify that the early stop only checks a single claimed rect. - Expand the layer-marks alternative.
- Drop the frame-time comparison with the paint-time prototype. It came from an unoptimized prototype on a benchmark that repaints everything every frame, so it isn't evidence either way. - Add the variant raised in review: repaint whenever a hit-test flag changes. That keeps the flags correct, but hitTestSelf and visitHitTestRegions answers can still change without a repaint.
chunhtai
left a comment
There was a problem hiding this comment.
I overall onboard with the idea of getting rect that will cover platform. but I prefer we don't do anything expansive calculation in framework because it will likely introduce complexity in framework API which will be hard to change and have bigger impact on the ecosystem.
We should aiming at introduce minimum api change in renderobject which only leave clues in scene to be picked up by webengine.
|
|
||
| ### When the pass runs, and why | ||
|
|
||
| The pass runs from a persistent frame callback **after** `drawFrame`, so after |
There was a problem hiding this comment.
since this is render object API, can position this call loop during drawframe after
https://github.com/flutter/flutter/blob/235d00193f11278b2e28872b4f17469d58a0dc5b/packages/flutter/lib/src/rendering/binding.dart#L698
|
|
||
| ### Keeping the walk small | ||
|
|
||
| The walk only needs what is painted over platform views: |
There was a problem hiding this comment.
also Invisible platform views or out of screen one should be ignored
There was a problem hiding this comment.
Offscreen ones are ignored (but I will add more optimizations to clip the bounds by the scene's overall bounds and ignore empty-sized rects). However, invisible platform views (ones registered with isVisible: false) still need to be processed because they can handle click events (that's exactly what the current PointerInterceptors do).
| platform view remains the worst case, at about 1,100. See | ||
| [Appendix C](#appendix-c-walk-size). | ||
|
|
||
| ### `FlutterView.updateHitTestRegions` |
There was a problem hiding this comment.
wouldn't be able to enforce the contract for third-party widgets
the current proposal still require custom widget to implement the additional API correctly. if we provide a setter or autolisten hittest state, i don't see it will be worse than the current proposal
| object types to decide what can be hit, and third-party render objects can take | ||
| part: | ||
|
|
||
| * A protected `RenderObject.visitHitTestRegions` reports where a render object |
There was a problem hiding this comment.
this api should be on renderbox if possible, size is only introduced in RenderBox
Putting the clues in |
- Run the pass as a step in RendererBinding.drawFrame, right after the scene is composited, instead of from a persistent frame callback. - Declare visitHitTestRegions on RenderBox, since it needs size. - Clip platform views to the view's bounds and ignore ones with an empty visible rect. Explain why platform views registered with isVisible: false are still processed. - Mark the new render object members @internal while the feature is opt-in.
This RFC records the design for automatic pointer interceptors on Flutter Web, and the alternatives we considered. Today, Flutter content painted over a platform view (such as an iframe) doesn't receive clicks unless the app wraps it in
PointerInterceptor. The proposal fixes this in the framework and the web engine, with no app changes. After each frame, a render-tree walk finds the areas over platform views that Flutter's hit test would claim, and the engine places invisible proxy elements over them.The design was settled in several web team discussions, and the implementation is underway, starting with flutter/flutter#195607. The RFC also covers the rejected alternatives (inlining
pointer_interceptorinto the framework, deriving regions from semantics, and recording regions during paint) and why each was rejected.I drafted this RFC with AI assistance from my own design notes, measurements and team discussions. I have reviewed and edited it, and I'm responsible for its content.
Tracking issue: flutter/flutter#179806
Pre-launch Checklist
design doc.dart run bin/assign_rfc_number.dartlocally when instructed).If you need help, consider asking for advice on the #hackers channel on Discord.