Mark the Flutter Views as focusable by setting a tabindex value. #50876

tugorez · 2024-02-22T19:48:03Z

Mark the Flutter View as focusable by setting a tabindex value.

When a given flutter view is focused its tabindex will be -1
When a given flutter view is not focused its tabindex will be 0
When semantics are enabled no tabindex will be set.

Relevant Issues are:

Design doc: https://flutter.dev/go/focus-management
Focus in web multiview: [web:multi-view] Make keyboard and focus multi-view aware flutter#137443
Platform dispatcher changes: Add the focus state related methods to the platform dispatcher #49841

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ditman

The stuff we've been talking about lately about the other bindings lacking instrumentation so they could be used from this one kind of informed this review.

I think the coolest change to this would be to stop receiving a singular "onViewFocusChanged" callback, and instead, expose a Stream of ViewFocusChanged events from here; that way, the class becomes more decoupled from how it's going to be used by the PlatformDispatcher (or the tests)

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

ditman · 2024-02-23T01:08:51Z

lib/web_ui/test/engine/platform_dispatcher/view_focus_binding_test.dart

  });
 }

+EngineFlutterView createAndRegisterView(EnginePlatformDispatcher dispatcher) {


How many times is this function duplicated in tests? It looks familiar! (I wonder if it'd make sense to start creating a collection of these helpers so they can be reused across the engine tests)

Any advice on where it should go?

Happy to address this in a follow up PR!

lib/web_ui/lib/src/engine/window.dart

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

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

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

ditman · 2024-02-23T01:33:16Z

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


+  late final ViewFocusBinding _viewFocusBinding = ViewFocusBinding(
+    viewManager: viewManager,
+    onViewFocusChange: invokeOnViewFocusChange,


If this class exposed a stream of events, then you don't need to pass the onViewFocusChange through its constructor, and the viewManager can default to EnginePlatformDispatcher.instance.viewManager, so ViewFocusBinding can become a singleton again :P

Hmmmm I find a bit weird the fact that EnginePlatformDispatcher depends on ViewFocusBinding and viceversa.

? It already depends on it, I'm just saying that instead of:

final _viewFocusBinding = ViewFocusBinding( viewManager: viewManager, onViewFocusChange: invokeOnViewFocusChange, )

you do:

final _viewFocusBinding = ViewFocusBinding( viewManager: viewManager, ); // or: ViewFocusBinding.instance; // elsewhere _viewFocusBinding.onViewFocusChange.listen((ViewFocusChange event) { // Do whatever we need with the event now... });

That way you don't need to pass a tear-off into the ViewFocusBinding instance?

? It already depends on it, I'm just saying that instead of:

How so? The view focus binding class doesn't have any dependencies on EnginePlatformDispatcher only on FlutterViewFocusManager

The view focus binding class doesn't have any dependencies on EnginePlatformDispatcher only on FlutterViewFocusManager

Could you create the view focus binding class outside of the EnginePlatformDispatcher, without using the EnginePlatformDispatcher (to retrieve the viewManager)? I think that's a fairly strong dependency :P

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

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

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

yjbanov

LGTM if LGT @ditman

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

ditman · 2024-02-24T02:07:57Z

(Holding off on reviewing this, @tugorez might want to tweak some odds and ends, will follow up with him next week! Thanks for the feature @tugorez!!)

tugorez · 2024-02-26T23:03:46Z

Without these changes: https://flutterwebaccesibility2.web.app/
With these changes: https://flutter-web-20c94.web.app/

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

ditman · 2024-02-27T02:30:09Z

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

+  }
+
+  void _handleViewCreated(int viewId) {
+    _markViewAsFocusable(viewId, reachableByKeyboard: true);


If we were to listen to focusin/out, we'd need to use this method to attach listener to new views.

ditman

This LGTM, but I'd like the focusin/out events to be attached to the rootElements of the views, if technically possible. I think that'd simplify this code a good bit!

tugorez · 2024-02-28T21:38:39Z

I made some changes.

App with engine without these changes: https://a11ybefore.web.app/
App with engine with these changes: https://a11yafter.web.app/

ditman · 2024-02-29T03:01:52Z

@tugorez you're still testing with platform views, right?

tugorez · 2024-02-29T15:30:24Z

@tugorez you're still testing with platform views, right?

Waiting on a final extra review from @yjbanov to make sure my changes don't break semantics.

ditman · 2024-03-06T02:41:42Z

I made some changes.

App with engine without these changes: https://a11ybefore.web.app/

App with engine with these changes: https://a11yafter.web.app/

Took a look with VoiceOver, and both apps seem to work the same for me. Let's merge this!

When a given flutter view is focused its tabindex will be -1 When a given flutter view is not focused its tabindex will be 0

tugorez · 2024-03-08T22:41:14Z

Let's land this PR as is and follow up with the multiview verification in flutter/flutter#144853

auto-submit · 2024-03-08T22:41:32Z

auto label is removed for flutter/engine/50876, due to Pull request flutter/engine/50876 is not in a mergeable state.

auto-submit · 2024-03-08T23:08:54Z

auto label is removed for flutter/engine/50876, due to - The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

ditman · 2024-03-09T00:39:46Z

Let's go @tugorez!!

…alue. (flutter/engine#50876)

…144862) flutter/engine@953927e...7541e90 2024-03-08 [email protected] [Impeller] implement mask blur for textures (flutter/engine#51183) 2024-03-08 [email protected] Don't rely on dart binary on PATH in run_test.py (flutter/engine#51302) 2024-03-08 [email protected] Mark the Flutter Views as focusable by setting a tabindex value. (flutter/engine#50876) 2024-03-08 [email protected] Update the instructions for updating licenses. (flutter/engine#51297) 2024-03-08 [email protected] Optimize overlays in CanvasKit (flutter/engine#47317) 2024-03-08 [email protected] Roll Fuchsia Linux SDK from 5Ra_AjCji-uR1GaX7... to lAV5jgp4796siOZgI... (flutter/engine#51296) 2024-03-08 [email protected] [Impeller] Add the KHR prefix to existing swapchain utilities. (flutter/engine#51295) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 5Ra_AjCji-uR to lAV5jgp4796s 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] 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

github-actions bot added the platform-web Code specifically for the web engine label Feb 22, 2024

tugorez changed the title ~~[WIP] Works with semantics~~ Marks the Flutter Views as focusable. Feb 23, 2024

tugorez changed the title ~~Marks the Flutter Views as focusable.~~ Marks the Flutter Views as focusable by setting a tabindex value. Feb 23, 2024

tugorez marked this pull request as ready for review February 23, 2024 00:45

tugorez requested review from ditman and yjbanov February 23, 2024 00:46

ditman reviewed Feb 23, 2024

View reviewed changes

lib/web_ui/lib/src/engine/platform_dispatcher.dart Show resolved Hide resolved

ditman reviewed Feb 23, 2024

View reviewed changes

lib/web_ui/lib/src/engine/platform_dispatcher/view_focus_binding.dart Outdated Show resolved Hide resolved

ditman reviewed Feb 23, 2024

View reviewed changes

lib/web_ui/lib/src/engine/platform_dispatcher/view_focus_binding.dart Show resolved Hide resolved

tugorez requested a review from ditman February 23, 2024 04:17

tugorez changed the title ~~Marks the Flutter Views as focusable by setting a tabindex value.~~ Mark the Flutter Views as focusable by setting a tabindex value. Feb 23, 2024

yjbanov approved these changes Feb 23, 2024

View reviewed changes

lib/web_ui/lib/src/engine/platform_dispatcher/view_focus_binding.dart Outdated Show resolved Hide resolved

ditman reviewed Feb 27, 2024

View reviewed changes

lib/web_ui/lib/src/engine/platform_dispatcher/view_focus_binding.dart Show resolved Hide resolved

ditman reviewed Feb 27, 2024

View reviewed changes

lib/web_ui/lib/src/engine/platform_dispatcher/view_focus_binding.dart Show resolved Hide resolved

ditman reviewed Feb 27, 2024

View reviewed changes

ditman approved these changes Feb 27, 2024

View reviewed changes

tugorez added 7 commits March 7, 2024 07:11

Marks the flutter views as focusable.

deaf50b

When a given flutter view is focused its tabindex will be -1 When a given flutter view is not focused its tabindex will be 0

Apply reviewer suggestions

6f69db8

Apply reviewer suggestions

87ddd10

Apply reviewer suggestions

db97ed8

Make it a mixin

e40ddd6

Revert mixin changes

f3ba20d

Prepend the accesibility placeholder

78731aa

tugorez added 2 commits March 7, 2024 07:11

Do not mark the view as focusable when semantics are enabled

6905458

Fix license file

314e4b6

tugorez added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

merge

1e576f1

tugorez added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

tugorez added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

auto-submit bot merged commit 767c25d into flutter:main Mar 8, 2024

engine-flutter-autoroll mentioned this pull request Mar 9, 2024

Roll Flutter Engine from 953927eae78b to 7541e902f1b4 (7 revisions) flutter/flutter#144862

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 9, 2024

767c25d83 Mark the Flutter Views as focusable by setting a tabindex v…

c900e28

…alue. (flutter/engine#50876)

Mark the Flutter Views as focusable by setting a tabindex value. #50876

Mark the Flutter Views as focusable by setting a tabindex value. #50876

Uh oh!

Conversation

tugorez commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

ditman Feb 23, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman commented Feb 24, 2024

Uh oh!

tugorez commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

tugorez commented Feb 28, 2024

Uh oh!

ditman commented Feb 29, 2024

Uh oh!

tugorez commented Feb 29, 2024

Uh oh!

ditman commented Mar 6, 2024

Uh oh!

tugorez commented Mar 8, 2024

Uh oh!

auto-submit bot commented Mar 8, 2024

Uh oh!

auto-submit bot commented Mar 8, 2024

Uh oh!

ditman commented Mar 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tugorez commented Feb 22, 2024 •

edited

Loading

ditman Feb 23, 2024 •

edited

Loading

tugorez commented Feb 26, 2024 •

edited

Loading