Skip to content

Conversation

@bartekpacia
Copy link
Member

@bartekpacia bartekpacia commented Nov 12, 2023

This PR adds String? identifier to Semantics and SemanticsProperties. The identifier will be exposed on Android as resource-id and on iOS as accessibilityIdentifier.

Mainly targeted at #17988

Initial Engine PR with Android support: flutter/engine#47961
iOS Engine PR: flutter/engine#48858

Migration

This change breaks the SemanticsUpdateBuilder API which is on the Framework<-->Engine border. For more details see engine PR.

Steps:
part 1: [engine] add SemanticsUpdateBuilderNew flutter/engine#47961
part 2: [flutter] use SemanticsUpdateBuilderNew <-- we are here
part 3: [engine] update SemanticsUpdateBuilder to be the same as SemanticsUpdateBuilderNew flutter/engine#48882
part 4: [flutter] use (now updated) SemanticsUpdateBuilder again #139942
part 5: [engine] remove SemanticsBuilderNew flutter/engine#49139

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • 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 framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels Nov 12, 2023
@goderbauer goderbauer requested a review from chunhtai November 14, 2023 23:15
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

which accessibilitynodeinfo property would this new identifier set to? There doesn't seem to be a resourceId in the accessibilitynodeinfo https://developer.android.com/reference/android/view/accessibility/AccessibilityNodeInfo

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

ah it is viewIdResourceName just saw the engine pr

Copy link
Contributor

Choose a reason for hiding this comment

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

This doc seems a bit confusing. The UIAutomator interact with semantics node, not renderObject.

Copy link
Member Author

@bartekpacia bartekpacia Nov 17, 2023

Choose a reason for hiding this comment

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

Good catch, thank you. I'm still new to this code. Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM, just some nit picking

@bartekpacia
Copy link
Member Author

Thank you - I applied your suggestions.

