Honor mixed state of tristate Checkbox in Semantics #35868

yaakovschectman · 2022-09-01T19:33:49Z

Flutter already supports tristate checkboxes that can have a mixed or half-checked state, but the semantics do not. Where the checked/unchecked state of a checkbox is passed to the Semantics node, a mixed state is not. This adds support for the mixed-state semantics flag to the engine. In conjunction with a PR to Flutter, we will be able to support the mixed state in semantics for checkboxes.

Addresses flutter/flutter#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 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.

lib/ui/semantics/semantics_node.h

shell/platform/common/accessibility_bridge.cc

shell/platform/windows/flutter_windows_view_unittests.cc

cbracken

Overall this approach lgtm. Adding @chunhtai for general a11y review.

yaakovschectman · 2022-09-02T14:31:28Z

I am going to need to push an interim PR to flutter/flutter to circumvent some tests that this change would break so that this change can be merged, which is necessary for those tests to be updated to be compatible with it.

lib/ui/semantics.dart

chunhtai · 2022-09-02T16:14:58Z

lib/ui/semantics.dart


+  /// Whether a tristate checkbox is in its mixed state.
+  ///
+  /// Should only be true when the checkbox has [tristate] = true.


this can still be false if [tristate] = true?

Also this comment is too specific, people who reading this doc may not know what tristate is.

Maybe

If this is true, the check box this semantics node represents is in a mixed state. For example, a [Checkbox] with [Checkbox.tristate] set to true can have checked, unchecked, or mixed state.

chunhtai · 2022-09-02T16:19:12Z

lib/web_ui/lib/semantics.dart

  static const SemanticsFlag hasToggledState = SemanticsFlag._(_kHasToggledStateIndex);
  static const SemanticsFlag isToggled = SemanticsFlag._(_kIsToggledIndex);
  static const SemanticsFlag hasImplicitScrolling = SemanticsFlag._(_kHasImplicitScrollingIndex);
+  static const SemanticsFlag isMixedCheck = SemanticsFlag._(_kIsMixedCheckIndex);


We may need a better name. maybe isCheckStateMixed

Maybe isMixedCheckState? But it's a bit at odds with existing naming convention typically hasFooState and isFoo.

cbracken · 2022-09-02T22:32:06Z

lib/ui/semantics.dart

+  /// can have checked,  unchecked, or mixed state.
+  ///
+  /// Should be false when the checkbox is either checked or unchecked.
+  ///


Suggested change

///

cbracken · 2022-09-02T22:35:27Z

shell/platform/windows/flutter_windows_view_unittests.cc

+    EXPECT_FALSE(native_state.lVal & STATE_SYSTEM_CHECKED);
+  }
+
+  // Now check mixed state


Super pedantic nit: end with a period (here and below). Look like at least one comment above was already missing one :/

Ref: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-correct-grammar

Suggested change

// Now check mixed state

// Now check mixed state.

cbracken · 2022-09-02T22:35:39Z

shell/platform/windows/flutter_windows_view_unittests.cc

+    IAccessible* native_view = root_node->GetNativeViewAccessible();
+    ASSERT_TRUE(native_view != nullptr);
+
+    // Look up against the node itself (not one of its children)


Suggested change

// Look up against the node itself (not one of its children)

// Look up against the node itself (not one of its children).

cbracken · 2022-09-02T22:36:02Z

shell/platform/windows/flutter_windows_view_unittests.cc

+    IAccessible* native_view = root_node->GetNativeViewAccessible();
+    ASSERT_TRUE(native_view != nullptr);
+
+    // Look up against the node itself (not one of its children)


Quick fix of existing comment:

Suggested change

// Look up against the node itself (not one of its children)

// Look up against the node itself (not one of its children).

cbracken · 2022-09-02T22:36:14Z

shell/platform/windows/flutter_windows_view_unittests.cc

+    EXPECT_TRUE(native_state.lVal & STATE_SYSTEM_CHECKED);
+  }

  // Test unchecked too


Quick fix of existing comment. Sadly, github won't let me fix the one on line 653/654.

Suggested change

// Test unchecked too

// Test unchecked too.

cbracken

I imagine you'll need to rebase to tip-of-tree once your framework-side test changes have landed.

chunhtai

LGTM

chunhtai · 2022-09-02T22:47:37Z

lib/ui/semantics.dart

+  /// For example, a [Checkbox] with [Checkbox.tristate] set to true
+  /// can have checked,  unchecked, or mixed state.
+  ///
+  /// Should be false when the checkbox is either checked or unchecked.


Suggested change

/// Should be false when the checkbox is either checked or unchecked.

/// Must be false when the checkbox is either checked or unchecked.

…engine#35868)

* Honor mixed state * Add to unit test * Add state enums * Fix delimiter * Add enums * Scope subtests, rename enum * Rename enums * Comment specification * Tidy up comments * Rename java enum * Trailing whitespace

yaakovschectman added 2 commits September 1, 2022 13:15

Honor mixed state

cad7efc

Add to unit test

07a2e0b

yaakovschectman added the accessibility label Sep 1, 2022

yaakovschectman requested review from a-wallen, cbracken and loic-sharma September 1, 2022 19:33

yaakovschectman self-assigned this Sep 1, 2022

flutter-dashboard bot added embedder Related to the embedder API platform-windows labels Sep 1, 2022

yaakovschectman added 2 commits September 1, 2022 15:56

Add state enums

f34e919

Fix delimiter

2a29c67

a-wallen reviewed Sep 1, 2022

View reviewed changes

lib/ui/semantics/semantics_node.h Outdated Show resolved Hide resolved

a-wallen reviewed Sep 1, 2022

View reviewed changes

shell/platform/common/accessibility_bridge.cc Outdated Show resolved Hide resolved

Add enums

0491222

cbracken reviewed Sep 2, 2022

View reviewed changes

shell/platform/windows/flutter_windows_view_unittests.cc Show resolved Hide resolved

cbracken reviewed Sep 2, 2022

View reviewed changes

cbracken requested a review from chunhtai September 2, 2022 02:46

Scope subtests, rename enum

e0b44f0

yaakovschectman mentioned this pull request Sep 2, 2022

Preliminary PR for engine changes for Tristate checkboxes flutter/flutter#110859

Merged

8 tasks

chunhtai reviewed Sep 2, 2022

View reviewed changes

Rename enums

441f766

yaakovschectman marked this pull request as ready for review September 2, 2022 21:02

Comment specification

f40cf80

yaakovschectman requested review from cbracken and chunhtai September 2, 2022 22:01

cbracken reviewed Sep 2, 2022

View reviewed changes

cbracken approved these changes Sep 2, 2022

View reviewed changes

chunhtai approved these changes Sep 2, 2022

View reviewed changes

yaakovschectman added 3 commits September 6, 2022 09:28

Tidy up comments

c1e9fd7

Rename java enum

4eff3af

Trailing whitespace

fa543d1

yaakovschectman merged commit a5cc2b4 into flutter:main Sep 6, 2022

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

Roll Flutter Engine from ce97b5a7f944 to 7db0a37ac76c (6 revisions) flutter/flutter#111044

Merged

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

a5cc2b4 Honor mixed state of tristate Checkbox in Semantics (flutter/…

409f077

…engine#35868)

	// Look up against the node itself (not one of its children)
	// Look up against the node itself (not one of its children).

	/// Should be false when the checkbox is either checked or unchecked.
	/// Must be false when the checkbox is either checked or unchecked.

Honor mixed state of tristate Checkbox in Semantics #35868

Honor mixed state of tristate Checkbox in Semantics #35868

Uh oh!

Conversation

yaakovschectman commented Sep 1, 2022

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

yaakovschectman commented Sep 2, 2022

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!

cbracken Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbracken Sep 2, 2022 •

edited

Loading

cbracken left a comment •

edited

Loading