Add on/off accessibility labels to CupertinoSwitch #39993

brandondiamond · 2019-09-06T17:42:23Z

Description

This PR adds accessibility labels to CupertinoSwitch. These labels match their counterparts in iOS 13. When the switch is active, a vertical "|" fades in; when inactive, a vertical "O" fades in.

Experimentation with iOS indicates that these icons are not typographical. They do not respond to text size or language changes. However, like the switch in general, the labels are sensitive to text direction. This PR ensures that the labels are rendered on the correct side of the switch.

This change will be implemented in two stages. The engine change (flutter/engine#12404) is currently in review. Meanwhile, this change implements the UI and updates tests. Once the engine change rolls out this PR will be ready to merge. Once merged, accessibility labels will be enabled automatically.

Related Issues

#4830: add accessibility labels to CupertinoSwitch.
#33797: update CupertinoSwitch to match iOS 13 styling.

Tests

I added the following tests:

Sharing code change first to confirm that this approach is acceptable :)
If so, I will add new golden tests to verify the appearance of the labels before, during, and after interaction. I will also add equivalent tests for RTL text direction.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

Images

Measurements

Normal Speed

⅕ Speed

cc @LongCatIsLooong @johnsonmh @willlarche

fluttergithubbot · 2019-09-06T17:42:24Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

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

brandondiamond · 2019-09-06T17:45:32Z

packages/flutter/lib/src/cupertino/switch.dart

@@ -144,6 +144,7 @@ class _CupertinoSwitchState extends State<CupertinoSwitch> with TickerProviderSt
        onChanged: widget.onChanged,
        vsync: this,
        dragStartBehavior: widget.dragStartBehavior,
+        enableOnOffLabels: MediaQuery.onOffSwitchLabelsEnabled(context)


I switched (pun semi-intended) the position of the word "enabled" so as to avoid suggesting that this method somehow enables on/off labels. That said, I'm worried that having a slightly different name may be confusing. What do you think?

I also remove the word "switch" from the equivalent parameter to RenderCupertinoSwitch since it seems redundant. Not sure if it's better to use the exact same name ("enableOnOffSwitchLabels") everywhere or to adapt for intuitiveness, as I've tried to do.

jonahwilliams · 2019-09-06T17:46:30Z

@brandondiamond this will need tests. Some good cases to start with:

When this setting is enabled, the labels are drawn correctly for on/off.
When this setting is disabled, the labels are not drawn
Changing the setting adjusts what is drawn

jonahwilliams · 2019-09-06T17:48:52Z

packages/flutter/lib/src/widgets/media_query.dart

@@ -827,6 +842,12 @@ class MediaQuery extends InheritedWidget {
    return MediaQuery.of(context, nullOk: true)?.boldText ?? false;
  }

+  /// Returns the enableOnOffSwitchLabels accessibility setting for the nearest
+  /// MediaQuery ancestor, or false if no such ancestor exists.
+  static bool onOffSwitchLabelsEnabled(BuildContext context) {


I wouldn't add any new APIs to media query

I added this comment to the original issue, probably belongs here. I agree with Jonah.

I think it would be best if we interested a new InheritedWidget in the cupertino library, rather than extending MediaQuery et al. Most of the features exposed by MediaQuery are more or less cross-platform. This "on/off labels flag" is specific to iOS and there are more flags like it. Instead of extending MediaQuery you could create a similar widget in the cupertino library called, say, CupertinoSystemSettings.

Sounds awesome -- thanks for the detail. One question: if I go that route, would the AccessibilityFeatures interface still be best for passing the bit from the platform? (I see a similar iOS-only bit managed there: "reduceMotion")

Yes good point: It looks like AccessibilityFeatures is the right place to plumb this value through from the engine. WidgetsApp already rebuilds when AccessibilityFeatures changes. That should be good enough for now, I think we can forgo creating an inherited CupertinoSystemSettings widget.

LongCatIsLooong · 2019-09-07T00:22:11Z

packages/flutter/lib/src/cupertino/colors.dart

+  /// enabled.
+  ///
+  /// Value derived from a screenshot of a switch with on/off labeling.
+  static const Color contrastingGray = Color(0xFFB8B8B8);


Is this a widely used color that is worth exposing? Also the color value I got from inspecting the native component using Xcode is 0xFFB3B3B3 (White:0.7 Alpha:1).

I don't think so -- I just felt guilty adding a color constant to switch.dart 😜

If that seems reasonable, I'll remove the constant from here and add it to the constants section (as 0xFFB3B3B3 -- I used the OS X color meter utility, but I think you're 100% right that Xcode is more accurate).

