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

Accessibility for Font size and Font family dropdowns in toolbar #13749

Closed

Conversation

mmotyczynska
Copy link
Contributor

@mmotyczynska mmotyczynska commented Mar 24, 2023

Suggested merge commit message (convention)

Fix (font): Screen readers should now announce selected option in dropdown lists for font size and font family.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@mlewand mlewand requested a review from oleq March 29, 2023 08:14
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

While reviewing this PR I attempted to make the heading dropdown accessible similarly because I think this won't end at font* features. Soon, I discovered this didn't play out so well

The PoC is here https://github.com/ckeditor/ckeditor5/compare/ck/13250-accessibility-selected-state-for-dropdowns-v2?expand=1.

The problem is that while font* dropdowns have invisible labels, the heading presents the label to the user. As a result, the label is read by the screen reader twice after using the new (correct) aria labels.

  • The first time it is read because .ck-button (ButtonView) has aria-labelledby that points to the label.
  • The second time because the screen reader discovers the "value" of the button (text of the label).

More details:

  • This is happening because of role="combobox" set on the <button> element. 
  • This proves that my initial approach to setting labels that inspired Marta could be wrong. It works only as long as the label is invisible.
  • We may need to rethink how dropdowns are labeled, which now that I think about it, totally makes sense:
    • We need a label for the whole dropdown, for instance "Font family" (this one is missing).
      • But the API is there: DropdownView#ariaDescribedById exists and has been used in UIs such as table properties (as a child of LabeledFieldView).
    • We need a label for the dynamic value of the feature, for instance "Helvetica", unlike what I proposed in the past and unlike what has been done in this PR.
    • Then we need attributes that couple the dropdown button with the list. This is covered in this PR.
    • Only then we'll get as close to the standards as possible.

@mmotyczynska
Copy link
Contributor Author

After additional research we agreed with Olek that we should change the implementation from using a combobox and listbox to using a regular button and menu roles. The new approach seems to work in main screen readers (VoiceOver, NVDA, JAWS) well: announces the selected item when navigating through a dropdown list. We decided to follow this example as it seems the most relevant in our case.

The new PoC was prepared for font family, font size and heading features. It's already covered with tests.

Some implementation details:

  • The list in a dropdown has the role menu and specified aria-label (a feature’s name).
  • Items in the list view now have menuitemradio attribute.
  • Selected item has aria-checked attribute set to true.

@mmotyczynska
Copy link
Contributor Author

As we agreed to go with a different solution, I will close this PR. The new version can be found in #13893.

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.

2 participants