[ios_platform_view, a11y] Make `FlutterPlatformViewSemanticsContainer` a SemanticsObject. #29531

cyanglaz · 2021-11-04T19:24:23Z

This PR makes FlutterPlatformViewSemanticsContainer a SemanticsObject.
This change removed the reference to a SemanticsObject in the FlutterPlatformViewSemanticsContainer.
Now a FlutterPlatformViewSemanticsContainer only contains a single a11yElement, which is the platform view.
This resolve the retain cycle just like how #24308 tried to resolve.
This also avoid the protocol pattern introduced in #24308, which seems making platform view drop parent reference.

fixes flutter/flutter#93024

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 Hixie said 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.

flutter-dashboard · 2021-11-04T19:24:26Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

cyanglaz · 2021-11-04T20:01:07Z

Some old tests might be failing due to this change, I'll work on the tests later

jmagman · 2021-11-04T19:34:47Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.h

+- (instancetype)initWithBridge:(fml::WeakPtr<flutter::AccessibilityBridgeIos>)bridge
+                           uid:(int32_t)uid
+    __attribute__((unavailable("Use initWithBridge:uid:platformViewId instead")));


We should instead use NS_UNAVAILABLE and NS_DESIGNATED_INITIALIZER for this pattern.

Suggested change

- (instancetype)initWithBridge:(fml::WeakPtr<flutter::AccessibilityBridgeIos>)bridge

uid:(int32_t)uid

__attribute__((unavailable("Use initWithBridge:uid:platformViewId instead")));

- (instancetype)initWithBridge:(fml::WeakPtr<flutter::AccessibilityBridgeIos>)bridge

uid:(int32_t)uid NS_UNAVAILABLE;

jmagman · 2021-11-04T19:52:31Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm

-  if ([element isKindOfClass:[FlutterPlatformViewSemanticsContainer class]]) {
-    return ((FlutterPlatformViewSemanticsContainer*)element).index;
-  }


Nice, I found this pretty confusing when I was stepping through, part of the bug seemed to be that the index was set by one container, then accessed by another?
Can you update the description to explain exactly what the bug was, for posterity?

shell/platform/darwin/ios/framework/Source/SemanticsObject.h

Co-authored-by: Jenn Magder <[email protected]>

jmagman · 2021-11-04T20:06:22Z

Verified the bug no longer reproduces on this PR.

jmagman · 2021-11-05T01:10:24Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm

+- (instancetype)initWithBridge:(fml::WeakPtr<flutter::AccessibilityBridgeIos>)bridge
+                           uid:(int32_t)uid {
+  return [super init];


FAILED: obj/flutter/shell/platform/darwin/ios/framework/Source/flutter_framework_source.SemanticsObject.o /opt/s/w/ir/cache/goma/client/gomacc ../../buildtools/mac-x64/clang/bin/clang++ -MD -MF obj/flutter/shell/platform/darwin/ios/framework/Source/flutter_framework_source.SemanticsObject.o.d -DFLUTTER_FRAMEWORK=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DSHELL_ENABLE_SOFTWARE -DSHELL_ENABLE_GL -DSHELL_ENABLE_METAL -DSK_GL -DSK_METAL -DSK_CODEC_DECODES_JPEG -DSK_ENCODE_JPEG -DSK_CODEC_DECODES_PNG -DSK_ENCODE_PNG -DSK_CODEC_DECODES_WEBP -DSK_ENCODE_WEBP -DSK_HAS_WUFFS_LIBRARY -DSK_ENABLE_DUMP_GPU -DSK_DISABLE_AAA -DSK_PARAGRAPH_LIBTXT_SPACES_RESOLUTION -DSK_LEGACY_INNER_JOINS -DSK_DISABLE_LEGACY_SHADERCONTEXT -DSK_DISABLE_LOWP_RASTER_PIPELINE -DSK_FORCE_RASTER_PIPELINE_BLITTER -DSK_DISABLE_EFFECT_DESERIALIZATION -DSK_ENABLE_SKSL -DSK_ASSUME_GL_ES=1 -DSK_ENABLE_API_AVAILABLE -DFLUTTER_RUNTIME_MODE_DEBUG=1 -DFLUTTER_RUNTIME_MODE_PROFILE=2 -DFLUTTER_RUNTIME_MODE_RELEASE=3 -DFLUTTER_RUNTIME_MODE_JIT_RELEASE=4 -DDART_LEGACY_API=\[\[deprecated\]\] -DFLUTTER_RUNTIME_MODE=1 -DFLUTTER_JIT_RUNTIME=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DUSE_CHROMIUM_ICU=1 -DU_ENABLE_TRACING=1 -DU_ENABLE_RESOURCE_TRACING=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DRAPIDJSON_HAS_STDSTRING -DRAPIDJSON_HAS_CXX11_RANGE_FOR -DRAPIDJSON_HAS_CXX11_RVALUE_REFS -DRAPIDJSON_HAS_CXX11_TYPETRAITS -DRAPIDJSON_HAS_CXX11_NOEXCEPT -DFLUTTER_API_SYMBOL_PREFIX=Embedder -DFLUTTER_NO_EXPORT -I../.. -Igen -I../../third_party/skia -I../../third_party/vulkan/src -I../../third_party/vulkan/include -I../../flutter/third_party/txt/src -I../../third_party/harfbuzz/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/rapidjson -I../../third_party/rapidjson/include -I../../third_party/dart/runtime -I../../third_party/dart/runtime/include -I../../flutter/third_party -I../../flutter -I../../flutter/shell/platform/darwin/common/framework/Headers -isysroot /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.0.sdk -mios-simulator-version-min=9.0 -fno-strict-aliasing -arch x86_64 -fcolor-diagnostics -Wall -Wextra -Wendif-labels -Werror -Wno-missing-field-initializers -Wno-unused-parameter -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wno-implicit-int-float-conversion -Wno-c99-designator -Wno-deprecated-copy -Wno-psabi -Wno-non-c-typedef-for-linkage -Wno-range-loop-construct -Wunguarded-availability -Wno-deprecated-declarations -fvisibility=hidden -stdlib=libc++ -Wstring-conversion -Wnewline-eof -Os -fno-ident -fdata-sections -ffunction-sections -g2 -Werror=overriding-method-mismatch -Werror=undeclared-selector -fvisibility-inlines-hidden -fobjc-call-cxx-cdtors -std=c++17 -fno-rtti -fno-exceptions -nostdinc++ -isystem /opt/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1 -c ../../flutter/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm -o obj/flutter/shell/platform/darwin/ios/framework/Source/flutter_framework_source.SemanticsObject.o ../../flutter/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm:760:17: error: convenience initializer should not invoke an initializer on 'super' [-Werror,-Wobjc-designated-initializers] return [super init];

Other places in this file do this

// Method declared as unavailable in the interface - (instancetype)init { [self release]; [super doesNotRecognizeSelector:_cmd]; return nil; }

But it's not necessary since you've marked the method as unavailable. If you try to call it, you'll get a compilation error:

error: 'initWithBridge:uid:' is unavailable

So you can remove this -[initWithBridge:uid:] implementation

shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm

cyanglaz · 2021-11-05T03:26:54Z

I tried to run the tests locally and got:

Testing failed:
	/Users/ychris/flutter/engine/src/out/ios_debug_sim_unopt/libocmock_shared.dylib: No such file or directory
	/Users/ychris/flutter/engine/src/out/ios_debug_sim_unopt/libios_test_flutter.dylib: No such file or directory
	Testing cancelled because the build failed.

It is probably related to something in my local engine build since CI builds fine.
@gaaclarke Do you have any clue what might have happened?

jmagman · 2021-11-05T03:32:41Z

Include ios_test_flutter in your ninja call.

./flutter/tools/gn --no-goma --unoptimized --ios --simulator && ninja -C out/ios_debug_sim_unopt ios_test_flutter

jmagman · 2021-11-05T03:42:09Z

I tried to run the tests locally and got:
Testing failed:
	/Users/ychris/flutter/engine/src/out/ios_debug_sim_unopt/libocmock_shared.dylib: No such file or directory
	/Users/ychris/flutter/engine/src/out/ios_debug_sim_unopt/libios_test_flutter.dylib: No such file or directory
	Testing cancelled because the build failed.
It is probably related to something in my local engine build since CI builds fine. @gaaclarke Do you have any clue what might have happened?

engine/testing/run_tests.py

Line 297 in 541779d

def EnsureIosTestsAreBuilt(ios_out_dir):

should check if the ios_test_flutter artifacts are built too, not just ios_debug_sim_unopt. @cyanglaz can you file an issue?

cyanglaz · 2021-11-05T16:44:35Z

@jmagman Ready to review :)

jmagman

LGTM! Might be worth some extensive manual testing if you haven't already to see if there's any fallout to this.

jmagman · 2021-11-05T21:59:39Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm


  SemanticsObject* child = [_semanticsObject children][index - 1];

-  // Swap the original `SemanticsObject` to a `PlatformViewSemanticsContainer`


Why was this pattern used in the first place? #8156

I don't really remember a particular reason the old pattern was used. I don't think the new pattern was ever considered when I first implemented this.

dnfield · 2021-11-08T20:33:47Z

@cyanglaz can any of this be applied to the FlutterSemanticsScrollView implementation? It seems like there may be som eoverlap but I feel like I'm missing a piece.

…ontainer` a SemanticsObject. (flutter/engine#29531)

…` a SemanticsObject. (#29531) (#29725) Co-authored-by: Chris Yang <[email protected]>

chunhtai · 2021-11-18T17:21:54Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm

- (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction {
-  return [_platformView accessibilityScroll:direction];
+- (NSArray*)accessibilityElements {
+  return @[ _platformView ];


you can override the nativeAccessibility prop gettter to return the _platformView

Make PlatformViewSemanticsContainer a SemanticsObject

b97edf9

flutter-dashboard bot added the platform-ios label Nov 4, 2021

flutter-dashboard bot added the needs tests label Nov 4, 2021

google-cla bot added the cla: yes label Nov 4, 2021

cyanglaz requested review from chunhtai and jmagman November 4, 2021 19:24

jmagman reviewed Nov 4, 2021

View reviewed changes

Chris Yang and others added 2 commits November 4, 2021 13:04

Update shell/platform/darwin/ios/framework/Source/SemanticsObject.h

cbad648

Co-authored-by: Jenn Magder <[email protected]>

clean up

95a0e5b

revert

5329ba5

jmagman reviewed Nov 5, 2021

View reviewed changes

cyanglaz marked this pull request as draft November 5, 2021 02:00

Chris Yang added 2 commits November 4, 2021 19:25

update tests

b9f3fe2

bring back removed code

e0b52b8

tests

c5a4ec1

cyanglaz marked this pull request as ready for review November 5, 2021 16:43

Chris Yang added 2 commits November 5, 2021 09:54

update test

71d825d

Merge branch 'master' into platformview_a11y

b3163cb

jmagman approved these changes Nov 5, 2021

View reviewed changes

cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 8, 2021

fluttergithubbot merged commit 0b3ac94 into flutter:master Nov 8, 2021

cyanglaz mentioned this pull request Nov 8, 2021

Add integration test for iOS platform view accessibility. flutter/flutter#93272

Open

engine-flutter-autoroll mentioned this pull request Nov 9, 2021

Roll Engine from 1ae721c0230e to 0b3ac9431351 (1 revision) flutter/flutter#93294

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2021

0b3ac94 [ios_platform_view, a11y] Make `FlutterPlatformViewSemanticsC…

bd27502

…ontainer` a SemanticsObject. (flutter/engine#29531)

cyanglaz deleted the platformview_a11y branch November 9, 2021 01:40

stuartmorgan-g mentioned this pull request Nov 9, 2021

[google_maps_flutter] XCUITests stuck when handling permission popups flutter/flutter#93325

Closed

godofredoc added a commit that referenced this pull request Nov 13, 2021

[ios_platform_view, a11y] Make `FlutterPlatformViewSemanticsContainer…

f21711c

…` a SemanticsObject. (#29531) (#29725) Co-authored-by: Chris Yang <[email protected]>

jmagman mentioned this pull request Nov 18, 2021

Fixes the accessibilityContainer of FlutterScrollableSemanticsObject #29771

Merged

8 tasks

chunhtai reviewed Nov 18, 2021

View reviewed changes

cyanglaz mentioned this pull request Sep 28, 2023

[iOS][a11y] VoiceOver on UIKitView seems to be broken flutter/flutter#135504

Closed

2 tasks


		SemanticsObject* child = [_semanticsObject children][index - 1];

		// Swap the original `SemanticsObject` to a `PlatformViewSemanticsContainer`

[ios_platform_view, a11y] Make FlutterPlatformViewSemanticsContainer a SemanticsObject. #29531

[ios_platform_view, a11y] Make FlutterPlatformViewSemanticsContainer a SemanticsObject. #29531

Uh oh!

Conversation

cyanglaz commented Nov 4, 2021

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Nov 4, 2021

Uh oh!

cyanglaz commented Nov 4, 2021

Uh oh!

jmagman Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman commented Nov 4, 2021

Uh oh!

jmagman Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cyanglaz commented Nov 5, 2021

Uh oh!

jmagman commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented Nov 5, 2021

Uh oh!

cyanglaz commented Nov 5, 2021

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

cyanglaz Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield commented Nov 8, 2021

Uh oh!

chunhtai Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[ios_platform_view, a11y] Make `FlutterPlatformViewSemanticsContainer` a SemanticsObject. #29531

[ios_platform_view, a11y] Make `FlutterPlatformViewSemanticsContainer` a SemanticsObject. #29531

jmagman commented Nov 5, 2021 •

edited

Loading