Support scrolling in iOS accessibility #26671

chunhtai · 2021-06-09T23:40:51Z

Fixes scrolling issues once and for all
Use a UIScrollView to represent scroll view in iOS accessibility.

fixes flutter/flutter#80711
fixes flutter/flutter#37974

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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai · 2021-06-10T18:58:28Z

This makes scrollable work natively in iOS, this however makes the scrollable focusable in voiceover. I think this is ok because that matches the UIScrollView in native App if you want it to be scrollable.

gaaclarke · 2021-06-10T22:51:23Z

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

+- (void)accessibilityBridgeDidFinishUpdate;
+
+/** Should only be called in conjunction with setting child/parent relationship. */
+- (void)setParent:(SemanticsObject*)parent;


Instead of providing this method, switch the property away from readonly and make sure the memory semantics are clear i.e. is this retaining a weak or strong reference?

gaaclarke · 2021-06-10T22:53:39Z

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

+/// iOS.
+@interface FlutterScrollableSemanticsObject : UIScrollView
+
+- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject;


Add NS_UNAVAILABLE init and NS_DESIGNATED_INITIALIZER for this initializer to force people to use this initializer.

The initWithSemanticsObject still uses init, I can't NS_UNAVAILABLE the init?

nvm i got it

gaaclarke · 2021-06-10T22:55:52Z

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

+  return globalTransform;
+}
+
+void ApplyTransform(SkPoint& point, const SkM44& transform) {


Instead of mutating the parameter, just return a new SkPoint. That leads to more clear usage.

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

gaaclarke · 2021-06-10T23:02:27Z

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

 @end  // FlutterSwitchSemanticsObject

+@implementation FlutterScrollableSemanticsObject {
+  SemanticsObject* _semanticsObject;


Please move this to a property so the memory semantics are explicit.

gaaclarke · 2021-06-10T23:05:24Z

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

+}
+
+- (id)accessibilityContainer {
+  if (_container == nil)


Add curly braces please https://google.github.io/styleguide/objcguide.html#conditionals

gaaclarke · 2021-06-10T23:08:21Z

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

-/** Should only be called in conjunction with setting child/parent relationship. */
- (void)privateSetParent:(SemanticsObject*)parent;


Why get rid of this flimsy protection?

the parent become a public property, do we still want to keep this?

I would. I admit it is flimsy but it at least brings attention to it.

gaaclarke · 2021-06-10T23:15:51Z

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


 void AccessibilityBridge::VisitObjectsRecursivelyAndRemove(SemanticsObject* object,
                                                           NSMutableArray<NSNumber*>* doomed_uids) {
+  [object accessibilityBridgeDidFinishUpdate];


The name of this method doesn't seem to match to me with where it is being called. If i had to name it I'd call it accessibilityBridgeWillVisitObjectsRecursivelyAndRemove, know what I mean?

The accessibilityBridgeWillVisitObjectsRecursivelyAndRemove name is vague and is hard to interpret. I put it in VisitObjectsRecursivelyAndRemove because I don't want to do another tree walk just to call this method.

The problem with what it is called now is that is leaking context information about where VisitObjectsRecursivelyAndRemove is called into calls higher in the call stack. Would it make more sense to move that to where VisitObjectsRecursivelyAndRemove is being called? Maybe you could make a method like this:

void finishUpdating(bridge, object) { doomed_uids = calculateDoomedUids(); [object accessibilityBridgeDidFinishUpdate]; bridge->VisitObjectsRecursivelyAndRemove(object, doomed_uids); }

That would make the name make more sense.

chunhtai · 2021-06-11T18:25:31Z

@gaaclarke PTAL :)

gaaclarke · 2021-06-14T17:58:55Z

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

+- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject {
+  self = [super initWithFrame:CGRectZero];
+  if (self) {
+    self.semanticsObject = semanticsObject;


You should use _semanticsObject = [semanticsObject retain]; You aren't suppose to reference properties through self in init methods.

gaaclarke

LGTM modulo those 2 comments.

This reverts commit e56dd3a.

…ter#26803) This reverts commit e56dd3a.

Support scrolling in iOS accessibility

9657e16

This comment has been minimized.

Sign in to view

flutter-dashboard bot added the platform-ios label Jun 9, 2021

google-cla bot added the cla: yes label Jun 9, 2021

chunhtai added 3 commits June 9, 2021 16:43

format

427131d

add tests

eab7353

update

5eccfde

chunhtai requested a review from gaaclarke June 10, 2021 18:55

gaaclarke reviewed Jun 10, 2021

View reviewed changes

This was referenced Jun 10, 2021

adding support for ios custom action overrides when switch control is enabled #20566

Closed

add customaction label override for ios flutter/flutter#82746

Closed

addressing comments

1752bb7

chunhtai force-pushed the issues/80711 branch from 02affe7 to 1752bb7 Compare June 11, 2021 18:23

chunhtai requested a review from gaaclarke June 11, 2021 18:25

gaaclarke reviewed Jun 14, 2021

View reviewed changes

gaaclarke approved these changes Jun 14, 2021

View reviewed changes

addressing comments'

f3987f2

chunhtai 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 Jun 14, 2021

fluttergithubbot merged commit e56dd3a into flutter:master Jun 14, 2021

engine-flutter-autoroll mentioned this pull request Jun 14, 2021

Roll Engine from e1d2d4b2643c to cb82d5e7b8a9 (4 revisions) flutter/flutter#84605

Closed

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

e56dd3a Support scrolling in iOS accessibility (flutter/engine#26671)

14f804f

engine-flutter-autoroll mentioned this pull request Jun 15, 2021

Roll Engine from e1d2d4b2643c to 13be5be6c072 (6 revisions) flutter/flutter#84607

Closed

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

e56dd3a Support scrolling in iOS accessibility (flutter/engine#26671)

648ba80

engine-flutter-autoroll mentioned this pull request Jun 15, 2021

Roll Engine from e1d2d4b2643c to e0011fa561d6 (8 revisions) flutter/flutter#84613

Merged

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

e56dd3a Support scrolling in iOS accessibility (flutter/engine#26671)

9226605

chunhtai added a commit to chunhtai/engine that referenced this pull request Jun 17, 2021

Revert "Support scrolling in iOS accessibility (flutter#26671)"

fe39db0

This reverts commit e56dd3a.

chunhtai added a commit that referenced this pull request Jun 17, 2021

Revert "Support scrolling in iOS accessibility (#26671)" (#26803)

86dacc0

This reverts commit e56dd3a.

nocduro mentioned this pull request Jun 20, 2021

add PageView cacheExtent flutter/flutter#42107

Closed

9 tasks

wqyfavor pushed a commit to wqyfavor/engine that referenced this pull request Jun 21, 2021

Revert "Support scrolling in iOS accessibility (flutter#26671)" (flut…

ad7595e

…ter#26803) This reverts commit e56dd3a.

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Support scrolling in iOS accessibility (flutter#26671)

a3e553d

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

Revert "Support scrolling in iOS accessibility (flutter#26671)" (flut…

fff41a0

…ter#26803) This reverts commit e56dd3a.

		/** Should only be called in conjunction with setting child/parent relationship. */
		- (void)privateSetParent:(SemanticsObject*)parent;

Support scrolling in iOS accessibility #26671

Support scrolling in iOS accessibility #26671

Uh oh!

Conversation

chunhtai commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

This comment has been minimized.

chunhtai commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jun 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chunhtai commented Jun 9, 2021 •

edited

Loading

chunhtai commented Jun 10, 2021 •

edited

Loading