Allow first headings to be level 2 #285

JoshTumath · 2018-12-12T16:41:11Z

Summary

This modifies the 'First heading was not a main heading' check so that h2 (as well as h1 headings) are allowed to appear first. Authors are still required to put headings in ascending order, but DOM structures like the example below will no longer cause a warning:

<body>
  <h2>My website</h2>

  <h1>Article headline</h1>
  <h2>Article subheading</h2>

  <h2>Other popular stories</h2>
</body>

Note: This PR also changes npm test to run two node_modules via npx.

Motivation and Context

This is a PR to resolve #284.

How Has This Been Tested?

I created an additional acceptance test, but please let me know if another type of test should be added.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

I've added tests for my code
My change requires a change to the documentation.
I have updated the documentation accordingly.

JoshTumath · 2018-12-12T16:52:00Z

The PR has failed on the CI because the CI is running Node 7, but the package.json of bbc-a11y mentions that it requires Node 8 and above. If I remember correctly, npx was bundled with Node 8 onwards. Is anyone able to help with this?

  "engines": {
    "node": ">=8.x"
  }

murkas

Looks all good to me - I'll definitely appreciate this change as Orbit has also been causing this test to fail for BBC Three 👍I can't speak in much depth for the change to the node version but it now matches package.json, so this should be beneficial, I should imagine!

ChrisBAshton

Great work, @JoshTumath. Practically ready for merge already!

Just a couple of suggestions for tweaks.

Thank you for raising this PR.

ChrisBAshton · 2018-12-21T13:39:21Z

features/support/web_server/two_headings_failures_and_one_warning.html

@@ -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</h4>


Suggested change

<h4>This is the main heading, but it's not an H1</h4>

<h4>This is the main heading, but it's not an H1 or H2</h4>

ChrisBAshton · 2018-12-21T13:39:34Z

lib/standards/tests/contentMustFollowHeadings.js

@@ -1,4 +1,4 @@
-var headingSelector = 'h1, h2, h3, h4, h5, h6, h7, h8'
+var headingSelector = 'h1, h2, h3, h4, h5, h6'


Thanks for fixing these 👍

ChrisBAshton · 2018-12-21T13:43:13Z

features/cli/command_line_help.feature

@@ -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 it should contain with whitespace removed:


I like what you've done here - but would re-word it slightly. Perhaps

Suggested change

Then it should contain with whitespace removed:

Then (ignoring whitespace) it should pass with:

Oh my gosh that's so much better. I had a very hard time thinking of a good way to phrase this.

EmmaJP · 2018-12-24T13:24:40Z

HISTORY.md

@@ -1,3 +1,7 @@
+# v2.3.0
+
+- Allow first heading to be level 2


Could this be rephrased please.

The change is not requiring the <h1> be the first heading in the page, rather than permitting a specific heading level to be the first heading.

EmmaJP · 2018-12-24T13:28:12Z

features/cli/display_failing_result.feature

@@ -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



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

EmmaJP

Thanks for working on this @JoshTumath ... I hit review too soon. Anyhow, I've left some comments, as I think this change may need a slightly different approach.

If there are headings before the <h1> would it be worth checking if the <h1> is the first heading within the main content container and if so simply ignoring earlier headings for other heading checks.

@ChrisBAshton how do you think that would work with the other checks? I seem to recall you did some work on this a while back.

EmmaJP · 2018-12-24T13:29:26Z

features/cli/display_failing_result.feature

@@ -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


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

EmmaJP · 2018-12-24T13:30:42Z

features/cli/display_failing_result.feature

@@ -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
-          - First heading was not a main heading: /html/body/h2
+          - First heading was not a level 1 or level 2 heading: /html/body/h4


See previous comments

EmmaJP · 2018-12-24T13:32:26Z

features/standards/mag/structure/02_headings.feature

@@ -140,7 +140,19 @@ Feature: Headings
    Then it passes

  @html @automated
-  Scenario: Subheading before the first main heading
+  Scenario: Level 2 heading before the first main heading


I think "Subheading before the main heading" was more in keeping with the guidance

EmmaJP · 2018-12-24T13:36:23Z

features/standards/mag/structure/02_headings.feature

@@ -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


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.

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?

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?

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.

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?

JoshTumath · 2019-01-03T12:01:25Z

