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

feat(td-ref-attr): report headers attribute referencing other <td> elements as unsupported #4589

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

engineerklimov
Copy link
Contributor

@engineerklimov engineerklimov commented Sep 26, 2024

Fix for the header attribute check. The check will report cells that references other <td> elements. Added unit and integrations tests as requested in corresponding issue.

Closes: #3987

@engineerklimov engineerklimov marked this pull request as ready for review September 26, 2024 10:51
@engineerklimov engineerklimov requested a review from a team as a code owner September 26, 2024 10:51
@engineerklimov
Copy link
Contributor Author

engineerklimov commented Oct 16, 2024

Please, let me know if there are any changes needed. Thanks!
@WilcoFiers and @straker for vis.

@WilcoFiers
Copy link
Contributor

@engineerklimov Heyya, apologies about the delays. Lots to do. I'll try to get to it within the week.

@engineerklimov
Copy link
Contributor Author

Appreciate your response, Sir
Looking forward to the review

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

There are some scenarios the original ticket didn't consider that I feel we need to test before we can start failing this. Let me know if you have the necessary software to do this testing, otherwise we'll see what we can do on our end. At the very least we'll need to test this with NVDA + Firefox and JAWS + Chrome. Normally we'd also want VO + Safari but since it doesn't support the headers attribute anyway there's no point in that.

lib/checks/tables/td-headers-attr-evaluate.js Outdated Show resolved Hide resolved
lib/checks/tables/td-headers-attr-evaluate.js Outdated Show resolved Hide resolved
test/checks/tables/td-headers-attr.js Outdated Show resolved Hide resolved
@engineerklimov

This comment was marked as outdated.

lib/checks/tables/td-headers-attr-evaluate.js Outdated Show resolved Hide resolved
lib/checks/tables/td-headers-attr-evaluate.js Outdated Show resolved Hide resolved
test/checks/tables/td-headers-attr.js Outdated Show resolved Hide resolved
test/checks/tables/td-headers-attr.js Show resolved Hide resolved
lib/checks/tables/td-headers-attr-evaluate.js Show resolved Hide resolved
@WilcoFiers WilcoFiers changed the title fix(td-ref-attr): report headers attribute referencing other <td> elements as unsupported feat(td-ref-attr): report headers attribute referencing other <td> elements as unsupported Nov 1, 2024
@WilcoFiers
Copy link
Contributor

I'm going to change this PR title to "feat". This PR makes the rule stricter, we can't put that in a patch release.

@engineerklimov
Copy link
Contributor Author

@WilcoFiers I did all changes, the only thing left is locales updates. Can you check error messages? If they are fine i will update all translations.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

LGTM. Let me ask a colleague for a final OK, since I wrote part of the solve.

@WilcoFiers WilcoFiers self-requested a review November 8, 2024 13:01
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! This is looking good overall, just noticed a few more bits to clean up

lib/checks/tables/td-headers-attr-evaluate.js Show resolved Hide resolved
lib/checks/tables/td-headers-attr.json Outdated Show resolved Hide resolved
lib/checks/tables/td-headers-attr.json Show resolved Hide resolved
@@ -1098,7 +1098,11 @@
"td-headers-attr": {
"pass": "Атрибут headers используется исключительно для ссылки на другие ячейки таблицы",
"incomplete": "Атрибут headers пуст",
"fail": "Атрибут headers не используется исключительно для ссылки на другие ячейки таблицы"
"fail": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to update locales. I found that all locales are quite outdated. Not sure that this change is good place to bring all locales up to date. So i updated only "ru" which i contributed recently. Not sure what is contribution policy here.

Comment on lines +19 to +20
"description": "Ensure that each cell in a table using the headers attribute refers only to header cells within that table",
"help": "Table cells using the headers attribute must refer only to header cells or data cells with the role=columnheader/rowheader within the same table."
Copy link
Contributor

@dbjorge dbjorge Nov 12, 2024

Choose a reason for hiding this comment

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

I appreciate the attempt to make this more concrete for folks that might not know exactly what a "header cell" formally means. But for consistency with other rules, it's actually preferable for "help" to be the shorter one rather than "description" (the fields are poorly named; "help" is really more like "suggested tooltip", so we try to keep it more concise than this). For both of them, we try to keep the overall lengths consistent between rules and fairly concise; the old help text was already towards the wordier end of what we prefer. When I'm trying to decide on wording for these, I find it helps to dump all the properties for each rule alongside one another with jq .metadata.description lib/rules/*.json and jq .metadata.help lib/rules/*.json.

But I do agree that it's more understandable to use precise element/attribute names. I think we can actually get away with not mentioning the "role=" option in the text here; we only allow headers to point to a non-<th> element with header roles because it happens to be accessibility supported (ie, not a WCAG violation), but it's not actually compliant with the HTML standard (which requires pointing specifically to <th> elements), so I don't think we need the help text to encourage it as an option. In general, rule descriptions/help can and should omit mention of edge cases that we don't encourage but technically allow.

Suggested change
"description": "Ensure that each cell in a table using the headers attribute refers only to header cells within that table",
"help": "Table cells using the headers attribute must refer only to header cells or data cells with the role=columnheader/rowheader within the same table."
"description": "Ensure that each cell in a table that uses the headers attribute refers only to other <th> elements in that table",
"help": "Table cell headers attributes must refer to other <th> elements in the same table."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report headers attribute referencing other <td> elements as unsupported
4 participants