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!: Clean up roles and properties #289

Merged
merged 1 commit into from
Aug 31, 2023
Merged

refactor!: Clean up roles and properties #289

merged 1 commit into from
Aug 31, 2023

Conversation

mwcampbell
Copy link
Contributor

I've decided to get as many of these cleanups in as possible before we publish the next release, which has other breaking changes.

I changed "text field" to "text input" because I think the latter is a bit more descriptive and not any longer.

It seems to me that Chromium got "text field" from WebKit, which in turn was influenced by Cocoa/AppKit, given that WebKit began at Apple. The same history also accounts for the old use of a PopupButton role for simple combo boxes. I reworked the combo box roles to be more in line with what's common across other platforms, including the web.

I think that when we have a flag that only applies to one type of role, it's better to just add another role. That's why I added Role::DefaultButton, Role::PasswordInput, and Role::MultilineTextInput.

I don't want to have any property that is effectively a subrole; I think it's better to have a large, flat list of roles. So, rather than replace the old input_type string property with an enum, I added roles for the text input types that we didn't already have. I also established a consistent naming convention for roles that we did already have, like SearchBox, Date, DateTime, and InputTime.

I'm not entirely sure that we won't ever need the css_display and live_relevant properties. But I'm also not sure if we want to keep them as strings, so I dropped them for now. Adding them later won't be a breaking change.

I dropped aria_role because the Role enum should be comprehensive enough. When in doubt, I bias toward whatever will make AccessKit less confusing and daunting for toolkit developers. In this case, I didn't want them to think that they had to also set the ARIA role as a string.

I dropped indirect_children because Chromium only uses that property to handle a quirk of how table columns are exposed on macOS. When we get to that, that platform-specific behavior should be confined to the macOS platform adapter.

I dropped the selected_from_focus flag because Chromium only uses that flag in its Windows UIA implementation, to decide whether to perform the default action when selecting a node. I decided that this can be done better by having distinct Select and Unselect default action verbs, so the platform adapter knows which action would be performed if it does the default action.

I haven't done a comprehensive review of all roles yet, but I dropped a few that I'm quite sure are unnecessary and that I think would be confusing.

@mwcampbell
Copy link
Contributor Author

I forgot to mention:

I dropped the nonatomic_text_field flag because, if we ever actually have to deal with that, I think the consumer library can calculate that flag from existing information, as Blink itself does in Chromium.

I also think the editable flag is redundant, since we have read_only.

Again, I want to clear away as much clutter as possible to make accessibility more approachable for toolkit developers.

@DataTriny DataTriny merged commit 4fc9c55 into main Aug 31, 2023
5 checks passed
@DataTriny DataTriny deleted the schema-cleanup branch August 31, 2023 18:00
@mwcampbell mwcampbell mentioned this pull request Aug 31, 2023
@Vrixyz
Copy link
Contributor

Vrixyz commented Oct 22, 2023

Sorry for arriving late, I noticed you removed LabelText (https://github.com/AccessKit/accesskit/pull/289/files#diff-d38884f7391ae50f795707dbb281656e8afe38ea3ecac15a6ee80d52724017c0L76) ; I'm not seeing it mentioned in issues/commits, I'll opt for StaticText instead (as I'm not sure when I'm creating it what this text will be used for).

The wording "static" makes me fear this is for some text which won't be changed, but my text might change. Is that a problem ? If you have any suggestions, I'd appreciate you enlighten me. (context is bevy winit update .

@DataTriny
Copy link
Member

@Vrixyz As long as you are not representing a text field/area then StaticText is the right role to use when describing a piece of text. You are of course allowed to modify the content, this role is just not meant for text directly editable by the user. Updating a StaticText node in response to a button click or even a text change in a text field somewhere else is acceptable.

We have to work on better documenting changes to this project at some point. Compiling a migration guide for new major release would be a good start.

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.

3 participants