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] NVDA screen reader not announcing the selected / not selected state for the 'Fonts' and 'Size' controls present in the CKEditor5 toolbar . #13250

Closed
msftedad opened this issue Jan 11, 2023 · 5 comments · Fixed by #13893
Assignees
Labels
squad:features Issue to be handled by the Features team. support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@msftedad
Copy link

📝 Provide detailed reproduction steps (if any)

  1. Open https://ckeditor.com/docs/ckeditor5/latest/examples/builds-custom/full-featured-editor.html
  2. Enable NVDA screen reader https://www.nvaccess.org/download/
  3. Tab to the 'Fonts' or 'Size' control and press Enter to expand. Use Use 'Down Arrow ' key to navigate through the expanded list items and press Enter to select any font or size .

✔️ Expected result

NVDA should announce whether a font or size is selected or not selected as the user navigates

❌ Actual result

NVDA does not report which item is selected or not selected

❓ Possible solution

https://developer.mozilla.org/en-US/docs/Web/API/Element/ariaSelected

📃 Other details

  • Browser: Edge 108.0.1462.76
  • OS: Windows 11 (any build)
  • First affected CKEditor version: 4, also repros on 5
  • Installed CKEditor plugins: N/A

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@msftedad msftedad added the type:bug This issue reports a buggy (incorrect) behavior. label Jan 11, 2023
@martynawierzbicka martynawierzbicka added the support:1 An issue reported by a commercially licensed client. label Feb 16, 2023
@Reinmar
Copy link
Member

Reinmar commented Feb 16, 2023

Notes from my discussion with @oleq (his replies)

Am I right that the issue stems from the fact that our dropdown buttons do not react to font features being applied?

 You're right. They are static.

Am I right that it works the same way in TinyMCE?

 Correct. Tiny and CKE5 work in the same way here. 

And that CKE4 works differently here and reflects the that state in the button?

 Correct. CKE4 is the best of the three in this aspect. It announces values correctly.

Was there a reason why we went this way with CKE5?

 I browsed #638 and #2277 and it probably comes down to:

Completely agree, remember that we have also heading, so it's equal to 3 dropdowns in a row, it's too much for the toolbar

which is a trade-off that totally makes sense: three dropdowns in the same row would extremely bloat our toolbars. Too much horizontal space would be wasted.

There's a silver lining, though: I don't see a good reason not to implement it the "CKE4 way" in CKE5. We can preserve "Font Size" tooltip and have the aria-label saying "Font size, [value of the command]" just like in CKE4. The improvement is rather cheap and it comes down to a few lines of code. We could use the same code for font/bg color and font family/size features.

diff --git a/packages/ckeditor5-font/src/fontsize/fontsizeui.ts b/packages/ckeditor5-font/src/fontsize/fontsizeui.ts
index ebdcbd3327..0afd5dc647 100644
--- a/packages/ckeditor5-font/src/fontsize/fontsizeui.ts
+++ b/packages/ckeditor5-font/src/fontsize/fontsizeui.ts
@@ -55,11 +55,12 @@ export default class FontSizeUI extends Plugin {
 
                            // Create dropdown model.
                            dropdownView.buttonView.set( {
-                                    label: t( 'Font Size' ),
                                     icon: fontSizeIcon,
-                                    tooltip: true
+                                    tooltip: t( 'Font Size' )
                            } );
 
+                           dropdownView.buttonView.bind( 'label' ).to( command, 'value', value => value ? t( 'Font Size, ' + value ) : t( 'Font Size' ) );
+
                            dropdownView.extendTemplate( {
                                     attributes: {
                                              class: [

@Reinmar
Copy link
Member

Reinmar commented Feb 16, 2023

There's a silver lining, though:

I like it 💎

@msftedad would that resolve this issue for you?

@msftedad
Copy link
Author

As per the above screen shot, voice over is announcing as expected after selecting the menu list.

When we verify in windows combination of JAWS and NVDA in CKE4 & CKE5, after selecting the font style in menu list, both NVDA and JAWS is not announcing the selected state with role.

@msftedad msftedad reopened this Feb 16, 2023
@oleq
Copy link
Member

oleq commented Mar 2, 2023

I researched this issue and here's what I found out:

  1. To begin with, the official w3 example should be considered the gold standard when it comes to dropdown accessibility.
    1. Despite that, neither NVDA nor JAWS will read the option item with aria-selected attribute. I checked that and only VoiceOver respects that attribute. I attach, the screenshot from NVDA below.
    2. Long story short, the problem lies in screen readers because they don’t respect the attribute of the spec.
  2. It is true that [Accessibility] NVDA screen reader not announcing the selected / not selected state for the 'Fonts' and 'Size' controls present in the CKEditor5 toolbar . #13250 (comment) provides only half of the solution.
    1. It allows the screen reader to read the currently selected value of the dropdown while navigating the toolbar (the dropdown is closed).
    2. It does not contribute to the screen reader support when the dropdown is open and the selected dropdown (list) item is focused.
      1. But in practice, this would not work (see 1.i.) anyway, even if aria-selected was used according to the spec.
  3. Long story short, these are things we can do in CKE5:
    1. We can improve the way we announce dropdown’s value while navigating the toolbar (2.i.).
    2. We can improve the overal experience of the list item navigation. Currently, screen readers won’t read list items as “[text of the item], 5 of 10”. They only read “[text of the item]”. To address that, we’ll need several new attributes. I checked this and it appears to be working.
    3. Lastly, we can add aria-selected to selected ButtonView to comply with the standard but as I mentioned in 1.i. this will not do anything right now for the screen reader users.

The PoC of all the above is in https://github.com/ckeditor/ckeditor5/compare/ci/13250-dropdown-selecte-item-poc?expand=1.

@SMTestGH
Copy link

SMTestGH commented Mar 3, 2023

@oleq thanks, agree with this approach. FYI NVDA considers aria-selected as by design (nvaccess/nvda#12893) and this is a known issue in JAWS.

@mlewand mlewand added the squad:features Issue to be handled by the Features team. label Mar 20, 2023
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Mar 20, 2023
@mmotyczynska mmotyczynska self-assigned this Mar 21, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 21, 2023
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. labels Apr 6, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Apr 17, 2023
oleq added a commit that referenced this issue Apr 20, 2023
…ed-state-for-dropdowns-v3

Fix (font, heading): Screen readers should now announce the selected option in dropdown lists for font size, font family, and heading features. Closes #13250.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Apr 20, 2023
@CKEditorBot CKEditorBot added this to the iteration 63 milestone Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:features Issue to be handled by the Features team. support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants