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

Allow first headings to be level 2 #285

Closed
wants to merge 8 commits into from
Closed

Allow first headings to be level 2 #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1b35e88
Run tests using npx
JoshTumath Dec 12, 2018
94f9091
Allow first heading to be level 2
JoshTumath Dec 12, 2018
30c5e9e
Use Node 8 in Travis CI
JoshTumath Dec 12, 2018
db1857d
Fix CLI acceptance tests
JoshTumath Dec 13, 2018
f9db4a5
Remove the forbidden semicolon
JoshTumath Dec 13, 2018
523e38e
Resolved various review comments
JoshTumath Jan 3, 2019
a6ce7bb
Merge remote-tracking branch 'origin' into first-heading-level-2
Jul 17, 2020
7734204
Revert unrelated changes
Jul 17, 2020
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
2 changes: 1 addition & 1 deletion features/cli/command_line_help.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Feature: Command Line Help

Scenario: Asking the command line tool for help
When I run `bbc-a11y --help`
Then it should pass with:
Then (ignoring whitespace) it should pass with:
"""
Usage: bbc-a11y [options] <urls>

Expand Down
4 changes: 2 additions & 2 deletions features/cli/coverage_reports.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,15 +237,15 @@ Feature: Summarise Tests

Structure: Headings: Headings must be in ascending order
Type: automated
Fails for each visible heading (<h2> to <h8>) that does follow the next highest-level heading. For example, a <h3> can only come after a <h2>
Fails for each visible heading (<h2> to <h6>) that does follow the next highest-level heading. For example, a <h3> can only come after a <h2>

Structure: Headings: Exactly one main heading
Type: automated
Fails for each visible <h1> element except the very first one

Structure: Headings: Content must follow headings
Type: automated
Fails for each heading element (<h1> up to <h8>) that has no text content after it. Text elements may appear as children or descendants of subsequent siblings
Fails for each heading element (<h1> up to <h6>) that has no text content after it. Text elements may appear as children or descendants of subsequent siblings

Structure: Containers and landmarks: Containers should be used to describe page structure
Type: manual
Expand Down
6 changes: 3 additions & 3 deletions features/cli/display_failing_result.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Feature: Display failing result
"""
✓ http://localhost:54321/subheading_first.html
⚠ Structure: Headings: Headings must be in ascending order
- First heading was not a main heading: /html/body/h3
- First heading was not a level 1 or level 2 heading: /html/body/h3

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to remove this test, rather than be specific about what level the first heading is.

For details on how to fix these errors, please see the following pages:
- http://www.bbc.co.uk/guidelines/futuremedia/accessibility/mobile/structure/headings
Expand All @@ -33,11 +33,11 @@ Feature: Display failing result
"""
✗ http://localhost:54321/two_headings_failures_and_one_warning.html
⚠ Structure: Headings: Headings must be in ascending order
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly change to "Headings after the h1 must be in hierarchical order'

- First heading was not a main heading: /html/body/h2
- First heading was not a level 1 or level 2 heading: /html/body/h4
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comments

* Structure: Headings: Exactly one main heading
- Found 0 h1 elements.
* Structure: Headings: Content must follow headings
- No content follows: /html/body/h2
- No content follows: /html/body/h4

For details on how to fix these errors, please see the following pages:
- http://www.bbc.co.uk/guidelines/futuremedia/accessibility/mobile/structure/headings
Expand Down
14 changes: 13 additions & 1 deletion features/standards/mag/structure/02_headings.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,18 @@ Feature: Headings

@html @automated
Scenario: Subheading before the first main heading
Given a page with the body:
"""
<h2>Heading 2a</h2>
<h3>Heading 3</h3>
<h1>Heading 1</h1>
<h2>Heading 2b</h2>
"""
When I test the "Structure: Headings: Headings must be in ascending order" standard
Then it passes

@html @automated
Scenario: Level 3 heading before the first main heading
Given a page with the body:
"""
<h3>Ignore me</h3>
Expand All @@ -150,7 +162,7 @@ Feature: Headings
When I test the "Structure: Headings: Headings must be in ascending order" standard
Then it passes with the warning:
"""
First heading was not a main heading: /html/body/h3
First heading was not a level 1 or level 2 heading: /html/body/h3
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not in keeping with the guidance. The requirement is for only one <h1> per page, and a hierarchical heading structure following that <h1>. There is no specific guidance before the <h1> in regard heading levels. Ideally there would be no headings before the <h1>, and I would certainly wish to encourage <h1> to be the first heading within the main content container.

Copy link
Contributor

@ChrisBAshton ChrisBAshton Dec 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There is no specific guidance before the <h1> in regard heading levels

Just to double-check that: I can get on board with the h1 not being the first heading in the page, keeping a h2 for Accessibility links and saving the h1 for the actual page content. That makes sense to me.

But if we were to arbitrarily use a h3, h4, h5 or h6 as the first heading in the page, that stops making sense!

Is that not worth scanning for and warning against?

Copy link
Author

Choose a reason for hiding this comment

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

I also agree with @ChrisBAshton.

However, it's true that there isn't really any guidance on this, and the passage in the HTML specification for sections and headings isn't even implemented in browsers (referring to the 'outline algorithm').

In an ideal world, the main article/content heading be determined by the fact that it's the top-level headline in the main content, rather than the heading level being h1; and the h1 would always be the title of the website (e.g. BBC).

There's a big discussion around this at the WHATWG; see whatwg/html#3499

But it's going to be years since we have a definitive resolution to this issue. Maybe recommending hierarchy before the h1 is the best compromise?

Copy link
Contributor

@EmmaJP EmmaJP Jan 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I understand where you're coming from @ChrisBAshton but the one errant subheading in Orb could be an <h4> rather than an <h2> and that should make no difference. The guidance isn't permitting headings before the <h1> but the test doesn't have to fail if there is a preceding subheading, so long as the heading structure after the <h1> is hierarchical.

And this isn't trying to set any kind of precedent, as the WCAG guideline is different again in its requirements. This is simply dealing with the BBC situation and guideline.

@JoshTumath the guideline to have one <h1> per page is to help visitors locate themselves within the site. If every <h1> across the site was "BBC" it could be very confusing.

Copy link
Author

@JoshTumath JoshTumath Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I understand where you're coming from ChrisBAshton but the one errant subheading in Orb could be an h4 rather than an h2 and that should make no difference.

My opinion is that we should encourage users of the bbc-a11y tool to use the ignore functionality in cases like that, rather than changing the logic. (But in the case of Orbit, it always seems to stick to using h2.)

Actually, my opinion is changing on this. The W3C guidelines on this seem contradictory or ambiguous in terms of how strict we need to be with the heading hierarchy. If we were to change the logic so that we don't test the heading hierarchy before the h1 element, does this mean we (theoretically) also aren't interested in testing any headings in the footer of the page?

"""

@html @automated
Expand Down
15 changes: 12 additions & 3 deletions features/step_definitions/a11y_steps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,19 @@ Then('it should fail with exactly:', function (expectedOutput) {
assert.equal(sanitisedActualOutput, expectedOutput, 'Expected:\n' + expectedOutput.replace(/\n/g, '[\\n]\n') + '\nActual:\n' + sanitisedActualOutput.replace(/\n/g, '[\\n]\n'))
})

Then('it should pass with:', function (string) {
Then('it should pass with:', function (expectedOutput) {
var actualOutput = (this.stdout + this.stderr)
if (actualOutput.indexOf(string) === -1) {
throw new Error('Expected:\n' + string + '\nActual:\n' + actualOutput)
if (actualOutput.indexOf(expectedOutput) === -1) {
throw new Error('Expected:\n' + expectedOutput + '\nActual:\n' + actualOutput)
}
assert.equal(this.exitCode, 0)
})

Then(/\(ignoring whitespace\) it should pass with:/, function (expectedOutput) {
var trimmedExpectedOutput = expectedOutput.replace(/\s/g, '')
var trimmedActualOutput = (this.stdout + this.stderr).replace(/\s/g, '')
if (trimmedActualOutput.indexOf(trimmedExpectedOutput) === -1) {
throw new Error('Expected:\n' + trimmedExpectedOutput + '\nActual:\n' + trimmedActualOutput)
}
assert.equal(this.exitCode, 0)
})
Expand Down
2 changes: 1 addition & 1 deletion features/support/web_server/two_headings_failures_and_one_warning.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
<body>
<p role="main">
<!-- there is no content following the heading below -->
<h2>This is the main heading, but it's not an H1</h2>
<h4>This is the main heading, but it's not an H1 or H2</h4>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an example of no <h1> or no content following the heading? It would be good to ensure examples only fail for the reason they illustrate.

</body>
</html>
4 changes: 2 additions & 2 deletions lib/standards/tests/contentMustFollowHeadings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var headingSelector = 'h1, h2, h3, h4, h5, h6, h7, h8'
var headingSelector = 'h1, h2, h3, h4, h5, h6'
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing these 👍

var iframeSelector = 'iframe:visible'

var NODE_IS_FINE = 'node-is-fine'
Expand All @@ -9,7 +9,7 @@ module.exports = {

type: 'automated',

failsForEach: 'heading element (<h1> up to <h8>) that has no text content after it. ' +
failsForEach: 'heading element (<h1> up to <h6>) that has no text content after it. ' +
'Text elements may appear as children or descendants of subsequent siblings',

test: function ({ $, fail }) {
Expand Down
16 changes: 9 additions & 7 deletions lib/standards/tests/headingsMustBeInAscendingOrder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,24 @@ module.exports = {

type: 'automated',

failsForEach: 'visible heading (<h2> to <h8>) that does follow the next' +
failsForEach: 'visible heading (<h2> to <h6>) that does follow the next' +
' highest-level heading. For example, a <h3> can only come after' +
' a <h2>',

test: function ({ $, fail, warn }) {
var headings = $('h1, h2, h3, h4, h5, h6, h7, h8')
var headings = $('h1, h2, h3, h4, h5, h6')
var headingLevels = headings.map(function (index, heading) {
return { heading: heading, level: parseInt(heading.tagName[1]) }
})

eachCons(headingLevels, 2).forEach(function (pair) {
if (pair[1].level > (pair[0].level + 1)) {
fail('Headings are not in order:', pair[0].heading, pair[1].heading)
}
})
if (headingLevels.length > 0 && headingLevels[0].level > 1) {
warn('First heading was not a main heading:', headingLevels[0].heading)

if (headingLevels.length > 0 && headingLevels[0].level > 2) {
warn('First heading was not a level 1 or level 2 heading:', headingLevels[0].heading)
}
}
}
Expand All @@ -32,9 +34,9 @@ function eachCons (a, n) {
}

function range (a, i, n) {
var r = []
var array = []
for (var j = 0; j < n; j++) {
r.push(a[i + j])
array.push(a[i + j])
}
return r
return array
}
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "bbc-a11y",
"version": "2.4.2",
"version": "2.4.1",
"description": "BBC Accessibility standards checker",
"main": "./bin/bbc-a11y.js",
"directories": {},
Expand Down