auto-submit bot pushed a commit to flutter/engine that referenced this pull request Dec 4, 2023
…ndroid (#47961)

Accompanying framework PR: flutter/flutter#138331

This PR implements support for exposing `SemanticsProperties.identifier` on Android as `resource-id`. Mainly targeted at flutter/flutter#17988. Would also fix https://github.com/flutter/flutter/issues/issues/137735 but it was marked as duplicate. Anyway, there's a lot of context in that issue.

This PR requires changing the `SemanticsUpdateBuilder` interface (defined in engine) that framework depends on, so it requires introducing a temporary API ([see question I asked on Discord](https://discord.com/channels/608014603317936148/608018585025118217/1174845658033819729) to learn more about this approach).

Steps:
**part 1: [engine] add `SemanticsUpdateBuilderNew`** <-- we are here
part 2: [flutter] use `SemanticsUpdateBuilderNew`
part 3: [engine] update `SemanticsUpdateBuilder` to be the same as `SemanticsUpdateBuilderNew`*
part 4: [flutter] use (now updated) `SemanticsUpdateBuilder` again.
part 5: [engine] remove `SemanticsBuilderNew`

I'd like to do these changes first, and only then continue with [the proper framework PR](flutter/flutter#138331).

*More specifically: update `SemanticsUpdateBuilder.updateNode()` to be the same as `SemanticsUpdateBuilderNew.updateNode()`. Number of arguments that function takes is the only change.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@bartekpacia bartekpacia marked this pull request as ready for review December 5, 2023 08:41
@github-actions github-actions bot added the a: tests "flutter test", flutter_test, or one of our tests label Dec 6, 2023
@srawlins
Copy link
Contributor

srawlins commented Dec 9, 2023

@bartekpacia the latest "Linux customer_testing" bot failure says it grabbed devtools commit 914356dedc97b4bafc33cc3f9c0334fefbb3417f. Not sure why...

@bartekpacia
Copy link
Member Author

Yeah, indeed. At first I thought the commit is maybe pinned elsewhere but nope.

@bartekpacia bartekpacia added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 11, 2023
@auto-submit auto-submit bot merged commit 948523b into flutter:master Dec 11, 2023
@bartekpacia bartekpacia deleted the feature/accessibility_identifier branch December 11, 2023 18:04
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2023
auto-submit bot pushed a commit to flutter/engine that referenced this pull request Dec 11, 2023
This PR adds `String? identifier` to `SemanticsUpdateBuilder` (currently it's only available in the temproary `SemanticsUpdateBuilderNew` API.

This is mainly targeted at flutter/flutter#17988

Steps:
part 1: [engine] add `SemanticsUpdateBuilderNew` #47961
part 2: [flutter] use `SemanticsUpdateBuilderNew`  flutter/flutter#138331
**part 3: [engine] update `SemanticsUpdateBuilder` to be the same as `SemanticsUpdateBuilderNew`** <-- we are here
part 4: [flutter] use (now updated) `SemanticsUpdateBuilder` again.
part 5: [engine] remove `SemanticsBuilderNew`
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Dec 12, 2023
Manual roll requested by [email protected]

flutter/flutter@c642f4e...9719097

2023-12-11 [email protected] Roll Flutter Engine from 5587d26aa2d4 to 4c309195b79d (1 revision) (flutter/flutter#139936)
2023-12-11 [email protected] Fix SelectionArea select-word edge cases (flutter/flutter#136920)
2023-12-11 [email protected] Roll Flutter Engine from 9b85b76db0de to 5587d26aa2d4 (3 revisions) (flutter/flutter#139933)
2023-12-11 [email protected] Roll Flutter Engine from 7eb6b7cab60c to 9b85b76db0de (2 revisions) (flutter/flutter#139931)
2023-12-11 [email protected] Use dart analyze package for `num.clamp` (flutter/flutter#139867)
2023-12-11 [email protected] Roll pub packages (flutter/flutter#139926)
2023-12-11 [email protected] Handle the case when _CupertinoBackGestureDetector is disposed during the drag.  (flutter/flutter#139585)
2023-12-11 [email protected] Add accessibility identifier to `SemanticsProperties` (flutter/flutter#138331)
2023-12-11 [email protected] Improve slider's value indicator display test (flutter/flutter#139198)
2023-12-11 [email protected] Add `enabled` property to `ExpansionTile` (flutter/flutter#139519)
2023-12-11 [email protected] Roll Packages from 6cd0657 to cb6dbcd (9 revisions) (flutter/flutter#139911)
2023-12-11 [email protected] Roll Flutter Engine from bc0222b64c96 to 7eb6b7cab60c (1 revision) (flutter/flutter#139891)
2023-12-10 [email protected] Roll Flutter Engine from fb80aafd259b to bc0222b64c96 (1 revision) (flutter/flutter#139885)
2023-12-09 [email protected] Roll pub packages (flutter/flutter#139864)
2023-12-09 [email protected] Roll Flutter Engine from b75960a5820a to fb80aafd259b (1 revision) (flutter/flutter#139863)
2023-12-09 [email protected] Roll Flutter Engine from e80c090d09c6 to b75960a5820a (1 revision) (flutter/flutter#139853)
2023-12-09 [email protected] Roll Flutter Engine from 101396fd3b82 to e80c090d09c6 (2 revisions) (flutter/flutter#139851)
2023-12-09 [email protected] Roll Flutter Engine from 503584615fd7 to 101396fd3b82 (2 revisions) (flutter/flutter#139847)
2023-12-09 [email protected] Roll Flutter Engine from e9cb19fa637a to 503584615fd7 (1 revision) (flutter/flutter#139837)
2023-12-08 [email protected] Roll Flutter Engine from 7dc51b85a634 to e9cb19fa637a (1 revision) (flutter/flutter#139831)
2023-12-08 [email protected] [flutter release] Add cherry pick template for pull request description (flutter/flutter#139590)
2023-12-08 [email protected] Roll Flutter Engine from 03c5f016e919 to 7dc51b85a634 (1 revision) (flutter/flutter#139829)
2023-12-08 [email protected] Add Overlay.wrap for convenience (flutter/flutter#139823)
2023-12-08 [email protected] Roll Flutter Engine from 72da960e2ef2 to 03c5f016e919 (1 revision) (flutter/flutter#139826)
2023-12-08 [email protected] Roll Flutter Engine from 5dd2619c282b to 72da960e2ef2 (1 revision) (flutter/flutter#139821)

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 [email protected],[email protected],[email protected] 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
auto-submit bot pushed a commit that referenced this pull request Dec 16, 2023
…y `SemanticsUpdateBuilderNew` (#139942)

This PR removes all usages of the temporary `SemanticsUpdateBuilderNew` API in favor of `SemanticsUpdateBuilder`. These two APIs are the same as of now.

This is mainly targeted at #17988

Steps:
part 1: [engine] add `SemanticsUpdateBuilderNew` flutter/engine#47961
part 2: [flutter] use `SemanticsUpdateBuilderNew` #138331
part 3: [engine] update `SemanticsUpdateBuilder` to be the same as `SemanticsUpdateBuilderNew` flutter/engine#48882
**part 4: [flutter] use (now updated) `SemanticsUpdateBuilder` again** <-- we are here
part 5: [engine] remove `SemanticsBuilderNew`
auto-submit bot pushed a commit to flutter/engine that referenced this pull request Dec 18, 2023
)

This PR completest the migration by removing `SemanticsUpdateBuilderNew` from the engine.

This is mainly targeted at flutter/flutter#17988

Steps:
part 1: [engine] add `SemanticsUpdateBuilderNew` #47961
part 2: [flutter] use `SemanticsUpdateBuilderNew` flutter/flutter#138331
part 3: [engine] update `SemanticsUpdateBuilder` to be the same as `SemanticsUpdateBuilderNew` #48882
part 4: [flutter] use (now updated) `SemanticsUpdateBuilder` again flutter/flutter#139942
**part 5: [engine] remove `SemanticsBuilderNew`** <-- we are here
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants