Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[web] switch from .didGain/LoseAccessibilityFocus to .focus #53360

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Jun 12, 2024

This is a repeat of #53134, which was merged prematurely.

Warning

Only land this after:

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

@yjbanov yjbanov requested a review from mdebbar June 12, 2024 17:55
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jun 12, 2024
@kevmoo
Copy link
Contributor

kevmoo commented Jun 13, 2024

@yjbanov the framework fix landed! 😄

@yjbanov
Copy link
Contributor Author

yjbanov commented Jun 16, 2024

@kevmoo Thanks for the ping. I've been testing this, and I think we'll need another framework fix before landing this.

@kevmoo
Copy link
Contributor

kevmoo commented Jun 25, 2024

@yjbanov – which framework fix are we waiting on?

@yjbanov
Copy link
Contributor Author

yjbanov commented Jun 27, 2024

@yjbanov – which framework fix are we waiting on?

flutter/flutter#150648. It's breaking g3. I'm working on a g3fix. But I'm still going to need to manually retest after landing the framework PR, before this can go in.

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 28, 2024
@auto-submit auto-submit bot merged commit ad1343c into flutter:main Jun 28, 2024
31 checks passed
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 29, 2024
…151016)

flutter/engine@f828363...ad1343c

2024-06-28 [email protected] [web] switch from .didGain/LoseAccessibilityFocus to .focus (flutter/engine#53360)
2024-06-28 [email protected] Reland [DisplayList] Add support for clipOval to leverage Impeller optimization (flutter/engine#53642)
2024-06-28 [email protected] [Impeller] experimental canvas bdf support. (flutter/engine#53597)

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
@jiahaog
Copy link
Member

jiahaog commented Jul 2, 2024

Reason for revert: Breaking google3 in b/350131288.

There is a test that does something like the following, to check if a radio button is selected.

      // Send a bunch of tabs to focus on the correct radio button
      await tester.sendKeyEvent(LogicalKeyboardKey.tab);
      await tester.pump();
      await tester.sendKeyEvent(LogicalKeyboardKey.tab);
      await tester.pump();
      await tester.sendKeyEvent(LogicalKeyboardKey.tab);
      await tester.pump();

      // Toggle the radio button with space
      await tester.sendKeyEvent(LogicalKeyboardKey.space);
      await tester.pump();

      final selectedRadio =
          tester.widget<Radio<bool>>(find.byType(Radio<bool>).at(1));
      expect(selectedRadio.value, isTrue);

After this commit, the above test fails. See the linked bug above for more details.

@jiahaog jiahaog added the revert Label used to revert changes in a closed and merged pull request. label Jul 2, 2024
Copy link
Contributor

auto-submit bot commented Jul 2, 2024

Time to revert pull request flutter/engine/53360 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jul 2, 2024
jiahaog added a commit that referenced this pull request Jul 2, 2024
auto-submit bot pushed a commit that referenced this pull request Jul 2, 2024
…53679)

Reverts #53360

Breaking google3 in b/350131288.

There is a test that does something like the following, to check if a radio button is selected.

```dart
      // Send a bunch of tabs to focus on the correct radio button
      await tester.sendKeyEvent(LogicalKeyboardKey.tab);
      await tester.pump();
      await tester.sendKeyEvent(LogicalKeyboardKey.tab);
      await tester.pump();
      await tester.sendKeyEvent(LogicalKeyboardKey.tab);
      await tester.pump();

      // Toggle the radio button with space
      await tester.sendKeyEvent(LogicalKeyboardKey.space);
      await tester.pump();

      final selectedRadio =
          tester.widget<Radio<bool>>(find.byType(Radio<bool>).at(1));
      expect(selectedRadio.value, isTrue);
```

After this commit, the above test fails. See the linked bug above for more details.
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
…lutter#151016)

flutter/engine@f828363...ad1343c

2024-06-28 [email protected] [web] switch from .didGain/LoseAccessibilityFocus to .focus (flutter/engine#53360)
2024-06-28 [email protected] Reland [DisplayList] Add support for clipOval to leverage Impeller optimization (flutter/engine#53642)
2024-06-28 [email protected] [Impeller] experimental canvas bdf support. (flutter/engine#53597)

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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
4 participants