Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@yaakovschectman
Copy link
Contributor

Switch widgets used to have the PUSHBUTTON role on Windows, which has no associated state. This PR switches it to a CHECKBUTTON and associates the PRESSED state with its MSAA node when it is toggled. Adds a unit test to verify that the native state reflects the toggle state of the Switch.

Also fixes some typos in comments in the existing checkbox unit test.

Addresses #flutter/flutter/111525

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.

varchild.lVal = CHILDID_SELF;
VARIANT native_state = {};
ASSERT_TRUE(SUCCEEDED(native_view->get_accState(varchild, &native_state)));
EXPECT_FALSE(native_state.lVal & STATE_SYSTEM_PRESSED);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to also test its MSAARole is ROLE_SYSTEM_CHECKBUTTON?


case ax::mojom::Role::kToggleButton:
return ROLE_SYSTEM_PUSHBUTTON;
return ROLE_SYSTEM_CHECKBUTTON;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is check button basically a check box? Why do we choose this over push button?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same role as for a checkbox. In #flutter/fluter/111525 we are currently of the opinion that it is the most appropriate role, as it is for interactable widgets with toggleable state, and the materials design guidelines recommend their use in place of checkboxes on mobile

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explanation. SGTM

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


case ax::mojom::Role::kToggleButton:
return ROLE_SYSTEM_PUSHBUTTON;
return ROLE_SYSTEM_CHECKBUTTON;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explanation. SGTM

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM stamp from a Japanese personal seal

@yaakovschectman yaakovschectman marked this pull request as ready for review September 15, 2022 20:48
Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - nice work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants