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

Add support for exposing accessibility identifier as resource-id on Android #47961

Merged
merged 15 commits into from
Dec 4, 2023
Merged

Add support for exposing accessibility identifier as resource-id on Android #47961

merged 15 commits into from
Dec 4, 2023

Conversation

bartekpacia
Copy link
Member

@bartekpacia bartekpacia commented Nov 12, 2023

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 flutter/flutter#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 to learn more about this approach).

Steps:
part 1: [engine] add SemanticsUpdateBuilderNew <-- we are here
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 #49139

I'd like to do these changes first, and only then continue with the proper framework PR.

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

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.

@bartekpacia bartekpacia changed the title Feature/android add accessibility identifier Add support for exposing accessibility identifier as resource-id on Android Nov 12, 2023
@bartekpacia
Copy link
Member Author

bartekpacia commented Nov 13, 2023

Demo of a basic example

Used framework commit: flutter/flutter@cdfd94c from PR flutter/flutter#138331

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: Center(
          child: Semantics(
            identifier: 'some-blue-container',
            label: 'A blue container',
            child: Container(
              width: 100,
              height: 100,
              color: Colors.blue,
            ),
          ),
        ),
      ),
    );
  }
}

Interesting part of uiautomator dump result:

<node index="0" text="" resource-id="some-blue-container" class="android.view.View" package="com.example.example" content-desc="A blue container" checkable="false" checked="false" clickable="false" enabled="true" focusable="true" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[409,1006][671,1268]" />
Full uiautomator dump result
<?xml version='1.0' encoding='UTF-8' standalone='yes' ?>
<hierarchy rotation="0">
    <node index="0" text="" resource-id="" class="android.widget.FrameLayout" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
        <node index="0" text="" resource-id="" class="android.widget.LinearLayout" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
            <node index="0" text="" resource-id="android:id/content" class="android.widget.FrameLayout" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
                <node index="0" text="" resource-id="" class="android.widget.FrameLayout" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="true" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
                    <node index="0" text="" resource-id="" class="android.view.View" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
                        <node index="0" text="" resource-id="" class="android.view.View" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
                            <node index="0" text="" resource-id="" class="android.view.View" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
                                <node index="0" text="" resource-id="" class="android.view.View" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,2274]">
                                    <node index="0" text="" resource-id="some-blue-container" class="android.view.View" package="com.example.example" content-desc="A blue container" checkable="false" checked="false" clickable="false" enabled="true" focusable="true" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[409,1006][671,1268]" />
                                </node>
                            </node>
                        </node>
                    </node>
                </node>
            </node>
        </node>
        <node index="1" text="" resource-id="android:id/statusBarBackground" class="android.view.View" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][1080,63]" />
        <node index="2" text="" resource-id="android:id/navigationBarBackground" class="android.view.View" package="com.example.example" content-desc="" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[0,0][0,0]" />
    </node>
</hierarchy>

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Nov 14, 2023
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.

Do you have iOS pr?

lib/ui/semantics.dart Outdated Show resolved Hide resolved
@@ -872,6 +876,7 @@ base class _NativeSemanticsUpdateBuilder extends NativeFieldWrapperClass1 implem
required double elevation,
required double thickness,
required Rect rect,
String? identifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

why nullable? can this be required?

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh I'm not sure. I took inspiration from tooltip which is also a raw string, not List<StringAttribute>, and it is nullable.

But in the native semantics layer (at least on Android) the node's resource-id cannot be null, only empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

it was probably an oversight or maybe due to easier migration. Can you try to make it required and not nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in c50bcee.

Should I do the same for tooltip (probably in a separate PR)?

lib/ui/semantics/semantics_update_builder.cc Outdated Show resolved Hide resolved
lib/ui/semantics/semantics_update_builder.cc Outdated Show resolved Hide resolved
@bartekpacia
Copy link
Member Author

Thank you for the review @chunhtai! This is not quite ready but I appreciate it. Also I'll ping you once it is ready.

Do you have iOS pr?

Not yet.

@bartekpacia
Copy link
Member Author

bartekpacia commented Nov 21, 2023

@chunhtai

Do you have iOS pr?

Should I implement the iOS part in the same PR as Android part, or can these changes be made in different PRs?

@chunhtai
Copy link
Contributor

I would suggest open a separate pr for iOS part and people can review entire thing as a whole.

@chunhtai
Copy link
Contributor

chunhtai commented Nov 21, 2023

Also can this apply to desktop?

@bartekpacia
Copy link
Member Author

Also can this apply to desktop?

On macOS – probably, it's pretty similar to mobile iOS. I don't know about other platforms though. But right now I'd like to bring this feature to mobile.

@bartekpacia
Copy link
Member Author

This PR requires changing the SemanticsUpdateBuilder interface (defined in engine) that framework depends on. So it will require introducing a temporary API (see question I asked on Discord).

@bartekpacia bartekpacia marked this pull request as ready for review November 23, 2023 22:28
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.

Just some nit picking. Otherwise LGTM, please only merge when all the relevant PRs are approved

@@ -872,6 +876,7 @@ base class _NativeSemanticsUpdateBuilder extends NativeFieldWrapperClass1 implem
required double elevation,
required double thickness,
required Rect rect,
String? identifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

it was probably an oversight or maybe due to easier migration. Can you try to make it required and not nullable?

lib/ui/semantics.dart Outdated Show resolved Hide resolved
@bartekpacia
Copy link
Member Author

please only merge when all the relevant PRs are approved

Could you clarify this? This PR is actually the first one that has to land. I described the plan in the PR description (link). (Also this is my first time doing a gradual migration like this, I'd appreciate you looking if that plan makes sense).

@chunhtai
Copy link
Contributor

I meant you should have all prs approved, usually the framework and engine pr. We don't want to merge the engine pr and figure out the the code need to be updated due to addressing comment in the framework pr. It would also help to reduce the time that the transitional code lives in the code base

Would be good to have clean up pr up as well, but that is required if it is hard to do.

@bartekpacia
Copy link
Member Author

Understood. So I'm waiting until the framework PR gets green light. I responded to your comments there btw.

Would be good to have clean up pr up as well, but that is required if it is hard to do.

I don't think that's necessary - cleanup PRs are going to be simple since SemanticsUpdateBuilder is used in 7 places in flutter/flutter code (excluding tests).

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 4, 2023
@auto-submit auto-submit bot merged commit c6395d9 into flutter:main Dec 4, 2023
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2023
@bartekpacia bartekpacia deleted the feature/android_add_accessibility_identifier branch December 4, 2023 20:19
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 4, 2023
…139497)

flutter/engine@eee8aeb...c6395d9

2023-12-04 barpac02@gmail.com Add support for exposing accessibility identifier as resource-id on Android (flutter/engine#47961)
2023-12-04 skia-flutter-autoroll@skia.org Roll Skia from 540d76ea74f8 to db4a29e0689e (2 revisions) (flutter/engine#48633)

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 matanl@google.com,rmistry@google.com,zra@google.com 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
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 11, 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](flutter/engine#47961).

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`*
part 4: [flutter] use (now updated) `SemanticsUpdateBuilder` again.
part 5: [engine] remove `SemanticsBuilderNew`
auto-submit bot pushed a commit that referenced this pull request Dec 11, 2023
…ntifier on iOS (#48858)

This PR is a sibling of #47961 but for iOS
auto-submit bot pushed a commit 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`
fluttermirroringbot pushed a commit to flutter/flutter 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 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
auto-submit bot pushed a commit that referenced this pull request Dec 20, 2023
This PR is inspired by [the suggestion made here](#47961 (comment)).
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-android platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter's semantics system doesn't allow for setting accessibility ID for UI testing
2 participants