squizlabs / HTML_CodeSniffer

HTML_CodeSniffer is a client-side JavaScript application that checks a HTML document or source code, and detects violations of a defined coding standard. Comes with standards that cover the three conformance levels of the W3C's Web Content Accessibility Guidelines (WCAG) 2.0 and the U.S. Section 508 legislation.
https://squizlabs.github.io/HTML_CodeSniffer/
BSD 3-Clause "New" or "Revised" License
1.12k stars 246 forks source link

False positive on checks for headers in complex tables #287

Closed bdeclerc closed 4 years ago

bdeclerc commented 4 years ago

The check for headers in complex tables seems to produce false positives - it expects things that appear nonsensical.

On page https://www.w3.org/WAI/tutorials/tables/multi-level/

there's an example "complex" table with headers and Codesniffer appears to want to mix headers from different parts of the table:

"Incorrect headers attribute on this td element. Expected "c1 co1 co3" but found "co1 c1" is stated, but "co1 c1" is actually correct.

I noticed this running the test on https://accessibility.belgium.be/nl/artikels/tabellen-en-datavisualisatie/gebruik-de-juiste-code-om-bij-complexe-datatabellen-de - but there the error report is automagically in Dutch so I looked for another example in English. The effect there is the same, there are separate "rome" and "parijs" parts to the table and codesniffer wants to put both "rome" and "parijs" as headers where clearly only one of them needs to be.

ironikart commented 4 years ago

After looking at the code I don't think it was setup to handle tables with complex header relationships. Ideally we'd skip these and add a notice if we cannot determine the header relationships, or the other option would be to refactor the code to handle it. Just a heads up that this may not be fixed quickly.

bdeclerc commented 4 years ago

I fully understand - I don't really believe it's possible to code a general solution to this with absolute certainty, as the relationship between table headers and table columns/rows here will often be the consequence of human interpretation, not pure logic, so while it's probably feasible to detect clearly erroneous situations, there are likely to be many cases where the correctness of the setup cannot be made by a non-human construct.

So perhaps better to move it into notices (or at least try to limit it to "obviously wrong" stuff?) - certainly at this time it's causing false positives, which is very undesirable :)

ironikart commented 4 years ago

So perhaps better to move it into notices (or at least try to limit it to "obviously wrong" stuff?) - certainly at this time it's causing false positives, which is very undesirable :)

Agree with this and it's consistent with the approach of other compliance tests. The code to check headers is already quite complex and adding to it will probably make it more error prone.

stefanruijsenaars commented 4 years ago

Would something like this make sense @ironikart? https://github.com/squizlabs/HTML_CodeSniffer/pull/290

ironikart commented 4 years ago

I think so @stefanruijsenaars. My only reservation with the PR is that non-english translations will throw an error if encountered. I will have a look at that separately (will add translations in the other language files, but leave them as english and see if we can get translations at some point in the future).

stefanruijsenaars commented 4 years ago

Added translations

ironikart commented 4 years ago

I've merged #290 which handles the table as discussed here. Closing this issue.