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
Info and relationships issues #16231
base: master
Are you sure you want to change the base?
Conversation
aria-describedby
instead of aria-expanded
in docs tooltips
WalkthroughThe recent updates across various CKEditor5 packages primarily focus on enhancing accessibility by standardizing roles like 'menuitemcheckbox' and adjusting properties related to ARIA states such as 'aria-pressed' and 'aria-checked. These changes aim to ensure consistent handling of UI components, particularly buttons, across different editor functionalities, promoting better accessibility practices within the editor's UI components. Changes
This table groups files with similar changes together, providing a clear overview of the type of modifications made across different components of the CKEditor5 project. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 1
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.
Some things are fine, some need changes and some need to be cleared up.
role: 'menuitemcheckbox' | ||
} ); | ||
|
||
buttonView.bind( 'isOn' ).to( command, 'value' ); |
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.
element: child2 | ||
} ); | ||
|
||
expect( parent.getAttribute( 'aria-placeholder' ) ).to.be.equal( 'Hello World!, Helloooo!!' ); |
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.
What value would that be to the users? There could be 10 placeholders in 10 random places in the document (at different nesting levels). It doesn't seem like aggregation is the way to go here. Here's the screenshot from http://fake.ckeditor.com:8125/ckeditor5-heading/tests/manual/title.html
Shouldn't the aria-placeholder
attribute change dynamically as users move between placeholder-enabled elements? Would a screen reader even respect that?
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.
Placeholder should provide an additional description for the field that is currently focused. Reading placeholders of all fields in one place defeats the purpose of that mechanism.
I would simply leave aria-placeholder
s in the title and body fields, removing it from the editor itself. But I'm not sure if it would be read 🤔 Dynamically changing placeholder also sounds like something that would be ignored.
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.
@Comandeer / @oleq So what should we do?
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.
Actionable comments posted: 1
Suggested merge commit message (convention)
Docs: Improved accessibility of tooltips in documentation by switching from
aria-expanded
toaria-describedby
in elements they are attached to.Feature (engine): Improved accessibility of editor placeholders by adding
aria-placeholder
attributes to elements that placeholders are attached to.Fix: Source mode and show block buttons are no longer marked as dropdown buttons in aria attributes.
Fix (ui): Improve accessibility of menu bar toggle items by marking them as
menucheckbox
aria roles.Additional information
Closes https://github.com/cksource/ckeditor5-commercial/issues/6037
Commercial PR: https://github.com/cksource/ckeditor5-commercial/pull/6180
Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes
Tests