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

Rule: Headers attribute only refers to cells in the same table (a25f45) #412

Merged
merged 95 commits into from
Feb 25, 2020

Conversation

jeeyyy
Copy link
Collaborator

@jeeyyy jeeyyy commented Jan 24, 2019

Rule: headers attribute only refers to cells in the same table

Closes issue: #406

Note:
For clear intents and purposes, the scope of this rule is kept to be fairly simple.
Intent is to author more rules for various other scenarios that involves this SC.

@jeeyyy jeeyyy changed the title [WIP] feat: intial rule in place [WIP] Rule: Data cells has a headers attribute Jan 28, 2019
@jeeyyy jeeyyy changed the title [WIP] Rule: Data cells has a headers attribute [WIP] Rule: Data cells has headers attribute Jan 28, 2019
@jeeyyy jeeyyy changed the title [WIP] Rule: Data cells has headers attribute [WIP] Rule: Headers attribute only refers to cells in the same table Jan 31, 2019
@jeeyyy jeeyyy changed the title [WIP] Rule: Headers attribute only refers to cells in the same table Rule: Headers attribute only refers to cells in the same table Jan 31, 2019
_rules/m29hgd.md Outdated Show resolved Hide resolved
_rules/m29hgd.md Outdated Show resolved Hide resolved
_rules/m29hgd.md Outdated Show resolved Hide resolved
@jeeyyy jeeyyy added the Rule Use this label for a new rule that does not exist already label Feb 12, 2019
Copy link
Member

@carlosapaduarte carlosapaduarte left a comment

Choose a reason for hiding this comment

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

Some minor requests for change.

_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
@jeeyyy jeeyyy changed the base branch from master to develop April 12, 2019 11:37
Copy link
Collaborator

@ShadowBB ShadowBB left a comment

Choose a reason for hiding this comment

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

Apart from the comment Wilco left about included in the accessibility tree I approve this rule.

_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
_rules/table-headers-attribute-refer-to-data-cells.md Outdated Show resolved Hide resolved
@jeeyyy jeeyyy dismissed stale reviews from ShadowBB and WilcoFiers January 22, 2020 11:24

Updated

@jeeyyy jeeyyy added the Review call 2 weeks Call for review for new rules and big changes label Feb 11, 2020
<th id="headerOfColumn">Projects</th>
</tr>
<tr>
<td headers="elmOutsideTable">15%</td>
Copy link
Member

@WilcoFiers WilcoFiers Feb 13, 2020

Choose a reason for hiding this comment

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

Every screen reader we've tested this in will look at row/column headers for fallback. That may not be what the algorithm says they should do, but someone testing with one of the top 4 AT / browser combinations is not going to see a problem here. I find this test case questionable. Here's the test case we ran a little while ago:

<table>
  <tr>
    <th scope="col">Projects</th>
    <th scope="col">Progress</th>
  </tr>
  <tr>
    <td headers="projects100">My Project</td>
    <td>15%</td>
  </tr>
</table>

IE + JAWS: Works
Chrome + JAWS: Works
Firefox + NVDA: Works
Safari + VO: Works

<th id="headerOfColumn">Projects</th>
</tr>
<tr>
<td headers="elmOutsideTable">15%</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this passes the SC (which I don't challenge), then the rule need to be changed to take fallback headers into account.
This certainly fails the rule right now, so if this passes the SC, the rule is bad.

And the rule in its current version is not referencing the assignation algorithm anymore…

(technically, the rule does assume that there is no fallback headers, but if this assumption is false for the vaste majority of cases, it needs to go away)

@jeeyyy
Copy link
Collaborator Author

jeeyyy commented Feb 17, 2020

Given the changes to the examples, I am adding another 1 week to this PR for final call, which will end on 24th Feb 2020

@jeeyyy jeeyyy merged commit a3c7e7f into develop Feb 25, 2020
@jeeyyy jeeyyy deleted the rule-table-headers-attr branch February 25, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review call 2 weeks Call for review for new rules and big changes reviewers wanted Rule Use this label for a new rule that does not exist already
Projects
None yet
Development

Successfully merging this pull request may close these issues.