brandondiamond · 2019-09-25T20:11:18Z

Goldens change (sorry for the ugly merge):
flutter/goldens@7eaaa4e

brandondiamond · 2019-09-25T20:12:46Z

Engine change is in review, too: flutter/engine#12404

brandondiamond · 2019-09-25T20:22:12Z

Confirmed that the labels do not change color in light/dark mode (they definitely look different, but I believe that's just an optical illusion).

fluttergithubbot · 2019-09-25T20:24:02Z

It looks like this pull request includes a golden file change. Please make sure to follow Handling Breaking Changes. While there are exceptions to this rule, if this patch modifies an existing golden file, it is probably not an exception. Only new golden files are not considered breaking changes.

Writing a golden file test for package:flutter may also provide guidance for this change.

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

Piinks · 2019-09-25T20:27:41Z

It looks like this pull request includes a golden file change. Please make sure to follow Handling Breaking Changes. While there are exceptions to this rule, if this patch modifies an existing golden file, it is probably not an exception. Only new golden files are not considered breaking changes.

These all look to be new golden files. Disregard the breaking change request. :)

brandondiamond · 2019-09-26T18:16:59Z

Engine change has now been merged. Once it rolls out in the next few hours, this should be good to go!

justinmc · 2019-09-26T22:20:45Z

@brandondiamond Heads up that there appear to be some legitimate things in the analyze check that's failing.

brandondiamond · 2019-09-27T04:43:33Z

Thanks for the review, Justin! It looks like the errors are likely a result of building with the old engine. (My engine PR is blocked on flutter/infra#62 which should hopefully get resolved today). Once that rolls out, I'll re-run the checks to make sure all is okay.

justinmc · 2019-09-27T17:37:52Z

Ah got it, sounds good.

Piinks · 2019-10-29T20:58:26Z

I know this may still be on hold, but for when it is ready:
#43371 has changed golden file testing, which will need a few minor adjustments here. When you resolve the merge conflicts, the goldens.version file will no longer exist.
It looks like this change is all new golden files, so aside from resolving the merge conflict, there should only be the step of triaging the results when they land on master.

When this is ready for an update, take a look at the updated Writing a golden file test for package:flutter, and let me know if I can help migrate this change to the new system. :)

cbracken · 2020-01-09T19:18:32Z

packages/flutter/lib/src/cupertino/switch.dart

@@ -147,6 +147,7 @@ class _CupertinoSwitchState extends State<CupertinoSwitch> with TickerProviderSt
        onChanged: widget.onChanged,
        vsync: this,
        dragStartBehavior: widget.dragStartBehavior,
+        enableOnOffLabels: SemanticsBinding.instance.accessibilityFeatures.onOffSwitchLabels,


Rather than use this directly, it's better to add this field to MediaQueryData, populate the data there from the binding (take a look at how we do this for accessibleNavigation) then use the field from MediaQueryData here.

Hixie · 2020-01-09T19:41:59Z

You should have a test that verifies that the rendering changes correctly if the value reported by the engine changes.

goderbauer · 2020-02-24T22:32:05Z

@brandondiamond Do you have plans to come back to this PR and address the feedback?

brandondiamond · 2020-02-24T22:36:26Z

Hey Michael! Yup -- I do. (I had to wind down my 20% for a bit but will tackle this after hours; I'll try to get to it this week or, at the very latest, next week)

flutter-dashboard · 2020-09-21T16:19:04Z

This pull request has not been updated in a while. Please update this pull request to receive results from Gold, or close it.

Piinks · 2020-09-22T19:49:20Z

This has not been updated in a long while, so I am going to close it for now. If you would like to return to this, feel free to re-open! Thanks!

Add on/off labels to switches, and stub/accessor in MediaQueryData.

bdb239a

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Sep 6, 2019

googlebot added the cla: yes label Sep 6, 2019

brandondiamond commented Sep 6, 2019

View reviewed changes

jonahwilliams reviewed Sep 6, 2019

View reviewed changes

LongCatIsLooong reviewed Sep 7, 2019

View reviewed changes

brandondiamond mentioned this pull request Sep 23, 2019

Support accessibility labels on iOS switches. flutter/engine#12404

Merged

brandondiamond changed the title ~~Add on/off accessibility labels to CupertinoSwitch (Part 1: Framework)~~ Add on/off accessibility labels to CupertinoSwitch Sep 23, 2019

brandondiamond added 4 commits September 23, 2019 15:44

Initial work integrating with engine changes.

8be98c0

Merge branch 'master' into switch-labels

934d575

Switch to accessibility flags from flutter/engine#12404

93c326a

Merge branch 'master' into switch-labels

854ccbb

justinmc self-requested a review September 24, 2019 21:57

brandondiamond added 2 commits September 25, 2019 14:39

Sync with darkmode changes.

33756f0

Add test + bump goldens version

fead877

brandondiamond requested a review from Piinks as a code owner September 25, 2019 20:10

brandondiamond requested a review from LongCatIsLooong September 25, 2019 20:14

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests c: API break Backwards-incompatible API changes will affect goldens Changes to golden files labels Sep 25, 2019

Piinks removed the c: API break Backwards-incompatible API changes label Sep 25, 2019

cbracken reviewed Jan 9, 2020

View reviewed changes

jmagman added this to Awaiting triage in iOS Platform - Cupertino widget review Feb 25, 2020

jmagman added this to Awaiting triage in Mobile - accessibility review via automation Feb 25, 2020

jmagman removed this from Awaiting triage in iOS Platform - Cupertino widget review Feb 25, 2020

xster moved this from Awaiting triage to Engineer reviewed in Mobile - accessibility review Apr 29, 2020

jmagman removed this from Engineer reviewed in Mobile - accessibility review Apr 29, 2020

Piinks closed this Sep 22, 2020

gilnobrega mentioned this pull request May 28, 2023

[a11y] CupertinoSwitch On/Off labels #127776

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add on/off accessibility labels to CupertinoSwitch #39993

Add on/off accessibility labels to CupertinoSwitch #39993

brandondiamond commented Sep 6, 2019 •

edited

fluttergithubbot commented Sep 6, 2019

brandondiamond Sep 6, 2019

jonahwilliams commented Sep 6, 2019

jonahwilliams Sep 6, 2019

HansMuller Sep 6, 2019 •

edited

brandondiamond Sep 7, 2019

HansMuller Sep 11, 2019

LongCatIsLooong Sep 7, 2019

brandondiamond Sep 7, 2019

brandondiamond commented Sep 25, 2019

brandondiamond commented Sep 25, 2019

brandondiamond commented Sep 25, 2019

fluttergithubbot commented Sep 25, 2019

Piinks commented Sep 25, 2019 •

edited

brandondiamond commented Sep 26, 2019

justinmc commented Sep 26, 2019

brandondiamond commented Sep 27, 2019 •

edited

justinmc commented Sep 27, 2019

Piinks commented Oct 29, 2019

cbracken Jan 9, 2020 •

edited

Hixie commented Jan 9, 2020

goderbauer commented Feb 24, 2020

brandondiamond commented Feb 24, 2020

flutter-dashboard bot commented Sep 21, 2020

Piinks commented Sep 22, 2020

Add on/off accessibility labels to CupertinoSwitch #39993

Add on/off accessibility labels to CupertinoSwitch #39993

Conversation

brandondiamond commented Sep 6, 2019 • edited

Description

Related Issues

Tests

Checklist

Breaking Change

Images

Measurements

Normal Speed

⅕ Speed

fluttergithubbot commented Sep 6, 2019

brandondiamond Sep 6, 2019

Choose a reason for hiding this comment

jonahwilliams commented Sep 6, 2019

jonahwilliams Sep 6, 2019

Choose a reason for hiding this comment

HansMuller Sep 6, 2019 • edited

Choose a reason for hiding this comment

brandondiamond Sep 7, 2019

Choose a reason for hiding this comment

HansMuller Sep 11, 2019

Choose a reason for hiding this comment

LongCatIsLooong Sep 7, 2019

Choose a reason for hiding this comment

brandondiamond Sep 7, 2019

Choose a reason for hiding this comment

brandondiamond commented Sep 25, 2019

brandondiamond commented Sep 25, 2019

brandondiamond commented Sep 25, 2019

fluttergithubbot commented Sep 25, 2019

Piinks commented Sep 25, 2019 • edited

brandondiamond commented Sep 26, 2019

justinmc commented Sep 26, 2019

brandondiamond commented Sep 27, 2019 • edited

justinmc commented Sep 27, 2019

Piinks commented Oct 29, 2019

cbracken Jan 9, 2020 • edited

Choose a reason for hiding this comment

Hixie commented Jan 9, 2020

goderbauer commented Feb 24, 2020

brandondiamond commented Feb 24, 2020

flutter-dashboard bot commented Sep 21, 2020

Piinks commented Sep 22, 2020

brandondiamond commented Sep 6, 2019 •

edited

HansMuller Sep 6, 2019 •

edited

Piinks commented Sep 25, 2019 •

edited

brandondiamond commented Sep 27, 2019 •

edited

cbracken Jan 9, 2020 •

edited