[web] DomRenderer becomes FlutterViewEmbedder #29994
Conversation
flutter-dashboard
bot
added
the
platform-web
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
LGTM with some minor suggestions!
| } | ||
| if (rootElement != null) { | ||
| domRenderer.removeAllChildren(rootElement!); | ||
| flutterViewEmbedder.removeAllChildren(rootElement!); |
There was a problem hiding this comment.
removeAllChildren should also move to util.dart?
| bool? _ellipseFeatureDetected; | ||
|
|
||
| /// Draws CanvasElement ellipse with fallback. | ||
| void drawEllipse( | ||
| html.CanvasRenderingContext2D context, | ||
| double centerX, | ||
| double centerY, | ||
| double radiusX, | ||
| double radiusY, | ||
| double rotation, | ||
| double startAngle, | ||
| double endAngle, | ||
| bool antiClockwise) { | ||
| // ignore: implicit_dynamic_function | ||
| _ellipseFeatureDetected ??= js_util.getProperty(context, 'ellipse') != null; | ||
| if (_ellipseFeatureDetected!) { |
There was a problem hiding this comment.
Can this be:
| bool? _ellipseFeatureDetected; | |
| /// Draws CanvasElement ellipse with fallback. | |
| void drawEllipse( | |
| html.CanvasRenderingContext2D context, | |
| double centerX, | |
| double centerY, | |
| double radiusX, | |
| double radiusY, | |
| double rotation, | |
| double startAngle, | |
| double endAngle, | |
| bool antiClockwise) { | |
| // ignore: implicit_dynamic_function | |
| _ellipseFeatureDetected ??= js_util.getProperty(context, 'ellipse') != null; | |
| if (_ellipseFeatureDetected!) { | |
| // ignore: implicit_dynamic_function | |
| late final bool _ellipseFeatureDetected = js_util.getProperty(context, 'ellipse') != null; | |
| /// Draws CanvasElement ellipse with fallback. | |
| void drawEllipse( | |
| html.CanvasRenderingContext2D context, | |
| double centerX, | |
| double centerY, | |
| double radiusX, | |
| double radiusY, | |
| double rotation, | |
| double startAngle, | |
| double endAngle, | |
| bool antiClockwise) { | |
| if (_ellipseFeatureDetected) { |
There was a problem hiding this comment.
No, because context is an argument to drawEllipse.
| void main() { | ||
| internalBootstrapBrowserTest(() => testMain); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should this file be renamed?
| test('can set style properties on elements', () { | ||
| final DomRenderer renderer = DomRenderer(); | ||
| final html.Element element = renderer.createElement('div'); | ||
| DomRenderer.setElementStyle(element, 'color', 'red'); | ||
| final html.Element element = html.document.createElement('div'); | ||
| setElementStyle(element, 'color', 'red'); | ||
| expect(element.style.color, 'red'); | ||
| }); |
There was a problem hiding this comment.
Some of these tests should probably move to util_test.dart now.
lib/web_ui/test/clipboard_test.dart
Outdated
|
|
||
| Future<void> testMain() async { | ||
| await ui.webOnlyInitializeTestDomRenderer(); | ||
| await ui.webOnlyInitializeTestFlutterViewEmbedder(); |
There was a problem hiding this comment.
This is a potentially user-breaking change, but if all tests pass... no problem!
There was a problem hiding this comment.
Good catch! While it's not a breaking change, I realized that this method doesn't need to be in dart:ui and it doesn't need the webOnly prefix. It exists only for the engine's own tests. Framework-side tests use ui.debugEmulateFlutterTesterEnvironment = true;.
I moved it to the engine test_embedding.dart and deleted the dart:ui-side code.
|
|
3 participants
Final step in turning
DomRendererfrom a kitchen sink of various DOM manipulation utilities and a manager of Flutter Web's top elements into a single-purpose class - to manage the placement of Flutter views (currently only one). This PR achieves this in two steps:DomRenderertoFlutterViewEmbedder(9bdb6e3)