Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: Rename Checked to Toggled; drop ToggleButton role #388

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

mwcampbell
Copy link
Contributor

ARIA and AT-SPI have both "checked" and "pressed" flags, but they're functionally equivalent; the only difference is that one is for checkboxes while the other is for toggle buttons. I prefer having just one property. AccessKit already had only a "checked" property, which the AT-SPI adapter translated to "pressed" if appropriate. But I think it makes sense to rename our "checked" property to something more generic, since "checked" only makes sense for checkboxes. Here I took some inspiration from UIA, though I went with the more succinct "toggled" rather than UIA's ToggleState.

I also think it makes sense to not have a dedicated ToggleButton role. Instead, now a Button becomes a toggle button if the toggled property is set. This is the same way that buttons become toggle buttons in ARIA.

If it seems that I'm being inconsistent about whether to add dedicated roles (as I did in a previous refactor) or use state flags to modify the role (as I'm doing here), the guiding principle is this: eliminate as many invalid combinations of role and state as we can. When I previously eliminated state flags and added more roles instead, the only purpose of the state flag was to modify one specific role. But here, where we have a state property that can be false, true, or unset (unlike the simple flags I previously eliminated), the property serves to both modify the role (two different roles in this case) and indicate some state. The goal is the same, by eliminating the ToggleButton role, we make it impossible to mess up the accessibility node by having a ToggleButton with no toggle state, and now, having a plain Button with toggle state is the one way to do a toggle button.

Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

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

I totally agree. Someone will have to revert slint-ui/slint#5033 but that's fine.

bindings/c/cbindgen.toml Outdated Show resolved Hide resolved
bindings/c/cbindgen.toml Outdated Show resolved Hide resolved
@DataTriny DataTriny merged commit 6bc040b into main Apr 25, 2024
10 checks passed
@DataTriny DataTriny deleted the toggle-refactor branch April 25, 2024 19:28
@mwcampbell mwcampbell mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants