Expose toggle buttons in accessibility tree #5123

Comandeer · 2022-03-12T23:48:51Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

New features:

* [#2444](https://github.com/ckeditor/ckeditor4/issues/2444): Togglable toolbar buttons are now exposed as toggle buttons in the browser's accessibility tree.

Fixed Issues:

* [#4543](https://github.com/ckeditor/ckeditor4/issues/4543): The selected states of toolbar buttons are not announced by screen readers.

What changes did you make?

I've changed the way of handling the [aria-pressed] attribute in the button plugin. After the changes, the attribute is always present on the button and only its value is changed (previously the attribute was removed when the button was not pressed). Additionally, I've added the isToggle option to the button's definition which indicates that the button is togglable (this option is needed to handle the initial state of the toggle buttons).

I've then added isToogle option to plugins using toggle buttons:

basicstyles,
bidi,
blockquote,
copyformatting,
justify,
list,
sourcearea.

However, there are still some plugins that require additional attention:

language and colorbutton – these plugins use menu buttons that are also togglable, however, screen readers do not support such combination, so the information about the state of the toggle button is lost,
maximize – this plugin uses the toggle button but instead of conveying the state using the [aria-pressed] attribute, it changes the label of the button (from "Maximize" to "Minimize"; I'd suggest replacing the current method with the [aria-pressed] one, used in other plugins.

Which issues does your PR resolve?

Closes #2444.
Closes #4543.

Comandeer · 2022-03-15T16:23:39Z

I've updated the maximize plugin to also use [aria-pressed] attribute instead of a dynamic label.

jacekbogdanski

TBC

plugins/button/plugin.js

jacekbogdanski · 2022-03-23T09:42:09Z

plugins/button/plugin.js

+				if ( this.isToggle && !this.hasArrow ) {
+					// Note: aria-pressed attribute should not be added to menuButton instances. (https://dev.ckeditor.com/ticket/11331).
+					// Do not remove the attribute, set its value (#2444).
+					element.setAttribute( 'aria-pressed', state === CKEDITOR.TRISTATE_ON );


I'm not sure if it's not introducing a breaking change. If someone is already using a button (3rd party plugin) without an arrow, it supports the aria-pressed feature. Maybe we should instead keep the previous logic for the non-defined isToggle value?

Actually, we have never supported [aria-pressed] on menubuttons – the only change I've introduced here is the explicit check if the button is a toggle one.

Looks like it was setting aria-pressed for !this.hasArrow check? Although I don't see it to be working correctly before your changes on VoiceOver despite having aria-pressed="true" for toggled button - not sure why.

Because toggle buttons are identified by the presence of the [aria-pressed] attribute. If the attribute is not present, it's considered a normal button. So adding that attribute only for the true value causes that the button is a toggle one only when it's pressed (in Chrome it isn't at all).

jacekbogdanski · 2022-03-23T09:47:26Z

plugins/maximize/plugin.js

-					// Toggle button label.
-					var button = this.uiItems[ 0 ];
-					// Only try to change the button if it exists (https://dev.ckeditor.com/ticket/6166)
-					if ( button ) {
-						var label = ( this.state == CKEDITOR.TRISTATE_OFF ) ? lang.maximize.maximize : lang.maximize.minimize;
-						var buttonNode = CKEDITOR.document.getById( button._.id );
-						buttonNode.getChild( 1 ).setHtml( label );
-						buttonNode.setAttribute( 'title', label );
-						buttonNode.setAttribute( 'href', 'javascript:void("' + label + '");' ); // jshint ignore:line
-					}


Aren't we losing some information about the second behavior of the button with that change? Previously, this button was nicely labeled with minimize once maximized. Now, it looks like this change favors screen readers only. Is it possible to keep both an aria-pressed attribute and a label (in the context of accessibility)?

In fact, "Maximize" is the only button that works this way. Every other one does not toggle its label, instead, they apply the correct value of the [aria-pressed] attribute.

Additionally, I do not think that the information is lost. The fact that the editor is maximized is communicated via two things: maximized editor (obviously) and the styling of the pressed button. It's analogous to other toggle buttons, e.g. "Bold" – we have a pressed "Bold" button, not a pressed "Unbold" button.

And a small nitpick – I really think that "MInimize" means something slightly different than restoring the maximized window to its original size 😛

Yeah, I think that makes sense, thanks for the explanation 👍🏻

Comandeer · 2022-03-23T15:34:40Z

I've removed the unnecessary template for the [aria-pressed] attribute.

jacekbogdanski

These changes look good but I was only able to do manual tests using VoiceOver as my VM machines decided to not support OS sounds. @sculpt0r could you give it a try and confirm that manual test for screen readers works correctly on JAWS + Firefox and NVDA?

There is one issue but I'm not sure if it's related to your changes. Toggling source area forces the screen reader (VoiceOver) to read some garbage about being inside a table (for manual test with a table) or inside a list (for nightly sample). I'm not sure, but it looks like screen reader focus goes somewhere else after toggling a button?

tests/plugins/button/manual/togglebutton.md

tests/plugins/button/manual/togglebuttonscreenreader.md

jacekbogdanski · 2022-03-25T10:08:31Z

plugins/button/plugin.js

+				if ( this.isToggle && !this.hasArrow ) {
+					// Note: aria-pressed attribute should not be added to menuButton instances. (https://dev.ckeditor.com/ticket/11331).
+					// Do not remove the attribute, set its value (#2444).
+					element.setAttribute( 'aria-pressed', state === CKEDITOR.TRISTATE_ON );


Looks like it was setting aria-pressed for !this.hasArrow check? Although I don't see it to be working correctly before your changes on VoiceOver despite having aria-pressed="true" for toggled button - not sure why.

tests/plugins/button/_helpers/buttontools.js

Comandeer · 2022-03-25T17:09:19Z

There is one issue but I'm not sure if it's related to your changes. Toggling source area forces the screen reader (VoiceOver) to read some garbage about being inside a table (for manual test with a table) or inside a list (for nightly sample). I'm not sure, but it looks like screen reader focus goes somewhere else after toggling a button?

That's weird 🤔 The focus does not go anywhere, it stays on the button. The issue you describe appears only in Chrome, I can't reproduce it in Firefox or Safari. It looks like a browser bug.

Comandeer · 2022-03-25T17:21:56Z

I've fixed the reported codestyle issues.

Co-authored-by: Jacek Bogdański <jacekbogd@gmail.com>

sculpt0r · 2022-03-28T10:16:43Z

Rebased onto the newest master

sculpt0r

@Comandeer nice work with the test tooling 👍

JAWS & NVDA works fine 🎉 I have only doubts about naming and other small stuff in a few places - please take a look at the inline comments.

Additionally - I think we should add a test with the language button in the toolbar. It is not the plain toggle button, but the code around hasArrow was changed and it will be good to verify whenever this button still works as we expected - also with screen readers.

plugins/button/plugin.js

tests/plugins/maximize/manual/togglebutton.md

tests/plugins/button/_helpers/buttontools.js

plugins/button/plugin.js

Co-authored-by: Bartłomiej Kalemba <b.kalemba@cksource.com>

Comandeer · 2022-03-30T10:14:49Z

Ok, I've refactored names a bit and added a manual test for screen readers and menu button.

sculpt0r

LGTM 🎉

Thank you for the separated test for the menu button. Thanks to that, I found and extracted an issue with aria-expanded: #5144

Added a changelog.

sculpt0r · 2022-03-31T11:26:54Z

@Comandeer - we have to wait for @jacekbogdanski approval - since he also reviewed this PR. Without it, GH is blocking me with the merge.

github-actions · 2022-04-08T09:19:20Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

jacekbogdanski · 2022-04-09T11:32:08Z

CHANGES.md

+New features:
+
+* [#2444](https://github.com/ckeditor/ckeditor4/issues/2444): Togglable toolbar buttons are now exposed as toggle buttons in the browser's accessibility tree.


A feature changelog entry should go to a major release.

Comandeer self-assigned this Mar 15, 2022

Comandeer removed their assignment Mar 15, 2022

jacekbogdanski self-assigned this Mar 22, 2022

jacekbogdanski self-requested a review March 22, 2022 11:03

jacekbogdanski reviewed Mar 23, 2022

View reviewed changes

Comandeer assigned Comandeer and unassigned jacekbogdanski Mar 23, 2022

Comandeer requested a review from jacekbogdanski March 23, 2022 15:34

Comandeer removed their assignment Mar 23, 2022

jacekbogdanski self-assigned this Mar 25, 2022

jacekbogdanski requested changes Mar 25, 2022

View reviewed changes

CKEditorBot unassigned jacekbogdanski Mar 25, 2022

sculpt0r self-assigned this Mar 28, 2022

sculpt0r self-requested a review March 28, 2022 08:54

Comandeer added 12 commits March 28, 2022 12:16

Add manual test.

c8f10c1

Add uni tests.

8ff5fd5

Add isToggle option.

6d0492f

Add ultimate manual test.

3ab065a

Move button test helpers to separate file.

f1e2386

Refactor tests for [aria-pressed].

65092e3

Implement toggle buttons in basicstyles plugin.

771d372

Implement toggle buttons in bidi plugin.

8a9385f

Implement toggle buttons in blockquote plugin.

1d21e12

Implement toggle buttons in copyformatting plugin.

44ad6ad

Implement toggle buttons in justify plugin.

c5d90c4

Implement toggle buttons in list plugin.

a113b61

Comandeer and others added 2 commits March 28, 2022 12:16

Update manual test description.

0e2417c

Co-authored-by: Jacek Bogdański <jacekbogd@gmail.com>

Codestyle fixes for button helpers.

0125f46

sculpt0r force-pushed the t/2444 branch from 899609c to 0125f46 Compare March 28, 2022 10:16

sculpt0r suggested changes Mar 29, 2022

View reviewed changes

CKEditorBot unassigned sculpt0r Mar 29, 2022

Comandeer self-assigned this Mar 30, 2022

Comandeer and others added 4 commits March 30, 2022 10:32

Fix typo in the manual test.

76403c6

Co-authored-by: Bartłomiej Kalemba <b.kalemba@cksource.com>

Rename variables.

4d46118

Co-authored-by: Bartłomiej Kalemba <b.kalemba@cksource.com>

Rephrase comment.

cefd314

Add manual test for menubutton.

3681428

Comandeer force-pushed the t/2444 branch from c28e87f to 3681428 Compare March 30, 2022 10:14

Comandeer requested a review from sculpt0r March 30, 2022 10:14

CKEditorBot unassigned Comandeer Mar 30, 2022

sculpt0r self-assigned this Mar 31, 2022

Add changelog entry

509d2cc

sculpt0r approved these changes Mar 31, 2022

View reviewed changes

CKEditorBot unassigned sculpt0r Mar 31, 2022

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Apr 8, 2022

Comandeer removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Apr 8, 2022

jacekbogdanski added 2 commits April 9, 2022 12:41

Improved changelog message, moved changelog entries to a major release.

e576d50

Updated version tag to 4.19.0

dc0e60c

jacekbogdanski approved these changes Apr 9, 2022

View reviewed changes

jacekbogdanski merged commit c0dbd14 into master Apr 9, 2022

jacekbogdanski deleted the t/2444 branch April 9, 2022 11:45

This was referenced Apr 9, 2022

Expose toggle buttons in accessibility tree #2444

Closed

[Accessibility] - The selected state for the toolbar buttons is not announced by screen reader #4543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose toggle buttons in accessibility tree #5123

Expose toggle buttons in accessibility tree #5123

Comandeer commented Mar 12, 2022 •

edited

Loading

Comandeer commented Mar 15, 2022

jacekbogdanski left a comment

jacekbogdanski Mar 23, 2022

Comandeer Mar 23, 2022

jacekbogdanski Mar 25, 2022

Comandeer Mar 25, 2022

jacekbogdanski Mar 23, 2022

Comandeer Mar 23, 2022

jacekbogdanski Mar 23, 2022 •

edited

Loading

Comandeer commented Mar 23, 2022

jacekbogdanski left a comment

jacekbogdanski Mar 25, 2022

Comandeer commented Mar 25, 2022

Comandeer commented Mar 25, 2022

sculpt0r commented Mar 28, 2022

sculpt0r left a comment

Comandeer commented Mar 30, 2022

sculpt0r left a comment

sculpt0r commented Mar 31, 2022

github-actions bot commented Apr 8, 2022

jacekbogdanski Apr 9, 2022

		New features:

		* [#2444](https://github.com/ckeditor/ckeditor4/issues/2444): Togglable toolbar buttons are now exposed as toggle buttons in the browser's accessibility tree.

Expose toggle buttons in accessibility tree #5123

Expose toggle buttons in accessibility tree #5123

Conversation

Comandeer commented Mar 12, 2022 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

Comandeer commented Mar 15, 2022

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Comandeer commented Mar 23, 2022

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented Mar 25, 2022

Comandeer commented Mar 25, 2022

sculpt0r commented Mar 28, 2022

sculpt0r left a comment

Choose a reason for hiding this comment

Comandeer commented Mar 30, 2022

sculpt0r left a comment

Choose a reason for hiding this comment

sculpt0r commented Mar 31, 2022

github-actions bot commented Apr 8, 2022

Choose a reason for hiding this comment

Comandeer commented Mar 12, 2022 •

edited

Loading

jacekbogdanski Mar 23, 2022 •

edited

Loading