Skip to content

Conversation

@catenglaender
Copy link
Contributor

@catenglaender catenglaender commented Apr 30, 2025

Issue

DataTable and OrderingTable UI Component did not pass W3C Validator.

image

Changes

image

I went on a mysterious journey into the depth of W3C and MDN 🧙‍♂️

There is a bug in the W3C validator: If you use aria-colindex on <td>, everything explodes with very unhelpful errors for the table header. The actual problem was that you can't have aria-colindex on <td> if you also have it on <th>, but none of the errors said that. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-colindex

Furthermore many, many roles are not necessary and even discouraged on <table>, <tr> and <td> in a display: table; context, so I simplified some tags. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/table_role

aria-colindex and aria-colcount and the colspan for the multi select footer were calculated incorrectly as

  1. aria-colindexstarts counting with 1 and doesn't allow 0.
  2. we add many columns that are not in the actual table data (multi-select & drag and drop column, position column, single select action column).

The logic is a bit hacky, as the OrderingTable uses the same column for multi-select and the drag and drop handle.

Input/Viewcontrol Fieldselection had a button in an <ul> not wrapped in <li>.

Not quite done

I will do the Unit Tests after code review, when we are sure the html structure will not change.

The Kitchen Sink page of the Ordering Table still fails to pass because something is not right with the UI form input select-field-input setting selected twice. That's another issue though.

@dsstrassner
Copy link
Contributor

I hope the assignments are correct.

https://mantis.ilias.de/view.php?id=44065 and https://mantis.ilias.de/view.php?id=44005

@Amstutz
Copy link
Contributor

Amstutz commented May 1, 2025

Thx Denis, afaik macht bei CaT Arbeiten @klees immer zuerst das interne Review.

klees
klees previously requested changes May 2, 2025
Copy link
Contributor

@klees klees left a comment

Choose a reason for hiding this comment

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

Hi @catenglaender,

aaah, look at us, not taking care or knowing s**t about accessibility ^^

Please answer the following questions:

  • grid-role: If I remember correctly, that role was the key to allow for movement via arrow-keys in the table. Did you check for that feature after removal of the grid role?

The rest looks good, thanks so much for the investigation and improvements.

Best regards!

@catenglaender catenglaender force-pushed the _10/UI/Table_accessibility-validation branch from e8062c1 to 88d8f9c Compare May 13, 2025 15:48
@catenglaender
Copy link
Contributor Author

Thanks for the input! 🙂

  • I restored the grid role (gridcell is not needed for <td> as far as I understand the best practices)
  • Unit tests are updated
  • @lukastocker will commit an even better fix for the viewcontrol dropdown in a separate PR related to another issue

@catenglaender catenglaender force-pushed the _10/UI/Table_accessibility-validation branch from 88d8f9c to 9c1ef9e Compare May 13, 2025 16:53
@catenglaender catenglaender requested a review from klees May 14, 2025 07:42
@Amstutz
Copy link
Contributor

Amstutz commented May 21, 2025

thx for the improvements. @klees is this ready for the coordinators to take a look at?

@Amstutz
Copy link
Contributor

Amstutz commented May 23, 2025

Hi @catenglaender

thx a lot!

  • I am afraid a recent merge generated a conflict. Can you resolve this?

Thx a lot

@catenglaender catenglaender force-pushed the _10/UI/Table_accessibility-validation branch from 9c1ef9e to d4fe87c Compare May 26, 2025 15:44
@catenglaender
Copy link
Contributor Author

✔️ Resolved merge conflicts
❗ Failing Unit Tests are unrelated to the files in this PR
@kergomard I took out a change of yours, where you fixed the alignment of the header and cells using the template block header_rowselection_ordering_no_multi_actions, because the amount of columns and content shown when there are no multi actions or the drag and drop is disabled has changed a bit. It's correct now as far I can tell from the examples (see screenshots below) but if you know of other cases I should test, please let me know.
image
image

@kergomard
Copy link
Contributor

Hi @catenglaender
I didn't check your changes out and test them, but as far as I understood the code your changes make my changes unnecessary.

Best,
@kergomard

@catenglaender
Copy link
Contributor Author

Thanks for your input! Very much appreciated.

Alright, this one is good to go then :)

@Amstutz Amstutz merged commit 85ff577 into ILIAS-eLearning:release_10 May 28, 2025
1 of 3 checks passed
Amstutz pushed a commit that referenced this pull request May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants