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-headers-attr): report headers attribute referencing other <td> elements as unsupported #4589

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
node_modules/
doc/api
test/integration/rules/td-headers-attr/td-headers-attr.html
2 changes: 1 addition & 1 deletion doc/rule-descriptions.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
| [server-side-image-map](https://dequeuniversity.com/rules/axe/4.10/server-side-image-map?application=RuleDescription) | Ensure that server-side image maps are not used | Minor | cat.text-alternatives, wcag2a, wcag211, section508, section508.22.f, TTv5, TT4.a, EN-301-549, EN-9.2.1.1 | needs&nbsp;review | |
| [summary-name](https://dequeuniversity.com/rules/axe/4.10/summary-name?application=RuleDescription) | Ensure summary elements have discernible text | Serious | cat.name-role-value, wcag2a, wcag412, section508, section508.22.a, TTv5, TT6.a, EN-301-549, EN-9.4.1.2 | failure, needs&nbsp;review | |
| [svg-img-alt](https://dequeuniversity.com/rules/axe/4.10/svg-img-alt?application=RuleDescription) | Ensure &lt;svg&gt; elements with an img, graphics-document or graphics-symbol role have an accessible text | Serious | cat.text-alternatives, wcag2a, wcag111, section508, section508.22.a, TTv5, TT7.a, EN-301-549, EN-9.1.1.1, ACT | failure, needs&nbsp;review | [7d6734](https://act-rules.github.io/rules/7d6734) |
| [td-headers-attr](https://dequeuniversity.com/rules/axe/4.10/td-headers-attr?application=RuleDescription) | Ensure that each cell in a table that uses the headers attribute refers only to other cells in that table | Serious | cat.tables, wcag2a, wcag131, section508, section508.22.g, TTv5, TT14.b, EN-301-549, EN-9.1.3.1 | failure, needs&nbsp;review | [a25f45](https://act-rules.github.io/rules/a25f45) |
| [td-headers-attr](https://dequeuniversity.com/rules/axe/4.10/td-headers-attr?application=RuleDescription) | Ensure that each cell in a table that uses the headers attribute refers only to other &lt;th&gt; elements in that table | Serious | cat.tables, wcag2a, wcag131, section508, section508.22.g, TTv5, TT14.b, EN-301-549, EN-9.1.3.1 | failure, needs&nbsp;review | [a25f45](https://act-rules.github.io/rules/a25f45) |
| [th-has-data-cells](https://dequeuniversity.com/rules/axe/4.10/th-has-data-cells?application=RuleDescription) | Ensure that &lt;th&gt; elements and elements with role=columnheader/rowheader have data cells they describe | Serious | cat.tables, wcag2a, wcag131, section508, section508.22.g, TTv5, TT14.b, EN-301-549, EN-9.1.3.1 | failure, needs&nbsp;review | [d0f69e](https://act-rules.github.io/rules/d0f69e) |
| [valid-lang](https://dequeuniversity.com/rules/axe/4.10/valid-lang?application=RuleDescription) | Ensure lang attributes have valid values | Serious | cat.language, wcag2aa, wcag312, TTv5, TT11.b, EN-301-549, EN-9.3.1.2, ACT | failure | [de46e4](https://act-rules.github.io/rules/de46e4) |
| [video-caption](https://dequeuniversity.com/rules/axe/4.10/video-caption?application=RuleDescription) | Ensure &lt;video&gt; elements have captions | Critical | cat.text-alternatives, wcag2a, wcag122, section508, section508.22.a, TTv5, TT17.a, EN-301-549, EN-9.1.2.2 | needs&nbsp;review | [eac66b](https://act-rules.github.io/rules/eac66b) |
Expand Down
84 changes: 50 additions & 34 deletions lib/checks/tables/td-headers-attr-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,63 +1,79 @@
import { tokenList } from '../../core/utils';
import { isVisibleToScreenReaders } from '../../commons/dom';
import { getRole } from '../../commons/aria';

// Order determines the priority of reporting
// Only if 0 of higher issues exists will the next be reported
const messageKeys = [
'cell-header-not-in-table',
'cell-header-not-th',
'header-refs-self',
'empty-hdrs' // incomplete
];
const [notInTable, notTh, selfRef, emptyHdrs] = messageKeys;

export default function tdHeadersAttrEvaluate(node) {
engineerklimov marked this conversation as resolved.
Show resolved Hide resolved
const cells = [];
const reviewCells = [];
const badCells = [];

const cellRoleById = {};
for (let rowIndex = 0; rowIndex < node.rows.length; rowIndex++) {
const row = node.rows[rowIndex];

for (let cellIndex = 0; cellIndex < row.cells.length; cellIndex++) {
cells.push(row.cells[cellIndex]);
const cell = row.cells[cellIndex];
cells.push(cell);

// Save header id to set if it's th or td with roles columnheader/rowheader
const cellId = cell.getAttribute('id');
if (cellId) {
cellRoleById[cellId] = getRole(cell);
}
}
}

const ids = cells
.filter(cell => cell.getAttribute('id'))
.map(cell => cell.getAttribute('id'));

const badCells = {
[selfRef]: new Set(),
[notInTable]: new Set(),
[notTh]: new Set(),
[emptyHdrs]: new Set()
};
cells.forEach(cell => {
let isSelf = false;
let notOfTable = false;

if (!cell.hasAttribute('headers') || !isVisibleToScreenReaders(cell)) {
return;
}

const headersAttr = cell.getAttribute('headers').trim();
if (!headersAttr) {
return reviewCells.push(cell);
badCells[emptyHdrs].add(cell);
return;
}

const cellId = cell.getAttribute('id');
// Get a list all the values of the headers attribute
const headers = tokenList(headersAttr);

if (headers.length !== 0) {
// Check if the cell's id is in this list
if (cell.getAttribute('id')) {
isSelf = headers.indexOf(cell.getAttribute('id').trim()) !== -1;
headers.forEach(headerId => {
engineerklimov marked this conversation as resolved.
Show resolved Hide resolved
if (cellId && headerId === cellId) {
// Header references its own cell
badCells[selfRef].add(cell);
} else if (!cellRoleById[headerId]) {
// Header references a cell that is not in the table
badCells[notInTable].add(cell);
} else if (
!['columnheader', 'rowheader'].includes(cellRoleById[headerId])
) {
// Header references a cell that is not a row or column header
badCells[notTh].add(cell);
}
});
});

// Check if the headers are of cells inside the table
notOfTable = headers.some(header => !ids.includes(header));

if (isSelf || notOfTable) {
badCells.push(cell);
for (const messageKey of messageKeys) {
if (badCells[messageKey].size > 0) {
this.relatedNodes([...badCells[messageKey]]);
if (messageKey === emptyHdrs) {
return undefined;
}
this.data({ messageKey });
return false;
}
});

if (badCells.length > 0) {
this.relatedNodes(badCells);
return false;
}

if (reviewCells.length) {
this.relatedNodes(reviewCells);
return undefined;
}

return true;
}
8 changes: 6 additions & 2 deletions lib/checks/tables/td-headers-attr.json
engineerklimov marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@
"metadata": {
"impact": "serious",
"messages": {
"pass": "The headers attribute is exclusively used to refer to other cells in the table",
"pass": "The headers attribute is exclusively used to refer to other header cells in the table",
engineerklimov marked this conversation as resolved.
Show resolved Hide resolved
"incomplete": "The headers attribute is empty",
"fail": "The headers attribute is not exclusively used to refer to other cells in the table"
"fail": {
"cell-header-not-in-table": "The headers attribute is not exclusively used to refer to other header cells in the table",
"cell-header-not-th": "The headers attribute must refer to header cells, not data cells",
"header-refs-self": "The element with headers attribute refers to itself"
}
}
}
}
4 changes: 2 additions & 2 deletions lib/rules/td-headers-attr.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
],
"actIds": ["a25f45"],
"metadata": {
"description": "Ensure that each cell in a table that uses the headers attribute refers only to other cells in that table",
"help": "Table cells that use the headers attribute must only refer to cells in 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."
},
"all": ["td-headers-attr"],
"any": [],
Expand Down
12 changes: 8 additions & 4 deletions locales/_template.json
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,8 @@
"help": "Non-empty <td> elements in larger <table> must have an associated table header"
},
"td-headers-attr": {
"description": "Ensure that each cell in a table that uses the headers attribute refers only to other cells in that table",
"help": "Table cells that use the headers attribute must only refer to cells in 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."
},
"th-has-data-cells": {
"description": "Ensure that <th> elements and elements with role=columnheader/rowheader have data cells they describe",
Expand Down Expand Up @@ -1096,9 +1096,13 @@
"fail": "Some non-empty data cells do not have table headers"
},
"td-headers-attr": {
"pass": "The headers attribute is exclusively used to refer to other cells in the table",
"pass": "The headers attribute is exclusively used to refer to other header cells in the table",
"incomplete": "The headers attribute is empty",
"fail": "The headers attribute is not exclusively used to refer to other cells in the table"
"fail": {
"cell-header-not-in-table": "The headers attribute is not exclusively used to refer to other header cells in the table",
"cell-header-not-th": "The headers attribute must refer to header cells, not data cells",
"header-refs-self": "The element with headers attribute refers to itself"
}
},
"th-has-data-cells": {
"pass": "All table header cells refer to data cells",
Expand Down
10 changes: 7 additions & 3 deletions locales/ru.json
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,8 @@
"help": "Непустые элементы <td> в больших таблицах должны иметь связанные заголовки таблицы"
},
"td-headers-attr": {
"description": "Убедитесь, что каждая ячейка в таблице, использующая атрибут headers, ссылается только на другие ячейки в этой таблице",
"help": "Ячейки таблицы, использующие атрибут headers, должны ссылаться только на ячейки в той же таблице"
"description": "Убедитесь, что каждая ячейка в таблице, использующая атрибут headers, ссылается только на другие элементы <th> в этой таблице",
"help": "Атрибуты headers ячеек таблицы должны ссылаться на другие элементы <th> в той же таблице."
},
"th-has-data-cells": {
"description": "Убедитесь, что элементы <th> и элементы с ролью columnheader/rowheader имеют ячейки данных, которые они описывают",
Expand Down Expand Up @@ -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.

"cell-header-not-in-table": "Атрибут headers не используется исключительно для ссылки на другие заголовочные ячейки в таблице",
"cell-header-not-th": "Атрибут headers должен ссылаться на заголовочные ячейки, а не на ячейки с данными",
"header-refs-self": "Элемент с атрибутом headers ссылается на самого себя"
}
},
"th-has-data-cells": {
"pass": "Все ячейки заголовков таблицы ссылаются на ячейки данных",
Expand Down
57 changes: 57 additions & 0 deletions test/checks/tables/td-headers-attr.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ describe('td-headers-attr', function () {
);
node = fixture.querySelector('table');
assert.isFalse(check.call(checkContext, node));
assert.deepEqual(checkContext._data, {
messageKey: 'cell-header-not-in-table'
});

fixtureSetup(
'<table id="hi">' +
Expand All @@ -102,6 +105,59 @@ describe('td-headers-attr', function () {
assert.isFalse(check.call(checkContext, node));
});

it('returns false if table cell referenced as header', function () {
fixtureSetup(`
<table>
<tr> <td id="hi">hello</td> </tr>
<tr> <td headers="hi">goodbye</td> </tr>
</table>
`);

var node = fixture.querySelector('table');
assert.isFalse(check.call(checkContext, node));
engineerklimov marked this conversation as resolved.
Show resolved Hide resolved
assert.deepEqual(checkContext._data, { messageKey: 'cell-header-not-th' });
});

it('returns true if table cell referenced as header with role rowheader or columnheader', function () {
var node;

fixtureSetup(`
<table>
<tr> <td role="rowheader" id="hi">hello</td> </tr>
<tr> <td headers="hi">goodbye</td> </tr>
</table>
`);

node = fixture.querySelector('table');
assert.isTrue(check.call(checkContext, node));

fixtureSetup(`
<table>
<tr> <td role="columnheader" id="hi">hello</td> </tr>
<tr> <td headers="hi">goodbye</td> </tr>
</table>
`);

node = fixture.querySelector('table');
assert.isTrue(check.call(checkContext, node));
});

it('relatedNodes contains each cell only once', function () {
fixtureSetup(`
<table>
<tr> <td id="hi1">hello</td> </tr>
<tr> <td id="hi2">hello</td> </tr>
<tr> <td id="bye" headers="hi1 hi2">goodbye</td> </tr>
</table>'
`);

var node = fixture.querySelector('table');
check.call(checkContext, node);
assert.deepEqual(checkContext._relatedNodes, [
fixture.querySelector('#bye')
]);
});

it('returns false if the header refers to the same cell', function () {
fixtureSetup(
'<table id="hi">' +
Expand All @@ -112,6 +168,7 @@ describe('td-headers-attr', function () {

var node = fixture.querySelector('table');
assert.isFalse(check.call(checkContext, node));
assert.deepEqual(checkContext._data, { messageKey: 'header-refs-self' });
});

it('returns true if td[headers] is hidden', function () {
Expand Down
20 changes: 20 additions & 0 deletions test/integration/rules/td-headers-attr/td-headers-attr.html
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
<td id="self" headers="self" hidden>World</td>
</table>

<table id="pass5">
<td role="rowheader" id="hdr1">Hello</td>
<td headers="hdr1">World</td>
</table>

<table id="fail1">
<th id="f1h1">Hello</th>
<td headers="f1h1 non-existing">World</td>
Expand All @@ -32,6 +37,21 @@
<td id="self" headers="self">World</td>
</table>

<table id="fail4">
<td id="hdr1">Hello</td>
<td headers="hdr1">World</td>
</table>
engineerklimov marked this conversation as resolved.
Show resolved Hide resolved

<table id="fail5">
<th role="cell" id="th-role-cell-hdr">Hello</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

So i updated .prettierignore, let me know if there is better way to fix it.

I'm pretty sure this was just a typo in his suggestion; this is intended to be a case exercising that the rule correctly fails if a headers attribute. points to a th element with a non-heading role, so I think it's correct for it to just be a th element. Shouldn't need to add it to .prettierignore, this is just prettier correctly catching a typo.

Suggested change
<th role="cell" id="th-role-cell-hdr">Hello</td>
<th role="cell" id="th-role-cell-hdr">Hello</th>

<td headers="th-role-cell-hdr">World</td>
</table>

<table id="fail6">
<th role="button" id="th-role-button-hdr">Hello</td>
<td headers="th-role-button-hdr">World</td>
</table>

<table id="inapplicable1" role="none">
<td id="self" headers="self">World</td>
</table>
Expand Down
11 changes: 9 additions & 2 deletions test/integration/rules/td-headers-attr/td-headers-attr.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
{
"description": "td-headers-attr test",
"rule": "td-headers-attr",
"violations": [["#fail1"], ["#fail2"], ["#fail3"]],
"passes": [["#pass1"], ["#pass2"], ["#pass3"], ["#pass4"]]
"violations": [
["#fail1"],
["#fail2"],
["#fail3"],
["#fail4"],
["#fail5"],
["#fail6"]
],
"passes": [["#pass1"], ["#pass2"], ["#pass3"], ["#pass4"], ["#pass5"]]
}