ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

Table dimensions are read as "0 by 0" by Windows Narrator

Open lkszzajac opened this issue 1 year ago • 2 comments

📝 Provide detailed reproduction steps (if any)

  1. Open any editor and remove its initial content (e.g. https://ckeditor.com/ckeditor-5/demo/feature-rich/)
  2. Create a table (e.g. 3x3, can be empty)
  3. Enable Windows Narrator
  4. Focus the table widget

✔️ Expected result

Narrator reads the widget as a table and announces its dimensions

❌ Actual result

Narrator reads the widget as a table and reports its dimensions as 0x0

https://github.com/user-attachments/assets/fb1ce81a-e0de-45df-9f26-0a3d6630dc1c

📃 Other details

  • Browser: Edge
  • OS: Windows 11
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …
  • Related issues in other screen readers:
    • https://github.com/ckeditor/ckeditor5/issues/11867
    • https://github.com/ckeditor/ckeditor5/issues/12224
    • https://github.com/ckeditor/ckeditor5/issues/13479
  • Issue does not reproduce in other screen readers because no other reader reports on table dimension by default on focus. VoiceOver can read the table dimensions only after focusing a cell and going "up" in focus by one level

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

lkszzajac avatar Aug 14 '24 11:08 lkszzajac

We've checked the behavior in these setups:

Windows/Edge:

  • Narrator - announces the table with "Enter table, 0x0"
  • JAWS - announces the table with "Edit, land, type of text"
  • NVDA - announces the table with "Figure, table, mutliline, blank"

macOS/Chrome:

  • VoiceOver - announces the table with "table" without any dimensions.

macOS/Firefox/Safari:

  • VoiceOver - announces the table with dimensions correctly, does not announce the cells as editable

lkszzajac avatar Aug 14 '24 14:08 lkszzajac

According to the ARIA spec, elements with the row role (and <tr> elements have it by default) can only contain children with the cell role. However, due to how we implemented nested editables, we overwrite the role of the <td> elements to textbox, creating an accessibility tree that is invalid according to the specification. Probably we should rethink our approach to nested editables in tables.

Some possible solutions:

  • remove the textbox role – however it could lead to loss of information about cells being editable; needs testing,
  • move the textbox role into a dynamically injected div element that would reside inside the cell – sounds quite brittle and error-prone,
  • keep the current structure and provide the table info ourselves – that's a really dirty hack, though.

Comandeer avatar Aug 16 '24 14:08 Comandeer

@Comandeer I checked few solutions:

remove the textbox role – however it could lead to loss of information about cells being editable; needs testing,

It works, but it stopped announcing that content is editable. At this moment, after focusing the first cell of the table, it's announcing: Tabela, przejdź do tabeli, zero na zero, edytuj, spacja but after modification: Tabela, Przejdź do tabeli, 5 na 6, wejście angielski, kolumna 3 z 5, koniec wiersza. So imo, it's even worse.

move the textbox role into a dynamically injected div element that would reside inside the cell – sounds quite brittle and error-prone,

I tried to add role: textbox to ck-table-bogus-paragraph that is directly under td and removing role: textbox from td. It's not announcing info about editable content, identically as with the missing role: textbox.

keep the current structure and provide the table info ourselves – that's a really dirty hack, though.

I'm not sure. Do you mean adding aria-label to td?

Mati365 avatar Sep 09 '24 11:09 Mati365

What screen reader did you test? VoiceOver? It would be better to test it in English and to also test JAWS and NVDA.

It works, but it stopped announcing that content is editable. At this moment, after focusing the first cell of the table, it's announcing: Tabela, przejdź do tabeli, zero na zero, edytuj, spacja but after modification: Tabela, Przejdź do tabeli, 5 na 6, wejście angielski, kolumna 3 z 5, koniec wiersza. So imo, it's even worse.

Isn't "wejście angielski" supposed to mean that the field is editable and its language is set to English? 🤔 Yet it's super cryptic (at least in Polish).

I'm not sure. Do you mean adding aria-label to td?

More of [aria-describedby] on the table/figure with the dimensions of the table.

Comandeer avatar Sep 09 '24 11:09 Comandeer

@Comandeer

What screen reader did you test? VoiceOver? It would be better to test it in English and to also test JAWS and NVDA.

Microsoft Reader using this HTML file: index.html

Isn't "wejście angielski" supposed to mean that the field is editable and its language is set to English? 🤔 Yet it's super cryptic (at least in Polish).

Checked, it reads it as "Passed to cell". Nothing about editing.

More of [aria-describedby] on the table/figure with the dimensions of the table.

I checked it using aria-describedby / aria-labeledby and it doesn't work. I tested it using:

      <figure class="table ck-widget ck-widget_with-selection-handle" style="width:29.68%;" contenteditable="false" >
         <table class="ck-table-resized" aria-labelledby="trash-desc">
            <tbody>
               <tr>
                  <td class="ck-editor__editable ck-editor__nested-editable" tabindex="-1" role="textbox" aria-labelledby="trash-desc" aria-describedby="trash-desc" contenteditable="true">
                     <span>Cześć!</span>
                  </td>

Basically it read content of trash-desc but always append 0 x 0 😞


move the textbox role into a dynamically injected div element that would reside inside the cell – sounds quite brittle and error-prone,

imho, it's the only correct way to fix this issue, and seems to be working correctly but it's the most complex one. At this moment Microsoft Narrator does not recognize td tags as cells so it's reading table size as 0x0.

Mati365 avatar Sep 09 '24 12:09 Mati365

I added draft PoC of table with adjusted HTML structure that works on Microsoft Narrator: https://github.com/ckeditor/ckeditor5/pull/17083

Unfortunately, it looks it's buggy and needs refactor of major parts of table CSS and investigation why Firefox hangs.

Mati365 avatar Sep 12 '24 06:09 Mati365

@Mati365 could you prepare a comparison table of screen readers of current solution vs the one you tested.

Witoso avatar Sep 12 '24 06:09 Witoso

@Witoso Checked few screen readers on that table:

obraz

It looks like changing structure of table HTML improved support for Microsoft Narrator and JAWS. NVDA works the same as before.

I did not test Apple VoiceOver.

Microsoft Narrator:

It's improved ✅ 0 by 0 issue has been fixed.

Before: Enter table, 0 by 0, <content editable content> After: Enter table, 10 by 12, <content editable content>

NVDA

Nothing changed, but it was ok already. ✅

Before: Table, Object with Data, Editing fields, <content editable content> After: Table, Object with Data, Editing fields, <content editable content>

JAWS

It looks like changing structure of table improved JAWS navigation a lot. ✅

Before: Type and test After: Terrestrial planets, Diameter (km), (from time to time coordinates of cell in the table), Type and test

Mati365 avatar Sep 12 '24 09:09 Mati365

My 2cs about inserting <div>s into <td>s to move contenteditable there. I think it's a no-go solution due to how it increases complexity and risks:

  • Those <div>s will be prone to styling issues. Margins, paddings, etc, will further affect the current layout. The bogus-paragraphs we use there already cause many issues.
  • Additional elements will potentially have impact on performance (more to render). Rendering large tables necessitate rendering huge numbers of elements already, and with such change we'd worsen this.

I'd test the other options proposed by @Comandeer. And one more idea (something we were considering with @niegowski many years ago): completely stopping using contenteditable=true/false for widgets (or at least tables). We intercept pretty much every interaction and our model/view guards potential selection states anyway. But this would be a massive change anyway and would require reviewing existing logic (bits might become unnecessary).

Reinmar avatar Feb 25 '25 22:02 Reinmar

One more thing: looking at https://github.com/ckeditor/ckeditor5/issues/16923#issuecomment-2289026967, are we actually sure that there's one solution that would satisfy all these engines? All seem to behave differently now.

What's the chance that by tuning the table's DOM structure we'll actually make all engines to announce tables correctly, taken for instance that they are a widget?

And finally, can't we simply use the options.label option of the toWidget() function to describe the element? Won't it in any way affect how the content is announced in this case?

Reinmar avatar Feb 25 '25 22:02 Reinmar

To summarize, we have the following options:

  • remove the textbox role – however, it could lead to loss of information about cells being editable; needs testing,
  • keep the current structure and provide the table info ourselves, or as @Reinmar put it can't we simply use the options.label option of the toWidget() function to describe the element?

Witoso avatar Feb 26 '25 07:02 Witoso

Plan:

  1. Prepare PoC for each approach
    1. div (use the previous poc, align with recent master)
    2. remove the textbox role
    3. additional widget label on a table (combined with textbox role from point 2.)
  2. Give it to tests (Narrator, NVDA, VoiceOver, JAWS)
    1. Entering table,
    2. Entering cells,
  3. For now, we keep contenteditable=true/false as backup.

Witoso avatar Feb 26 '25 10:02 Witoso

Before changes

https://github.com/user-attachments/assets/c352edaf-5271-4cdf-b43f-dfd260958a83


After changes

div with contenteditable within td.

PR: https://github.com/ckeditor/ckeditor5/pull/18086 POCer: https://pocer.ckeditor.dev/Microsoft%20Narrator%20tables%20PoC%20(extra%20divs%20within%20td)/2025-03-05%2009-37-39/ckeditor5-table/tests/manual/table.html

https://github.com/user-attachments/assets/60054854-f94e-463b-ad97-f980a259aa84

Remove the textbox role.

PR: https://github.com/ckeditor/ckeditor5/pull/18088 POCer: https://pocer.ckeditor.dev//Microsoft%20Narrator%20tables%20PoC%20(remove%20role%3Dtextbox)/2025-03-05%2009-48-36

https://github.com/user-attachments/assets/05f8b236-a47a-40f1-aaa4-dbd64d340398

toWidget with option label

I used toWidget() and label option and it is ignored. The default widget label is also ignored (Press Enter to type after or press Shift + Enter to type before the widget). It looks like Microsoft Narrator does not support this.

PR: https://github.com/ckeditor/ckeditor5/pull/18086 POCer: https://pocer.ckeditor.dev//Microsoft%20Narrator%20tables%20PoC%20(widget%20label)/2025-03-05%2009-56-06

aria-describedby on whole table / aria-label on cell

I checked these two attributes. aria-describedby is ignored on table and figure by Microsoft Narrator. aria-label is not ignored, thought.

aria-describedby.html.txt

Mati365 avatar Mar 05 '25 09:03 Mati365

@Mati365 please schedule the tests per

Give it to tests (Narrator, NVDA, VoiceOver, JAWS) Entering table, Entering cells,

Witoso avatar Mar 05 '25 09:03 Witoso

Done

Mati365 avatar Mar 05 '25 13:03 Mati365

Fyi, I reopened PRs. They were auto-closed after removing master-it84.

Mati365 avatar Mar 10 '25 07:03 Mati365

Comparison of behavior between current master branch and ck/narrator-remove-textbox-role on different screen readers:

master

Narrator

https://github.com/user-attachments/assets/3531ea19-f09a-4f36-916d-fd8f49dcd23b

NVDA

https://github.com/user-attachments/assets/af2f8f99-9d70-4d76-a500-c33772ba56d4

JAWS

https://github.com/user-attachments/assets/3d941049-8a5e-45a8-92b2-f078dcb9b031

VoiceOver @ Safari

https://github.com/user-attachments/assets/72f780ba-b0f1-4e71-a732-997428c97fd9

VoiceOver @ Chrome

https://github.com/user-attachments/assets/3d8407b0-f4af-464e-ab72-4e25f5da8563

ck/narrator-remove-textbox-role

Narrator

https://github.com/user-attachments/assets/217d306d-6396-4a5a-bdba-bd552749b32c

https://github.com/user-attachments/assets/8218770f-beec-495f-869e-08581dcbe02f

NVDA

https://github.com/user-attachments/assets/6a5d14b0-b919-4e9c-b473-ee8976ae61d7

https://github.com/user-attachments/assets/2573e648-685c-40c9-bc7e-45c987a6e3f3

JAWS

https://github.com/user-attachments/assets/08e2543d-15b2-40ac-b749-d079305dc868

https://github.com/user-attachments/assets/f23863ae-149d-45c1-b41b-23a971946e33

VoiceOver @ Safari

https://github.com/user-attachments/assets/9f9a13b3-48e7-45dc-9afc-d5d615f9716b

https://github.com/user-attachments/assets/6b411f13-8f46-4ae6-a826-4af3c11bcfb0

VoiceOver @ Chrome

https://github.com/user-attachments/assets/8ca1c875-06df-4412-b3df-d57e4a031321

https://github.com/user-attachments/assets/1e7b3a71-a470-4429-9369-27ecb7e3f70a

lkszzajac avatar Mar 18 '25 14:03 lkszzajac

As removing role=textbox doesn't degrade the experience in most screen readers, but rather improves it (with table dimensions appearing in NVDA, Narrator, JAWS) we decided to implement this solution.

Witoso avatar Mar 18 '25 14:03 Witoso