Copied Apple's semantics for switches, made checkboxes the same. #16211

gaaclarke · 2020-01-29T23:08:07Z

Started delegating accessibility calls to a real UISwitch in order to get the same behavior from flutter switches as UISwitch with respect to VoiceOver.

goderbauer · 2020-01-29T23:29:14Z

If I focus on a native switch in iOS it does says something like "on/off, double tap to toggle setting". It doesn't say "0/1, Button".

0 or 1 does not seem like an appropriate announcement for a switch.

gaaclarke · 2020-01-30T01:34:14Z

If I focus on a native switch in iOS it does says something like "on/off, double tap to toggle setting". It doesn't say "0/1, Button".

0 or 1 does not seem like an appropriate announcement for a switch.

It absolutely does, I'm not just making stuff up Michael, lol. See the attached XCode project and Accessibility Inspector screenshot. If it says anything else that is a customization of the semantics from the app developer. We can't make it say anything else unless we want to tackle localization of "on" and "off". It makes more sense that an app developer would handle that, that's why I only supply "1" and "0" if a value isn't already specified.

Switch.zip

gaaclarke · 2020-01-30T18:15:26Z

@goderbauer and I looked at this on device, Accessibility Inspector and the device report different results.

chinmaygarde · 2020-01-30T19:30:11Z

looked at this on device, Accessibility Inspector and the device report different results.

Wonderful.

gaaclarke · 2020-01-30T19:30:32Z

@goderbauer I made the changes we talked about in person, please review.

gaaclarke · 2020-01-30T19:32:40Z

Also, I kept the old logic in. It shouldn't get hit because FlutterSwitchSemanticsObject will override those calls, but it is more correct and safer in case something breaks.

chinmaygarde · 2020-01-30T19:49:22Z

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

+/// A proxy class for SemanticsObject and UISwitch.  For most Accessibility and
+/// SemanticsObject methods it delegates to the semantics object, otherwise it
+/// sends messages to the UISwitch.
+@interface FlutterSwitchSemanticsObject : UISwitch


I definitely lack some context here which is making it hard for me to review this. Why is this object a switch subclass? It is not going to be ever used as a switch. That is, it is not in the view hierarchy and you have already overridden most of ally methods. If you wanted to delegate to the base classes implementation of the a11y methods, the responses will be based on a switch in its default state (and not how it appears in the Flutter view hierarchy). Towards that end, shouldn't it be on the implementation to override all the a11y methods as necessary here?

Again. I could be missing something but the base class and the forwarding I don't get the point of.

No problem, I can explain. There is magic that happens on iOS for UISwitch with respect to VoiceOver. If you look at the results of a UISwitch's accessibility methods, it doesn't match up to what VoiceOver actually says. In order to get the magic the SemanticsObject has to be a UISwitch (it has undocumented methods or there is some reflection happening). We do something similar for text fields.

goderbauer · 2020-01-30T19:56:52Z

Can you update the PR description with what's actually announced for a switch now?

goderbauer · 2020-01-30T20:03:28Z

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

+    return @([self node].value.data());
+  }
+
+  if ([self node].HasFlag(flutter::SemanticsFlags::kHasToggledState) ||


Can we remove this code and just continue to use the UIAccessibilityTraitSelected as a fallback? (That code should also get a comment explaining why it should never trigger)

What was there was wrong for 3 reasons:

'Selected' has wholly different semantics for users of VoiceOver

The VoiceOver never explained the negative state, only the positive

There was no indication that you could interact with the object

The way things are setup this should never happen, but {false: ("Autosave", "0", "Button") true: ("Autosave", "1", "Button")} is more informative and correct than {false:("Autosave") true:{"Selected", "Autosave"}

Added comments.

goderbauer · 2020-01-30T20:04:19Z

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

@@ -765,6 +848,13 @@ - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction {
        !node.HasFlag(flutter::SemanticsFlags::kIsReadOnly)) {
      // Text fields are backed by objects that implement UITextInput.
      object = [[[TextInputSemanticsObject alloc] initWithBridge:GetWeakPtr() uid:uid] autorelease];
+    } else if (node.HasFlag(flutter::SemanticsFlags::kHasToggledState) ||


You'll probably also have to add some code below to handle the case where a SemanticsNode switches from having these flags to not having them and vice versa.

Can that happen that a node will have kHasToggledState and then it doesn't? That seems like it would be immutable in practice since a new SemanticsObject would get generated for a new entity that is no longer a switch. Are we recycling SemanticsObjects?

While unlikely, the framework supports switching the role of a SemanticsObject, that's why we have the code below for text fields.

Ok, I think it's pretty weird to accommodate for something that shouldn't happen, but done.

gaaclarke · 2020-01-30T20:41:02Z

Can you update the PR description with what's actually announced for a switch now?

Done

gaaclarke · 2020-02-03T23:00:35Z

@chinmaygarde Can you give this another glance? I had to make a few changes for @goderbauer and he would like someone with objc chops to review it.

goderbauer

LG, but please get somebody an ObjC expert to take a look as well.

goderbauer · 2020-02-03T23:10:42Z

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

+      if (DidFlagChange(object.node, node, flutter::SemanticsFlags::kIsTextField) ||
+          DidFlagChange(object.node, node, flutter::SemanticsFlags::kIsReadOnly) ||
+          DidFlagChange(object.node, node, flutter::SemanticsFlags::kHasCheckedState) ||
+          DidFlagChange(object.node, node, flutter::SemanticsFlags::kHasToggledState)) {
        // The node changed its type from text field to something else, or vice versa. In this


Update this comment? It's not just text fields.

cbracken

gaaclarke · 2020-02-05T18:13:22Z

landing on red (appears to be a fuchsia flake)

…me. (flutter/engine#16211)

* 9347c93 Roll src/third_party/dart c8ed304e979a..3414b5167554 (52 commits) (flutter/engine#16362) * 16cd6f0 Roll fuchsia/sdk/core/mac-amd64 from 6h3IH... to Ke00Y... (flutter/engine#16360) * 8c6cc65 Fix runtime type errors when running with canvaskit (flutter/engine#16312) * 677b563 Refactor of Vulkan GPUSurface code (flutter/engine#16224) * 7ca44d3 Kill the test harness if any test exceeds a timeout. (flutter/engine#16349) * 7f6149c Revert "Remove use of the deprecated AccessibilityNodeInfo boundsInPa… (flutter/engine#16355) * 488f489 Roll fuchsia/sdk/core/linux-amd64 from Tszo5... to VJv0H... (flutter/engine#16363) * 7c9b11a Roll src/third_party/skia 71ce449d2814..2aee7d24da8f (5 commits) (flutter/engine#16364) * 7e1d144 Expose enable-service-port-fallback switch (flutter/engine#16366) * 4cda916 Expose the dart kernel snapshot target and copied assets as a public dependency (flutter/engine#16266) * 73c5130 Roll src/third_party/dart 3414b5167554..68e904e444dc (17 commits) (flutter/engine#16367) * 1cd8f3b Fix and consolidate wstring conversion utils (flutter/engine#16342) * b98a39e Add docs (flutter/engine#16368) * e3e6de2 Roll buildroot to c44791c89d2b51bfce864ab2cf5228d41ece1fcc (flutter/engine#16371) * e24ec59 Fuchsia a11y actions (flutter/engine#16321) * d8400c9 Roll src/third_party/skia 2aee7d24da8f..14d64afaa8a3 (10 commits) (flutter/engine#16374) * eeabd4d Roll src/third_party/dart 68e904e444dc..48808f7dce81 (17 commits) (flutter/engine#16377) * 22b994c Roll fuchsia/sdk/core/mac-amd64 from Ke00Y... to ubThi... (flutter/engine#16378) * 0471f44 Roll src/third_party/skia 14d64afaa8a3..6c9b1fd6663e (7 commits) (flutter/engine#16380) * 852d824 Roll src/third_party/dart 48808f7dce81..34893faa6079 (7 commits) (flutter/engine#16381) * 4aa2083 Roll src/third_party/skia 6c9b1fd6663e..bc3307c395e2 (1 commits) (flutter/engine#16383) * 036c370 Copied Apple's semantics for switches, made checkboxes the same. (flutter/engine#16211) * c107969 fix build_id logic for fuchsia symbols (flutter/engine#16397) * 11b7704 [fuchsia] Migrate flutter runner to use Present2 (flutter/engine#14162) * 646ec35 Update license output (flutter/engine#16379) * 925c60b Fix Windows embedding issues caught by clang (flutter/engine#16369) * 31bf3e1 Roll src/third_party/dart 34893faa6079..b3396cbdcae1 (36 commits) (flutter/engine#16422) * 8f89bac [web] Fixes incorrect transform when context save and transforms are deferred. (flutter/engine#16412) * f34bc65 use percent for golden diff rates; tighten the values (flutter/engine#16430)

…tter#16211)

…me. (flutter#16211)" This reverts commit 584c02c.

Copied Apple's semantics for switches, made checkboxes the same.

430eeea

auto-assign bot requested a review from chinmaygarde January 29, 2020 23:08

googlebot added cla: yes labels Jan 29, 2020

gaaclarke requested a review from goderbauer January 29, 2020 23:11

Moved logic around so that a value field can override what is said.

ef4dc40

removed trailing whitespace

c517767

chinmaygarde approved these changes Jan 30, 2020

View reviewed changes

gaaclarke added 2 commits January 30, 2020 11:26

Made a proxy UISwitch so we get UISwitch's VoiceOver on device.

9f74d5b

Moved FlutterSwitchSemanticsObjectDown

c3e136b

chinmaygarde self-requested a review January 30, 2020 19:30

formatted code

1e33b4c

chinmaygarde reviewed Jan 30, 2020

View reviewed changes

goderbauer reviewed Jan 30, 2020

View reviewed changes

gaaclarke added 2 commits January 30, 2020 12:50

Added some informative comments.

949d57a

Started handling the changing of semanticsobjects

6e8c07b

gaaclarke requested a review from goderbauer January 30, 2020 21:44

Merge branch 'master' of github.com:flutter/engine into switch-semantics

5f28b12

goderbauer reviewed Feb 3, 2020

View reviewed changes

Updated a comment, added an assert.

b4703d0

cbracken approved these changes Feb 4, 2020

View reviewed changes

Merge branch 'master' of github.com:flutter/engine into switch-semantics

649e9b0

gaaclarke merged commit 036c370 into flutter:master Feb 5, 2020

engine-flutter-autoroll mentioned this pull request Feb 5, 2020

Roll engine 81dffd1241e6..c107969ecd4e (23 commits) flutter/flutter#50217

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2020

036c370 Copied Apple's semantics for switches, made checkboxes the sa…

58a1177

…me. (flutter/engine#16211)

gaaclarke mentioned this pull request Feb 5, 2020

Switches and Checkboxes on iOS have accessibility issues flutter/flutter#49259

Closed

engine-flutter-autoroll mentioned this pull request Feb 5, 2020

Roll engine 81dffd1241e6..11b770489f1e (24 commits) flutter/flutter#50227

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2020

036c370 Copied Apple's semantics for switches, made checkboxes the sa…

74aabe5

…me. (flutter/engine#16211)

engine-flutter-autoroll mentioned this pull request Feb 6, 2020

Roll engine 81dffd1241e6..925c60b38bad (26 commits) flutter/flutter#50238

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020

036c370 Copied Apple's semantics for switches, made checkboxes the sa…

4de1c8d

…me. (flutter/engine#16211)

engine-flutter-autoroll mentioned this pull request Feb 6, 2020

Roll engine 81dffd1241e6..8f89bac4b785 (28 commits) flutter/flutter#50250

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020

036c370 Copied Apple's semantics for switches, made checkboxes the sa…

051b28a

…me. (flutter/engine#16211)

engine-flutter-autoroll mentioned this pull request Feb 6, 2020

Roll engine 81dffd1241e6..f34bc65bee49 (29 commits) flutter/flutter#50255

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 6, 2020

036c370 Copied Apple's semantics for switches, made checkboxes the sa…

491fceb

…me. (flutter/engine#16211)

NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Copied Apple's semantics for switches, made checkboxes the same. (flu…

584c02c

…tter#16211)

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "Copied Apple's semantics for switches, made checkboxes the sa…

8205d9e

…me. (flutter#16211)" This reverts commit 584c02c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copied Apple's semantics for switches, made checkboxes the same. #16211

Copied Apple's semantics for switches, made checkboxes the same. #16211

gaaclarke commented Jan 29, 2020 •

edited

Loading

goderbauer commented Jan 29, 2020

gaaclarke commented Jan 30, 2020

gaaclarke commented Jan 30, 2020

chinmaygarde commented Jan 30, 2020

gaaclarke commented Jan 30, 2020

gaaclarke commented Jan 30, 2020 •

edited

Loading

chinmaygarde Jan 30, 2020

gaaclarke Jan 30, 2020

goderbauer commented Jan 30, 2020

goderbauer Jan 30, 2020

gaaclarke Jan 30, 2020

gaaclarke Jan 30, 2020

goderbauer Jan 30, 2020

gaaclarke Jan 30, 2020

goderbauer Jan 30, 2020

gaaclarke Jan 30, 2020

gaaclarke commented Jan 30, 2020

gaaclarke commented Feb 3, 2020

goderbauer left a comment

goderbauer Feb 3, 2020

gaaclarke Feb 3, 2020

cbracken left a comment

gaaclarke commented Feb 5, 2020

Copied Apple's semantics for switches, made checkboxes the same. #16211

Copied Apple's semantics for switches, made checkboxes the same. #16211

Conversation

gaaclarke commented Jan 29, 2020 • edited Loading

goderbauer commented Jan 29, 2020

gaaclarke commented Jan 30, 2020

gaaclarke commented Jan 30, 2020

chinmaygarde commented Jan 30, 2020

gaaclarke commented Jan 30, 2020

gaaclarke commented Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Jan 30, 2020

gaaclarke commented Feb 3, 2020

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

gaaclarke commented Feb 5, 2020

gaaclarke commented Jan 29, 2020 •

edited

Loading

gaaclarke commented Jan 30, 2020 •

edited

Loading