[web] Implement multi-view for semantics #48251

yjbanov · 2023-11-21T00:06:17Z

Implement multi-view for semantics. Main changes:

EngineSemanticsOwner singleton is split into two classes:
- EngineSemantics is a singleton that manages global properties of the semantics tree, such as whether semantics is currently enabled, and what gesture mode is used.
- EngineSemanticsOwner now supports creating multiple instances of the class. EngineFlutterView now points a EngineSemanticsOwner that it owns.
Fix a number of issues with disposal logic. Now that views can come in and out disposal has to be more robust than previously. EngineSemanticsOwner and SemanticsObject acquired a dispose() method that cleans up the tree and individual nodes respectively.
- In particular, this fixes an issue in Skwasm mode where slight differences in asynchrony caused the semantics test to fail because old nodes continued firing events after they were removed from the document due to improper disposal.

mdebbar · 2023-11-21T20:34:24Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

  ''')
  void updateSemantics(ui.SemanticsUpdate update) {
-    EngineSemanticsOwner.instance.updateSemantics(update);
+    implicitView?.semantics.updateSemantics(update);


At some point, we need the framework to pass a view parameter here, right?

According to the deprecation message above this method is going away forever.

mdebbar · 2023-11-21T20:36:08Z

lib/web_ui/lib/src/engine/semantics/scrollable.dart

+      final Zone zone = Zone.current;
      _scrollListener = createDomEventListener((_) {
-        _recomputeScrollPosition();
+        zone.run(_recomputeScrollPosition);


Is this necessary? If so, could you add a comment explaining why?

Good catch! This was left-over from my experimentation. Will clean up.

mdebbar · 2023-11-21T20:43:19Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

-      for (int i = 0; i < len; i++) {
-        _detachObject(keys[i]);
+      for (final EngineFlutterView view in EnginePlatformDispatcher.instance.views) {
+        view.semantics.dispose();


What happens if semantics is enabled again later? Is there a way to revive view.semantics?

In a production setting semantics is only enabled one-way; there's no going back. However, our tests do rely on being able to dispose of the old tree to set up a clean environment for the following tests, so the object is revivable after it is disposed. This is more like "reset" than it is "dispose". I'll rename it.

`FakeView` wraps the same underlying `FlutterView`. Sending semantics updates and Scene objects from multiple fake views into the same engine `FlutterView` violates contracts with the engine. This PR stubs out `render` and `updateSemantics` methods in `FakeView` classes to prevent that. This unblocks flutter/engine#48251, which implements multi-view semantics for web.

mdebbar

LGTM! But I'm not sure if we should keep the flt-view-id attribute. It doesn't seem to be used in any meaningful way in this PR. Do you have plans to use it in the future?

mdebbar · 2023-11-30T18:19:24Z

lib/web_ui/test/engine/semantics/semantics_multi_view_test.dart

+    expect(domDocument.querySelectorAll('flutter-view[flt-view-id="${view1.viewId}"]'), hasLength(1));
+    expect(domDocument.querySelectorAll('flutter-view[flt-view-id="${view2.viewId}"]'), hasLength(1));


I'm still unsure what the purpose of the flt-view-id attribute is. These checks seem to be equivalent to:

Suggested change

expect(domDocument.querySelectorAll('flutter-view[flt-view-id="${view1.viewId}"]'), hasLength(1));

expect(domDocument.querySelectorAll('flutter-view[flt-view-id="${view2.viewId}"]'), hasLength(1));

expect(view1.dom.rootElement.isConnected, isTrue);

expect(view2.dom.rootElement.isConnected, isTrue);

As discussed offline, this attribute is useful for manual inspection, testing, etc. I left some docs about it.

ditman

LGTM! Comments are minor, and we can tweak/fix later.

ditman · 2023-11-30T19:35:13Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

+  bool _isDisposed = false;
+
+  void dispose() {
+    assert(!_isDisposed);


This assert goes away at build time, should this also if (_isDisposed) return;? Or is this only expected to fail at dev time?

That's a good question. I'm just following an established convention. This pattern is used everywhere else. I'd be on board to revisit it, but that's probably outside the scope of this PR?

Definitely out of scope, I didn't realize this pattern was the standard :P

ditman · 2023-11-30T19:43:28Z

lib/web_ui/lib/src/engine/view_embedder/dom_manager.dart

+    // Example:
+    //
+    //     document.querySelector('flutter-view[flt-view-id="$viewId"]')
+    rootElement.setAttribute('flt-view-id', viewId);


The embedding strategy is setting most of the flt values in the hostElement:

engine/lib/web_ui/lib/src/engine/embedder.dart

Lines 57 to 66 in 3848f50

// Initializes the embeddingStrategy so it can host a single-view Flutter app.

window.embeddingStrategy.initialize(

hostElementAttributes: <String, String>{

'flt-renderer': '${renderer.rendererTag} ($rendererSelection)',

'flt-build-mode': buildMode,

// TODO(mdebbar): Disable spellcheck until changes in the framework and

// engine are complete.

'spellcheck': 'false',

},

);

It is likely that this will eventually move there once it is made view-aware.

SGTM. Feel free to move it around when the embedder becomes multi-view aware.

ditman · 2023-11-30T19:45:52Z

lib/web_ui/test/engine/semantics/semantics_multi_view_test.dart

+  // TODO(yjbanov): see https://github.com/flutter/flutter/issues/138906
+  //                This test tests multi-view mode only, but currently there's
+  //                no way to cleanly boot into pure multi-view mode. As a
+  //                workaroud we boot into a mode that includes the implicit
+  //                view, but we then remove the implicit view.
+  await bootstrapAndRunApp();
+  final EngineFlutterWindow implicitView = EnginePlatformDispatcher.instance.implicitView!;
+  EnginePlatformDispatcher.instance.viewManager.disposeAndUnregisterView(implicitView.viewId);


Mouad has landed the bit of code that disables the implicit view when multiViewEnabled is true. He's writing other tests for that, so he might be able to revisit this one!

…or message

flutter-dashboard · 2023-11-30T23:36:49Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #48251 at sha a8f95c6

yjbanov · 2023-12-01T19:09:06Z

Landing manually. The gold check seems to be a false alarm. It likely complains about existing non-triaged goldens in the engine repo, but the triage for this PR is empty (and it should be).

yjbanov · 2023-12-01T19:09:21Z

I'll still keep an eye on it after I merge.

)

…139388) flutter/engine@69f0e55...95995c4 2023-12-01 [email protected] [web] Implement multi-view for semantics (flutter/engine#48251) 2023-12-01 [email protected] [Impeller] Fix size of squares in DrawPoints(PointMode). (flutter/engine#48547) 2023-12-01 [email protected] Roll Skia from ecf10008c8bd to 5849e2b1aa8f (2 revisions) (flutter/engine#48569) 2023-12-01 [email protected] Roll Skia from 0ed7f26b3adb to ecf10008c8bd (1 revision) (flutter/engine#48568) 2023-12-01 [email protected] Roll Skia from e69fbf086bb6 to 0ed7f26b3adb (1 revision) (flutter/engine#48565) 2023-12-01 [email protected] Roll Skia from 57c6a5a97dc5 to e69fbf086bb6 (1 revision) (flutter/engine#48562) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

) `FakeView` wraps the same underlying `FlutterView`. Sending semantics updates and Scene objects from multiple fake views into the same engine `FlutterView` violates contracts with the engine. This PR stubs out `render` and `updateSemantics` methods in `FakeView` classes to prevent that. This unblocks flutter/engine#48251, which implements multi-view semantics for web.

…lutter#139388) flutter/engine@69f0e55...95995c4 2023-12-01 [email protected] [web] Implement multi-view for semantics (flutter/engine#48251) 2023-12-01 [email protected] [Impeller] Fix size of squares in DrawPoints(PointMode). (flutter/engine#48547) 2023-12-01 [email protected] Roll Skia from ecf10008c8bd to 5849e2b1aa8f (2 revisions) (flutter/engine#48569) 2023-12-01 [email protected] Roll Skia from 0ed7f26b3adb to ecf10008c8bd (1 revision) (flutter/engine#48568) 2023-12-01 [email protected] Roll Skia from e69fbf086bb6 to 0ed7f26b3adb (1 revision) (flutter/engine#48565) 2023-12-01 [email protected] Roll Skia from 57c6a5a97dc5 to e69fbf086bb6 (1 revision) (flutter/engine#48562) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

yjbanov requested review from ditman and mdebbar November 21, 2023 00:06

github-actions bot added the platform-web Code specifically for the web engine label Nov 21, 2023

yjbanov force-pushed the a11y-multi-view branch from 3aeb2a4 to 0e35738 Compare November 21, 2023 03:29

mdebbar reviewed Nov 21, 2023

View reviewed changes

yjbanov mentioned this pull request Nov 21, 2023

make FakeView not send Scene and semantics to the engine flutter/flutter#138849

Merged

yjbanov requested a review from mdebbar November 22, 2023 22:02

yjbanov force-pushed the a11y-multi-view branch from 00a36d6 to 90de867 Compare November 28, 2023 18:32

mdebbar approved these changes Nov 30, 2023

View reviewed changes

ditman approved these changes Nov 30, 2023

View reviewed changes

yjbanov added 10 commits November 30, 2023 14:34

multi-view semantics draft

8b85882

merge with EngineFlutterView changes; add test

3b449eb

fix analysis warning

cec01cb

switch to bootstrapAndRunApp()

8d9177e

more robust debugEmulateHotRestart; more legible tree consistency err…

6489c99

…or message

fix engine bootstrap in multi view test

90ce14d

remove unnecessary zones and rename dispose => reset

9c5a03d

merge with upstream

9027a2c

check if semantics is enabled prior to updating semantic DOM

92b2a4e

document flt-view-id attribute

a8f95c6

yjbanov force-pushed the a11y-multi-view branch from 4c9431d to a8f95c6 Compare November 30, 2023 22:34

flutter-dashboard bot added the will affect goldens label Nov 30, 2023

yjbanov merged commit 95995c4 into flutter:main Dec 1, 2023

engine-flutter-autoroll mentioned this pull request Dec 1, 2023

Roll Flutter Engine from 69f0e5550702 to 95995c48d591 (6 revisions) flutter/flutter#139388

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2023

95995c48d [web] Implement multi-view for semantics (flutter/engine#48251

e85317a

)

yjbanov mentioned this pull request Dec 2, 2023

[web:multi-view] render semantics to multiple FlutterViews flutter/flutter#137445

Closed

		expect(domDocument.querySelectorAll('flutter-view[flt-view-id="${view1.viewId}"]'), hasLength(1));
		expect(domDocument.querySelectorAll('flutter-view[flt-view-id="${view2.viewId}"]'), hasLength(1));

	// Initializes the embeddingStrategy so it can host a single-view Flutter app.
	window.embeddingStrategy.initialize(
	hostElementAttributes: <String, String>{
	'flt-renderer': '${renderer.rendererTag} ($rendererSelection)',
	'flt-build-mode': buildMode,
	// TODO(mdebbar): Disable spellcheck until changes in the framework and
	// engine are complete.
	'spellcheck': 'false',
	},
	);

[web] Implement multi-view for semantics #48251

[web] Implement multi-view for semantics #48251

Uh oh!

Conversation

yjbanov commented Nov 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Nov 30, 2023

Uh oh!

yjbanov commented Dec 1, 2023

Uh oh!

yjbanov commented Dec 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ditman Nov 30, 2023 •

edited

Loading