Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Transfer focus to view rootElement on blur/remove. #55045

Merged
merged 9 commits into from
Nov 2, 2024

Conversation

tugorez
Copy link
Contributor

@tugorez tugorez commented Sep 9, 2024

The safeBlur/safeRemove/safeRemoveSync methods in the view manager should become the standard way to "blur" and "remove" elements within the web engine.

Calling these method ensures the blur operation doesn't disrupt the framework's view focus management because these methods transfer the focus from the current element to its containing EngineFlutterView's rootElement, so focus never abandons the Flutter view unless the user wants to.

This is a generalization of the former DefaultTextEditingStrategy.scheduleFocusFlutterView, which turns out is needed in anything that touches elements that may receive focus in the engine, not just text editing.

Issue

(Maybe) Part of flutter/flutter#157387
(Opportunistically) Fixes flutter/flutter#46638

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.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Sep 9, 2024
@tugorez tugorez force-pushed the semantics branch 3 times, most recently from 3c356a7 to ecebe3c Compare September 9, 2024 19:24
element.remove();

EnginePlatformDispatcher.instance.viewManager.safelyBlurElement(
element,
Copy link
Contributor

@yjbanov yjbanov Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

element is not always the target of the focus. See, for example,

/// The element that gets focus when [focusAsRouteDefault] is called.
.

Maybe what we need is have an abstract getter DomElement get focusTarget on the SemanticRole class, and then pass role.focusTarget to safelyBlurElement. Then each concrete SemanticRole class will implement the getter, returning the element that's meant to have focus.

delayed: false,
removeElement: true,
);

_parent = null;
semanticRole?.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some roles will remove their DOM children that may have focus in their implementation of dispose().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any test break. Is this a behavior that can/will be added later? Do I need to change other classes as well?

Apologies, I have zero experience with semantics.

@ditman
Copy link
Member

ditman commented Oct 23, 2024

I have created a small "repro" of this in JS so I can understand better what's happening. The goal is to move the focus away from the element and to the rootElement of the container view (if any) instead of calling .blur() on it.

This more or less prevents the focus from ever leaving the Flutter View (to the body), at least when the DOM modifications are performed in the engine.

As @tugorez alludes to in the PR, this method should be used every time we want to do ".blur()" or ".remove()" on elements that are focusable. I'm not sure what other parts of the engine this would need to be applied to, but worst case scenario we could potentially apply this to our implementations of .blur() and .remove() in the dom.dart file (that sounds too blunt to me though)

I have some changes to this PR (mainly shuffling code around, and prevent creating a new blur function every time we call the safelyBlurElement). I'm attempting to write/update some tests to verify it works as expected.

@ditman
Copy link
Member

ditman commented Oct 23, 2024

Some questions from an offline review:

  • (@mdebbar) How to prevent this regressing, do we want to make the default "remove/blur" method names "scary to use", and make this code part of the general "remove/blur" methods that we expose? (change dom.dart), so at least we force people to read docs and choose the right one?
  • (@yjbanov) Do we need a "target" element to move focus to (not only the flutter view?), or would the framework "blur this, then focus that" always?
  • When deleting element that contains activeElement we need to safely blur as well. FIXED.

tugorez and others added 3 commits October 24, 2024 17:58
…thod, instead of creating a closure every time.
Provide semantic method names instead of a bag of configuration:

* safeBlur (asynchronously transfer focus back to rootElement)
* safeRemove (asynchronously remove after transferring focus)
* safeRemoveSync (synchronously remove after transferring focus)
@ditman ditman changed the title Add a safelyBlurElement util. [web] Transfer focus to viewRoot on blur/remove. Oct 25, 2024
@ditman ditman changed the title [web] Transfer focus to viewRoot on blur/remove. [web] Transfer focus to view rootElement on blur/remove. Oct 25, 2024
@ditman
Copy link
Member

ditman commented Oct 25, 2024

Woops, some more tests broke when I flipped the element == activeElement check to element == activeElement || removeElement && element.contains(activeElement)

https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20Engine%20Drone/2834484/overview

* Ensures nothing starts focused before each test.
* Expects focus to be returned to the root of the flutter view after
  internal housekeping.
* Re-enables some tests in Firefox.
@ditman ditman marked this pull request as ready for review November 1, 2024 00:31
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if LGT @mdebbar

}

final String? viewIdAttribute = viewRoot.getAttribute(GlobalHtmlAttributes.flutterViewIdAttributeName);
assert(viewIdAttribute != null, 'Located Flutter view is missing its id attribute.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional checks ✅

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 1, 2024
@ditman
Copy link
Member

ditman commented Nov 1, 2024

autosubmit applied, I'm feeling lucky! :P

@auto-submit auto-submit bot merged commit b5a2319 into flutter:main Nov 2, 2024
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 4, 2024
…158127)

flutter/engine@05cb5d7...25c7e47

2024-11-04 [email protected] Retry mac_unopt one time in presubmit (flutter/engine#56319)
2024-11-04 [email protected] [Impeller] faster descriptor type mapping. (flutter/engine#56351)
2024-11-04 [email protected] Roll Skia from 75740b68a282 to e2ad60ea8039 (8 revisions) (flutter/engine#56354)
2024-11-04 [email protected] Move detection of cutouts in Android engine to `onApplyWindowInsets` (flutter/engine#55992)
2024-11-04 [email protected] Roll Dart SDK from 69cec5dc51f9 to f238183cf168 (1 revision) (flutter/engine#56353)
2024-11-04 [email protected] Roll Skia from bab7d954758b to 75740b68a282 (1 revision) (flutter/engine#56349)
2024-11-04 [email protected] Roll Fuchsia Linux SDK from 07KmbdEtnhkg_tUhe... to amgHXcqtplha8LuI_... (flutter/engine#56348)
2024-11-03 [email protected] Roll Skia from 6944cd128603 to bab7d954758b (2 revisions) (flutter/engine#56346)
2024-11-02 [email protected] Roll Dart SDK from 61bf0877807e to 69cec5dc51f9 (2 revisions) (flutter/engine#56335)
2024-11-02 [email protected] Multiple touches of a stylus should be considered from the same device (flutter/engine#56075)
2024-11-02 [email protected] Roll Skia from 89ac72bb4922 to 6944cd128603 (2 revisions) (flutter/engine#56331)
2024-11-02 [email protected] [web] Transfer focus to view rootElement on blur/remove. (flutter/engine#55045)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 07KmbdEtnhkg to amgHXcqtplha

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
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…ine#55045)

The `safeBlur`/`safeRemove`/`safeRemoveSync` methods in the view manager should become the standard way to "blur" and "remove" elements within the web engine.

Calling these method ensures the blur operation doesn't disrupt the framework's view focus management because these methods transfer the focus from the current element to its containing EngineFlutterView's `rootElement`, so focus never abandons the Flutter view unless the user wants to.

This is a generalization of the former `DefaultTextEditingStrategy.scheduleFocusFlutterView`, which turns out is needed in anything that touches elements that may receive focus in the engine, not just text editing.

## Issue

(Maybe) Part of flutter#157387
(Opportunistically) Fixes flutter#46638

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web][tests] Fix skipped firefox unit tests
4 participants