Thank you very much for the review comments! I have implemented some of them, but some are dependant on a resolution to our discussion above about whether we should care about heading hierarchy above the h1 element.

EmmaJP

It would be good to resolve this PR.

HISTORY.md

EmmaJP · 2019-11-25T16:16:02Z

features/support/web_server/two_headings_failures_and_one_warning.html

@@ -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>


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.

github-actions · 2023-08-12T01:46:41Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2023-08-22T01:47:22Z

This PR was closed because it has been stalled for 10 days with no activity.

github-actions · 2023-08-27T02:08:30Z

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

JoshTumath added 2 commits December 12, 2018 12:56

Run tests using npx

1b35e88

Allow first heading to be level 2

94f9091

JoshTumath added 3 commits December 12, 2018 17:13

Use Node 8 in Travis CI

30c5e9e

Fix CLI acceptance tests

db1857d

Remove the forbidden semicolon

f9db4a5

murkas approved these changes Dec 14, 2018

View reviewed changes

ChrisBAshton reviewed Dec 21, 2018

View reviewed changes

EmmaJP reviewed Dec 24, 2018

View reviewed changes

EmmaJP suggested changes Dec 24, 2018

View reviewed changes

EmmaJP reviewed Dec 24, 2018

View reviewed changes

Resolved various review comments

523e38e

EmmaJP added the review-needed PR needs to be reviewed by at least one other collaborator label Nov 12, 2019

EmmaJP suggested changes Nov 25, 2019

View reviewed changes

Andy Smith added 2 commits July 17, 2020 11:32

Merge remote-tracking branch 'origin' into first-heading-level-2

a6ce7bb

Revert unrelated changes

7734204

github-actions bot added the Stale label Aug 12, 2023

github-actions bot closed this Aug 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow first headings to be level 2 #285

Allow first headings to be level 2 #285

JoshTumath commented Dec 12, 2018 •

edited

Loading

JoshTumath commented Dec 12, 2018

murkas left a comment

ChrisBAshton left a comment

ChrisBAshton Dec 21, 2018

ChrisBAshton Dec 21, 2018

ChrisBAshton Dec 21, 2018

JoshTumath Jan 3, 2019

EmmaJP Dec 24, 2018 •

edited

Loading

EmmaJP Dec 24, 2018

EmmaJP left a comment •

edited

Loading

EmmaJP Dec 24, 2018

EmmaJP Dec 24, 2018

EmmaJP Dec 24, 2018

EmmaJP Dec 24, 2018

ChrisBAshton Dec 28, 2018 •

edited

Loading

JoshTumath Jan 3, 2019

EmmaJP Jan 3, 2019 •

edited

Loading

JoshTumath Jan 4, 2019 •

edited

Loading

JoshTumath commented Jan 3, 2019

EmmaJP left a comment

EmmaJP Nov 25, 2019

github-actions bot commented Aug 12, 2023

github-actions bot commented Aug 22, 2023

github-actions bot commented Aug 27, 2023

	<h4>This is the main heading, but it's not an H1</h4>
	<h4>This is the main heading, but it's not an H1 or H2</h4>

		@@ -1,4 +1,4 @@
		var headingSelector = 'h1, h2, h3, h4, h5, h6, h7, h8'
		var headingSelector = 'h1, h2, h3, h4, h5, h6'

	Then it should contain with whitespace removed:
	Then (ignoring whitespace) it should pass with:

Allow first headings to be level 2 #285

Allow first headings to be level 2 #285

Conversation

JoshTumath commented Dec 12, 2018 • edited Loading

Summary

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

JoshTumath commented Dec 12, 2018

murkas left a comment

Choose a reason for hiding this comment

ChrisBAshton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaJP Dec 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaJP left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisBAshton Dec 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaJP Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

JoshTumath Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

JoshTumath commented Jan 3, 2019

EmmaJP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2023

github-actions bot commented Aug 22, 2023

github-actions bot commented Aug 27, 2023

JoshTumath commented Dec 12, 2018 •

edited

Loading

EmmaJP Dec 24, 2018 •

edited

Loading

EmmaJP left a comment •

edited

Loading

ChrisBAshton Dec 28, 2018 •

edited

Loading

EmmaJP Jan 3, 2019 •

edited

Loading

JoshTumath Jan 4, 2019 •

edited

Loading