Repository navigation
View Metrics Overrides - #190375
View Metrics Overrides#190375victorsanni wants to merge 19 commits into
Conversation
github-actions
Bot
added
the
framework
… gestures DPR etc
github-actions
Bot
added
a: accessibility
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a debug-only view metrics override mechanism (ViewMetricsOverride) to allow developer tooling to simulate different device configurations (such as device pixel ratio, physical size, and accessibility settings) per view. It integrates these overrides into RendererBinding, MediaQuery, pointer event conversion, and accessibility evaluations, and registers the ext.flutter.viewMetricsOverride service extension. A syntax error was identified in the newly added test file where .new() is used incorrectly to initialize a map.
| return _instance ??= ViewMetricsOverrideTestBinding(); | ||
| } | ||
|
|
||
| final Map<String, ServiceExtensionCallback> extensions = .new(); |
There was a problem hiding this comment.
The syntax .new() is invalid in Dart without a receiver class or constructor prefix. It should be initialized as an empty map literal <String, ServiceExtensionCallback>{} or using Map<String, ServiceExtensionCallback>.new().
| final Map<String, ServiceExtensionCallback> extensions = .new(); | |
| final Map<String, ServiceExtensionCallback> extensions = <String, ServiceExtensionCallback>{}; |
| @protected | ||
| ViewConfiguration createViewConfigurationFor(RenderView renderView) { | ||
| return ViewConfiguration.fromView(renderView.flutterView); | ||
| final ui.FlutterView view = renderView.flutterView; |
There was a problem hiding this comment.
feels like a weird place to inject the override, if we want to override the viewConfig, should we do it in ViewConfiguration.fromView?
that being said, should we do it in mediaquery level, so we can keep all override in the same place.
dpr -> MediaQueryData.devicePixelRatio
physical size -> MediaQueryData.size
There was a problem hiding this comment.
we can keep all override in the same place.
There are two things I am worried about if the view is not aware of size and dpr overrides:
-
Some widgets layout with MediaQuery size, some don't (so get their constraints from view constraints). If a developer overrides the size in devtools, some parts of their layout will change but some won't, leading to a layout state that never really happens in real life. On a real device of a specific size, the view is that size so the layout changes in a predictable way.
-
AccessibilityEvaluations e.g MinimumTapTargetEvaluation is using view.devicePixelRatio not MediaQueryData.devicePixelRatio. Say a developer overrides dpr in devtools and reruns the evaluation, the new evaluation run will ignore the dpr change since the view's dpr is not changing.
There was a problem hiding this comment.
yeah you are correct, it seems it is safer to do override view level directly.
| /// and the overridden value. | ||
| @protected | ||
| double? devicePixelRatioForView(int viewId) { | ||
| return platformDispatcher.view(id: viewId)?.devicePixelRatio; |
There was a problem hiding this comment.
If we want to do it outside of mediaquery, should we go all the way back to dart:ui? that way, we don't need to have different override across different place like this
There was a problem hiding this comment.
Agree here, it feels like fighting multiple fires trying to inject the override in MediaQueryData, ViewConfiguration, a11y evaluations etc. With this option overrides won't propagate to direct ui.FlutterView consumers, each will have to apply the overrides itself.
Like you said, it will probably be better to wrap the FlutterView so every consumer gets it automatically (option 2 in the doc). Since we can't override all metrics cleanly only in MediaQueryData, might as well just do it in dart:ui. The doc says it is significant development effort though, I am not sure since there is already precedent of implementing FlutterView in the framework (TestFlutterView in flutter_test) where it didn't look too hard to do.
I will convert this PR to a draft while exploring a dart:ui solution; when that is done I will close this PR as superseded by that one.
| ..onPlatformBrightnessChanged = handlePlatformBrightnessChanged; | ||
| addPersistentFrameCallback(_handlePersistentFrameCallback); | ||
| assert(() { | ||
| debugViewMetricsOverridesNotifier.addListener(_debugHandleViewMetricsOverridesChanged); |
There was a problem hiding this comment.
as part of the pr, we should also attempt to wire up the vm services, just to make sure everything can be hooked up as expected.
tracking issue: flutter/devtools#9893
Implements Option 4 in https://flutter.dev/go/view-metrics-overrides:
physicalSizeanddevicePixelRatiooverrides on the render level (ViewConfiguration) andMediaQuerylevelViewMetricsOverridesto encapsulate view metricsdebugViewMetricsOverridesmap for per-ViewoverridesdebugBrightnessOverridefor now, added TODO to refactor and usedebugViewMetricsOverridesin subsequent PRdisplayCornerRadiiinMediaQueryData.copyWithsince it depends on overridabledevicePixelRatioMinimumTapTargetEvaluation) read fromFlutterViewdirectly which will bypass overrides, this PR makes it read fromRenderViewwhich already has overrides applied viaViewConfiguration.GestureBinding._devicePixelRatioForViewso thatRendererBindingcan override it to apply view metrics overrides. This makes it so gestures aren't using the wrong DPR.ViewMetricsOverridesAPI docs.