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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 19 commits into from
+158 −47

Conversation

engineerklimov
Copy link
Contributor

@engineerklimov engineerklimov commented Sep 26, 2024 •
edited
Loading

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 engineerklimov mentioned this pull request Oct 2, 2024
Open
@engineerklimov
Copy link
Contributor Author

engineerklimov commented Oct 16, 2024 •
edited
Loading

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

WilcoFiers
WilcoFiers requested changes Oct 28, 2024
View reviewed changes
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
test/integration/rules/td-headers-attr/td-headers-attr.html Show resolved Hide resolved
@engineerklimov

This comment was marked as outdated.

Sign in to view
WilcoFiers
WilcoFiers requested changes Oct 31, 2024
View reviewed changes
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/integration/rules/td-headers-attr/td-headers-attr.html Outdated Show resolved Hide resolved
test/integration/rules/td-headers-attr/td-headers-attr.html 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.

WilcoFiers
WilcoFiers reviewed Nov 8, 2024
View reviewed changes
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
dbjorge
dbjorge requested changes Nov 8, 2024
View reviewed changes
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
engineerklimov
engineerklimov commented Nov 12, 2024
View reviewed changes
locales/ru.json
@@ -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.

dbjorge
dbjorge requested changes Nov 12, 2024
View reviewed changes
lib/rules/td-headers-attr.json Outdated Show resolved Hide resolved
@dbjorge dbjorge changed the title feat(td-ref-attr): report headers attribute referencing other <td> elements as unsupported feat(td-headers-attr): report headers attribute referencing other <td> elements as unsupported Nov 15, 2024
dbjorge
dbjorge requested changes Nov 15, 2024
View reviewed changes
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.

It looks like fmt_check is still showing 1 error to resolve, + it'd be good to do an npm run build to re-generate /locales/_template.json to match the most recent rule text update.

Other than those, this looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbjorge dbjorge dbjorge requested changes

@straker straker Awaiting requested review from straker

@WilcoFiers WilcoFiers Awaiting requested review from WilcoFiers

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Report headers attribute referencing other <td> elements as unsupported
4 participants
@engineerklimov @WilcoFiers @dbjorge @straker