Use tristate checkbox engine changes by yaakovschectman · Pull Request #111032 · flutter/flutter

yaakovschectman · 2022-09-06T15:01:52Z

Sends the mixed checkbox state for tristate checkboxes to the engine in order to update semantics properties. Previously the half-checked/mixed/indeterminate state of a tristate checkbox was not read properly by screen readers.

Addresses #110107

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

yaakovschectman · 2022-09-06T15:42:56Z

Need to wait for next engine roll

loic-sharma · 2022-09-06T22:48:30Z

Just curious, when can this be null? If never, should this be non-nullable?

My thought process was to leave it to default to null when the widget has no checked state or is not tristate, like how checked is nullable and is not set when the widget has no check state.

I'd tweak the comment to clarify the non-null case as that's not readily apparent. Effectively: non-null if this is a checkbox, true if this is a tristate checkbox in the mixed state, false otherwise.

Perhaps something like:

If non-null, indicates that this subtree represents a checkbox or similar widget with a "checked" state. If true, this is a tristate checkbox in the mixed state.

This must be false when [Checkbox.tristate] is false.

loic-sharma · 2022-09-06T22:57:16Z

Should this modify SemanticsFlag.isChecked? If no, why not?

Also, should isChecked modify SemanticsFlag.isCheckStateMixed?

In all cases I can imagine, they should be mutually exclusive when true, so the question is just whether we want to enforce that internally in the SemanticsConfiguration

Gotcha. Consider adding assertions then: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-asserts-liberally-to-detect-contract-violations-and-verify-invariants

The thing is about asserting that exclusivity is that it would create an issue for the tests that ensure all flags are supported, as they work by setting all accessibility flags and making sure they are each reflected in the resulting semantics. If we do make this assertion, those tests would need to change in a manner that I'm not yet sure on how.

Agreed that generally we should be making liberal use of assertions to assert any invariants, especially since they're documented invariants.

Can you point to tests that would be broken by this? Could be it's the tests that need updating now that we're modelling new state.

cbracken · 2022-09-07T19:33:14Z

Agreed that generally we should be making liberal use of assertions to assert any invariants, especially since they're documented invariants.

Can you point to tests that would be broken by this? Could be it's the tests that need updating now that we're modelling new state.

yaakovschectman · 2022-09-07T19:37:41Z

@cbracken https://github.com/flutter/flutter/blob/394f762c0683491a1807158aa817bd5088697919/packages/flutter/test/widgets/custom_painter_test.dart#L417 and https://github.com/flutter/flutter/blob/394f762c0683491a1807158aa817bd5088697919/packages/flutter/test/widgets/semantics_test.dart#L552 would break with this, as they test by expecting a semantics node with all possible flags set. If two flags assert mutual exclusivity, we cannot possibly create a semantics node that matches that expectation. One option that may coexist with the assertions is to manually remove the mixed flag from the expected semantics to avoid the clash.

cbracken · 2022-09-07T19:52:20Z

Sounds like the test needs to be updated since all flags true is no longer a valid state.

yaakovschectman · 2022-09-07T19:54:32Z

@cbracken The latest commit addresses this, please take a look.

cbracken · 2022-09-07T20:09:06Z

Make the assert a precondition check at the top of the setter to prevent it from getting into a bad state if the user violates an invariant.

cbracken

Co-authored-by: Chris Bracken <[email protected]>

Typo fix in comment Co-authored-by: Loïc Sharma <[email protected]>

a-wallen · 2022-09-08T20:37:10Z

+          key: const Key('a'),
+          container: true,
+          explicitChildNodes: true,
+          // flags


Is this comment necessary?

Are you referring to //flags? This was already present in the earlier definitions in this test function, so it was copied to the modified instance created here.

yaakovschectman added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Sep 6, 2022

yaakovschectman requested review from a-wallen, cbracken, gspencergoog, guidezpl, loic-sharma and pdblasi-google September 6, 2022 15:01

yaakovschectman self-assigned this Sep 6, 2022

flutter-dashboard Bot added a: tests "flutter test", flutter_test, or one of our tests p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Sep 6, 2022

yaakovschectman force-pushed the tristate_tests branch 2 times, most recently from 1b82960 to 4f74f1b Compare September 6, 2022 20:44

yaakovschectman marked this pull request as ready for review September 6, 2022 21:01

loic-sharma reviewed Sep 6, 2022

View reviewed changes

guidezpl removed their request for review September 7, 2022 15:34

loic-sharma reviewed Sep 7, 2022

View reviewed changes

Comment thread packages/flutter/test/material/checkbox_test.dart Resolved

loic-sharma reviewed Sep 7, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/semantics/semantics.dart Resolved

yaakovschectman requested a review from loic-sharma September 7, 2022 19:05

cbracken reviewed Sep 7, 2022

View reviewed changes

yaakovschectman requested a review from cbracken September 8, 2022 17:48

cbracken approved these changes Sep 8, 2022

View reviewed changes

loic-sharma reviewed Sep 8, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/semantics/semantics.dart Resolved

loic-sharma approved these changes Sep 8, 2022

View reviewed changes

yaakovschectman and others added 9 commits September 8, 2022 16:03

Introduce tests for tristate checkboxes

08be2c3

Initial

477f544

Use tristate changes in engine

1ae439f

Flutter_test matchers test update

666c788

Comments, tests

691775a

Update packages/flutter/lib/src/semantics/semantics.dart

bff4902

Co-authored-by: Chris Bracken <[email protected]>

Assert mutual exclusivity

fe8aab9

Assert valid state before updating state

2940ca2

Update packages/flutter/lib/src/semantics/semantics.dart

904c05e

Typo fix in comment Co-authored-by: Loïc Sharma <[email protected]>

yaakovschectman force-pushed the tristate_tests branch from c7571b2 to 904c05e Compare September 8, 2022 20:03

a-wallen reviewed Sep 8, 2022

View reviewed changes

yaakovschectman merged commit 4d3c122 into flutter:master Sep 8, 2022

yaakovschectman deleted the tristate_tests branch September 8, 2022 21:42

yaakovschectman mentioned this pull request Sep 8, 2022

Semantics for Checkbox should react to the three-state mixed state #110107

Closed

This was referenced Sep 9, 2022

Roll Flutter from fc444aedab12 to ad946f8478bf (20 revisions) flutter/packages#2585

Merged

Roll Flutter from 66c306b45228 to ad946f8478bf (21 revisions) flutter-team-archive/plugins#6378

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2022

4d3c122 Use tristate checkbox engine changes (flutter/flutter#111032)

3b3afe0

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 9, 2022

4d3c122 Use tristate checkbox engine changes (flutter/flutter#111032)

b8a89ee

engine-flutter-autoroll mentioned this pull request Sep 9, 2022

Roll Flutter from 66c306b45228 to 4cd734ae3a1c (24 revisions) flutter-team-archive/plugins#6379

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 9, 2022

4d3c122 Use tristate checkbox engine changes (flutter/flutter#111032)

d83128b

jorgelrj mentioned this pull request Oct 16, 2026

[web] Tristate Checkbox with value null is exposed as aria-checked="false"; SemanticCheckable never writes "mixed" #194547

Closed

Conversation

yaakovschectman commented Sep 6, 2022

Pre-launch Checklist

Uh oh!

yaakovschectman commented Sep 6, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakovschectman commented Sep 7, 2022

Uh oh!

cbracken commented Sep 7, 2022

Uh oh!

yaakovschectman commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants