Reland: Request focus if accessibility focus is given to a Focus widget (#142942) #149840
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
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
a93cbd9 to
82ef474
Compare
jmagman
reviewed
Jun 7, 2024
| // node will gain focus and take focus from this widget. | ||
| // TODO(gspencergoog): Allow this to be set on iOS once the issue is debugged. | ||
| // Issue: https:// | ||
| onFocus: defaultTargetPlatform != TargetPlatform.iOS && _couldRequestFocus |
Member
There was a problem hiding this comment.
Does this issue reproduce on Android? I'm not sure if it's iOS-only, or if there's only a test on iOS so that's where it was caught.
Contributor
Author
There was a problem hiding this comment.
As far as I can tell, no, but as you say, there are no tests on Android that test this. The failure mode seems iOS specific (I doubt Android would implement their text field identically to iOS to the point where it has a similar effect on the bounding box of the native text field), but I suppose something different could happen on Android.
I could also exclude Android explicitly, however, if you think that it's likely enough to worry about.
Member
There was a problem hiding this comment.
🤷♀️ Not sure if this is anything to worry about. If it's anywhere else it may actually be on macOS, which this web/desktop feature would be targeting. I was trying to replicate this test on macOS, but I was hitting some wacky accessibility problems that tricked the XCUITests.
yjbanov
approved these changes
Jun 12, 2024
| // for losing the focus because if focus is lost, that means another | ||
| // node will gain focus and take focus from this widget. | ||
| // TODO(gspencergoog): Allow this to be set on iOS once the issue is debugged. | ||
| // Issue: https:// |
Reland: Request focus if SemanticsAction.focus is sent to a focusable widget (flutter#142942)
82ef474 to
c04ee56
Compare
c04ee56 to
8397ff9
Compare
jmagman
approved these changes
Jun 12, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jun 13, 2024
…cus widget (#142942) (flutter/flutter#149840)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jun 13, 2024
…cus widget (#142942) (flutter/flutter#149840)
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Jun 13, 2024
flutter/flutter@b1f9d71...01db23b 2024-06-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7fcbfce608f to 4cb3025d3abf (28 revisions) (flutter/flutter#150199) 2024-06-13 dkwingsmt@users.noreply.github.com Revert "[CupertinoActionSheet] Add sliding tap gesture" (flutter/flutter#150147) 2024-06-13 hans.muller@gmail.com RawScrollbar: don't listen for drag gestures when scrolling is not possible (flutter/flutter#149925) 2024-06-13 katelovett@google.com Update testowners (flutter/flutter#150141) 2024-06-12 109111084+yaakovschectman@users.noreply.github.com Revert "Reland 2: [CupertinoActionSheet] Match colors to native" (flutter/flutter#150142) 2024-06-12 dkwingsmt@users.noreply.github.com Reland 2: [CupertinoActionSheet] Match colors to native (flutter/flutter#150129) 2024-06-12 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.6 to 4.1.7 (flutter/flutter#150132) 2024-06-12 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.8 to 3.25.9 (flutter/flutter#150133) 2024-06-12 15619084+vashworth@users.noreply.github.com Improve build time when using SwiftPM (flutter/flutter#150052) 2024-06-12 gspencergoog@users.noreply.github.com Reland: Request focus if accessibility focus is given to a Focus widget (#142942) (flutter/flutter#149840) 2024-06-12 52160996+FMorschel@users.noreply.github.com Update WidgetStatesController docs (flutter/flutter#150081) 2024-06-12 tessertaha@gmail.com [Reland] Fix `SegmentedButton` clipping when drawing segments (#149739) (flutter/flutter#150090) 2024-06-12 nate.w5687@gmail.com Fix markdown hyperlinks in the style guide (flutter/flutter#150071) 2024-06-12 magder@google.com Update packages desktop PR triage link lables (flutter/flutter#150124) 2024-06-12 victorsanniay@gmail.com Add mouse cursor property to `CupertinoRadio` (flutter/flutter#149681) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
victorsanni
pushed a commit
to victorsanni/flutter
that referenced
this pull request
Jun 14, 2024
…et (flutter#142942) (flutter#149840) ## Description This attempts to re-land flutter#142942 after being reverted in flutter#149741 because it broke the iOS [platform view UI integration test](https://github.com/flutter/flutter/blob/master/dev/integration_tests/ios_platform_view_tests/ios/PlatformViewUITests/PlatformViewUITests.m?rgh-link-date=2024-06-06T19%3A47%3A27Z). The changes here from the original are that in the Focus widget we no longer set the `onFocus` for the `Semantics` if the platform is iOS. It was not intended to do anything on iOS anyhow. Also, I updated the matchers to not actually do anything yet with the SemanticsAction.focus matching, so that this can be landed without breaking customer tests, and once they have been updated to correctly look for the focus action, we can land a PR that will turn it on. ## Related Issues - flutter#149838 - flutter#83809 - flutter#149842 ## Tests - Updated framework tests to look for the appropriate things using the matchers, even though it doesn't actually test for them yet.
victorsanni
pushed a commit
to victorsanni/flutter
that referenced
this pull request
Jun 14, 2024
…et (flutter#142942) (flutter#149840) ## Description This attempts to re-land flutter#142942 after being reverted in flutter#149741 because it broke the iOS [platform view UI integration test](https://github.com/flutter/flutter/blob/master/dev/integration_tests/ios_platform_view_tests/ios/PlatformViewUITests/PlatformViewUITests.m?rgh-link-date=2024-06-06T19%3A47%3A27Z). The changes here from the original are that in the Focus widget we no longer set the `onFocus` for the `Semantics` if the platform is iOS. It was not intended to do anything on iOS anyhow. Also, I updated the matchers to not actually do anything yet with the SemanticsAction.focus matching, so that this can be landed without breaking customer tests, and once they have been updated to correctly look for the focus action, we can land a PR that will turn it on. ## Related Issues - flutter#149838 - flutter#83809 - flutter#149842 ## Tests - Updated framework tests to look for the appropriate things using the matchers, even though it doesn't actually test for them yet.
auto-submit bot
pushed a commit
to flutter/engine
that referenced
this pull request
Jun 28, 2024
This is a repeat of #53134, which was merged prematurely. > [!WARNING] > Only land this after: > * flutter/flutter#149840 lands in the framework. > * You have personally manually tested the change together with the latest framework on all browsers. ## Original PR description Stop using `SemanticsAction.didGain/LoseAccessibilityFocus` on the web, start using `SemanticsAction.focus`. This is because on the web, a11y focus is not observable, only input focus is. Sending `SemanticsAction.focus` will guarantee that the framework move focus to the respective widget. There currently is no "unfocus" signal, because it seems to be already covered: either another widget gains focus, or an HTML DOM element outside the Flutter view does, both of which have their respective signals already. More details in the discussion in the issue flutter/flutter#83809. Fixes flutter/flutter#83809 Fixes flutter/flutter#148285 Fixes flutter/flutter#143337
auto-submit bot
pushed a commit
that referenced
this pull request
Jul 8, 2024
## Description This re-enables the `SemanticsAction.focus` matchers so that they actually do something now instead of ignoring the action. This was so that we could land the focus action changes without causing breakages in customer tests, and now that customer tests have been updated, we can land this PR turning it on again. ## Related Issues - Fixes #149842 ## Related PRs - #149840 ## Tests - Updates semantics tests to actually look for the focus action.
victorsanni
pushed a commit
to victorsanni/flutter
that referenced
this pull request
Jul 8, 2024
## Description This re-enables the `SemanticsAction.focus` matchers so that they actually do something now instead of ignoring the action. This was so that we could land the focus action changes without causing breakages in customer tests, and now that customer tests have been updated, we can land this PR turning it on again. ## Related Issues - Fixes flutter#149842 ## Related PRs - flutter#149840 ## Tests - Updates semantics tests to actually look for the focus action.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 6, 2024
…cus widget (#142942) (flutter/flutter#149840)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Description
This attempts to re-land #142942 after being reverted in #149741 because it broke the iOS platform view UI integration test.
The changes here from the original are that in the Focus widget we no longer set the
onFocusfor theSemanticsif the platform is iOS. It was not intended to do anything on iOS anyhow.Also, I updated the matchers to not actually do anything yet with the SemanticsAction.focus matching, so that this can be landed without breaking customer tests, and once they have been updated to correctly look for the focus action, we can land a PR that will turn it on.
Related Issues
onFocuscallback forSemanticsAction.focuson iOS. #149838hasFocusActionandSematicsAction.focusin matchers. #149842Tests