This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[web] - fix Safari textfield selection bug #47917
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mdebbar
reviewed
Nov 10, 2023
| // order to calculate the correct pointer event offsets. | ||
| // See: https://github.com/flutter/flutter/issues/136006 | ||
| if(textEditing.strategy is SafariDesktopTextEditingStrategy) { | ||
| mainTextEditingElement.style.pointerEvents = 'all'; |
Contributor
There was a problem hiding this comment.
I have two questions here:
- Do we need to set this back to
noneafter the input loses focus? - Should we preemptively set
pointer-events: allfor all inputs inEngineAutofillForm.fromFrameworkMessage()?
Contributor
Author
There was a problem hiding this comment.
- We only need to ensure that it remains
noneif it remains on the screen and another input on the form is focused. This is being automatically handled for us infromFrameworkMessageinsideEngineAutofillForm. This happens when we click any other input in the form. The other case where it loses focus is where we just click anywhere outside of the input and the form. In this case, the input gets moved offscreen so we don't have to worry about it. - We can't do this because of the Safari autofill fix where we need all the non-active inputs to be on screen but hidden from the user. We actually preemptively disable pointer events for non active inputs in
EngineAutofillForm.fromFrameworkMessage()so that we don't have pointer collisions if users inadvertently hover/click over those hidden inputs
Contributor
There was a problem hiding this comment.
Okay that makes sense. Thanks for explaining this!
mdebbar
approved these changes
Nov 13, 2023
| // order to calculate the correct pointer event offsets. | ||
| // See: https://github.com/flutter/flutter/issues/136006 | ||
| if(textEditing.strategy is SafariDesktopTextEditingStrategy) { | ||
| mainTextEditingElement.style.pointerEvents = 'all'; |
Contributor
There was a problem hiding this comment.
Okay that makes sense. Thanks for explaining this!
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 13, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 13, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 13, 2023
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 13, 2023
…138375) flutter/engine@db6da00...046ec85 2023-11-13 [email protected] Fix fuchsia upload symbols. (flutter/engine#47938) 2023-11-13 [email protected] Roll Skia from 1a39bd56cda2 to 9ac5cad143ec (3 revisions) (flutter/engine#47984) 2023-11-13 [email protected] [web] - fix Safari textfield selection bug (flutter/engine#47917) 2023-11-13 [email protected] Roll Skia from 17b6555a1551 to 1a39bd56cda2 (2 revisions) (flutter/engine#47980) 2023-11-13 [email protected] Add a libcxxabi module that provides support for C++ thread-local storage (flutter/engine#47886) 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to fix Safari autofill, we've had to give inactive elements non-zero size because Safari does not respect offscreen or 0-sized inputs, and this leads to broken autofill behavior (see: #43058).
As part of these changes, we needed to disable pointer events for the parent
<form>element so that we don't have pointer event collisions if users hover over or click into the invisible autofill elements within that form.This led to an issue where offsets weren't being calculated correctly for "active" inputs because they relied on pointer events bubbling up and being caught by the form. The fix is to explicitly set pointer events on the active inputs, so that we can correctly discern when our pointer event target is actually the input and correctly calculate the offsets.
Fixes flutter/flutter#136006
Pre-launch Checklist
///).