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

Check the number of h1 elements on block theme templates #468

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

matiasbenedetto
Copy link
Collaborator

@matiasbenedetto matiasbenedetto commented Sep 26, 2024

What?

Check the number of h1 elements on each block theme template.
This check only runs when checking block themes.

Results:

  • If one template has multiple h1 tags, it outputs a REQUIRED change error.
  • If a template has zero h1 tags, it outputs a WARNING.
  • If a template has only one h1 tag, it outputs nothing.

How

It iterates over each block of a template and looks for core/heading or core/post-title with level one to determine the number of h1 blocks that will be printed on a page.

Why?

To help theme creators use one h1 tag per template following accessibility recommendations like: https://www.a11yproject.com/posts/how-to-accessible-heading-structure/

Screenshot

Results in wp-admin:
image

Results in CLI:
image

@matiasbenedetto matiasbenedetto changed the title Checks the number of h1 elements on block theme templates Check the number of h1 elements on block theme templates Sep 26, 2024
@justintadlock
Copy link
Contributor

In general, I like the concept of this check, but I don't think it should produce a Required notice that would block submission of a theme to the directory.

Theme Check is primarily for ensuring that themes meet the WordPress.org Theme Review Guidelines. If we want to add a Required-level notice, it should first be discussed by the Themes Team, agreed upon, and added to the guidelines.

I have no issues with either of the first two items being a Warning-level notice for all block themes or even Required for themes with the accessibility-ready tag (they're part of the a11y requirements).

@matiasbenedetto
Copy link
Collaborator Author

I have no issues with either of the first two items being a Warning-level notice for all block themes or even Required for themes with the accessibility-ready tag (they're part of the a11y requirements).

Thanks for pointing this out @justintadlock. I agree with making both WARNING instead of REQUIRED following the a11y requirements linked. I changed that here: c05ac27

@aristath
Copy link
Member

aristath commented Oct 2, 2024

Multiple H1 headings is not something we should warn against... It's a perfectly valid pattern and does have valid use cases

@matiasbenedetto
Copy link
Collaborator Author

Multiple H1 headings is not something we should warn against... It's a perfectly valid pattern and does have valid use cases

@aristath The a11y requirements from the Theme Handbook says something different:

Failure criteria:
These are specific criteria that are used to judge whether your hierarchy meets our requirements:
No H1 element present on the page.
More than two H1 headings on a single page.

Should we modify the a11n requirements or add this to the plugin? I'm not sure.

@carolinan
Copy link
Collaborator

What is going to count as a template?
Because many templates today are technically PHP pattern files.

@aristath
Copy link
Member

aristath commented Oct 9, 2024

Should we modify the a11n requirements or add this to the plugin? I'm not sure.

The "Single H1 heading" rule is mostly a remnant of the HTML4 days...
Multiple H1 headings are valid and in some cases, they just make more sense for the content of the page in question.
A11y-wise it will surely be a problem for 15-year-old screen-readers, but in those systems, a lot of the HTML5 stuff WP uses will be an issue as well, so that should not be the main criterion.
For a few years, there was also the myth that multiple h1 headings are bad for SEO, but that has also been debunked several times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants