-
Notifications
You must be signed in to change notification settings - Fork 543
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
TodoMVC: first step to improve accessibility #5580
base: master
Are you sure you want to change the base?
Conversation
@@ -34,6 34,7 @@ export component CreateTaskView { | |||
HorizontalLayout { | |||
IconButton { | |||
icon: Icons.close; | |||
accessible-label: @tr("close"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be a little more description in saying what is closed? (like a button to play music should say "play music")
@DataTriny What do you think?
(the same comment applies to most of the other labels as well, of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessible names should be capitalized just like any user-facing text. Of course people using speech systhesis won't notice, but others might use a Braille display for instance.
I would probably add a description saying that this button goes back to the task list.
@@ -46,6 46,7 @@ export component TaskListView { | |||
SelectionListView { | |||
width: 100%; | |||
model: TaskListAdapter.tasks; | |||
accessible-label: @tr("task listview"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DataTriny Could you help us out here: Should accessible labels be capitalised? (my feeling is yes, but it would be good to confirm :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, capitalization is the least of my concerns here. Why do you talk about "listview" to your users? "List of tasks" would be far more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other things worth trying:
- Put the "Create" button just after the time combobox in the tab index order,
- Place the focus on the task name edit after the "Add" button is clicked,
- Put the focus on the task list when the program starts and when coming back from the create task screen.
- Both controls to pick the due date and time seem to not be labelled.
@@ -34,6 34,7 @@ export component CreateTaskView { | |||
HorizontalLayout { | |||
IconButton { | |||
icon: Icons.close; | |||
accessible-label: @tr("close"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessible names should be capitalized just like any user-facing text. Of course people using speech systhesis won't notice, but others might use a Braille display for instance.
I would probably add a description saying that this button goes back to the task list.
@@ -46,6 46,7 @@ export component TaskListView { | |||
SelectionListView { | |||
width: 100%; | |||
model: TaskListAdapter.tasks; | |||
accessible-label: @tr("task listview"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, capitalization is the least of my concerns here. Why do you talk about "listview" to your users? "List of tasks" would be far more appropriate.
@@ -61,6 62,7 @@ export component TaskListView { | |||
|
|||
ActionButton { | |||
icon: Icons.add; | |||
accessible-label: @tr("add"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit more context could be useful. "Create new task" or something along those lines would be clearer.
@@ -48,6 48,7 @@ export component SelectionListViewItemDelegate { | |||
check-box := CheckBox { | |||
horizontal-stretch: 0; | |||
y: (parent.height - self.height) / 2; | |||
accessible-label: @tr("selected"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checkbox indicates whether a task is done right? "selected" is misleading.
The proper solution here would be to build the checkbox name from the task name and the due date: ", due by "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little tricky. I think for now I'll change the label to indicate whether the task is done. Regarding a proper solution, I'm curious: Would you build the checkbox to have an accessible label that says "Task with description XYZ, due by ABC" and then set the accessibility role of the separate description Text
label to none
?
|
||
IconButton { | ||
y: (parent.height - self.height) / 2; | ||
icon: Icons.remove; | ||
accessible-label: @tr("remove"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be far more practical to support the delete key as well. In a proper TODO list app you would use the up and down arrow keys to select listview elements, pressing tab could put the focus to the currently selected item's remove button, but preferably not the other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, full keyboard navigation support would be nice to have. For now I've changed this to "Delete Task".
@@ -91,6 92,8 @@ export component SelectionListView inherits ListView { | |||
callback toggle(/* index */ int); | |||
callback remove(/* index */ int); | |||
|
|||
accessible-role: list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder that the accessible role is not the right one. At least for the winit/AccessKit backend you are using the role for semantic lists, as you would use in a paragraph of text. The role to use is Role::ListBox
and each element in the list should have Role::ListBoxOption
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we don't have support for ListBox
yet :(. Maybe not for this PR.
From look at the ARIA description for list vs. listbox, it sounds like to me that ListView
should default to listbox
as a role, as more often than not it's something the user interacts with and selects from, as opposed to a static list
just for display (like a bullet list). Would you agree?
@DataTriny thank you. Your feedback helps a lot. |
Due to the merge conflict I'll take the liberty of rebasing this. |
Use more elaborate label text.
03e4be0
to
03f1e58
Compare
No description provided.