Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48126 closed defect (bug) (worksforme)

Twenty Nineteen: Duplicate <h1> tags

Reported by: williampatton's profile williampatton Owned by: adamsilverstein's profile adamsilverstein
Milestone: Priority: normal
Severity: normal Version: 5.3
Component: Bundled Theme Keywords: has-patch
Focuses: accessibility Cc:

Description (last modified by williampatton)

In twenty nineteen theme there are situations where duplicate <h1> tags appear on the page. It is best to have just a single <h1> on page for various reasons.

Some details of what was discovered in twenty twenty: https://github.com/WordPress/twentytwenty/issues/490 and https://github.com/WordPress/twentytwenty/pull/496

The same issue affects twentyninteen. This ticket should be updated with more relevant details.

Attachments (1)

48126.diff (881 bytes) - added by adamsilverstein 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @williampatton
5 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core-themes by david.baumwald. View the logs.


5 years ago

#3 @davidbaumwald
5 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Version set to trunk

#4 @adamsilverstein
5 years ago

  • Focuses accessibility added

#5 @adamsilverstein
5 years ago

@williampatton Do you have any more details/steps to reproduce? I was unable to reproduce this in twentynineteen, are you sure this is an issue?

I tried to reproduce by setting the homepage to a page, when I visited the page title was wrapped in an h1 while the site title was wrapped in <p class="site-title">

Checking the code I see we check:

is_front_page() && is_home() before outputting an h1 - which should only be true when the homepage shows the latest posts in my testing. maybe I am missing some setup step?

And in the docs I found that:

is_home If a static page is set for the front page of the site, this function will return true only on the page you set as the "Posts page". I verified this is correct, even when i set both the posts and home page to the same page, I couldn't get both conditions to be true.

Can you please add exact details about how do you create a condition where you get double h1s and verify this in fact happens in twentynineteen?

#6 @sabernhardt
5 years ago

I could not find double h1 tags either. However, my special page of posts was missing the h1 heading.

Home page (latest posts): h1 tag on site link, h2 for each post.
Posts page: no h1 tag on site link, no h1 for content, h2 for each post.
Home page (static): no heading around site link, one h1 for page heading ("Home").
Single post/page: no heading tag on site link, one h1 for page heading.
Category archive: no heading tag on site link, one h1 for page heading, h2 for each post.

#7 @adamsilverstein
5 years ago

Thanks for testing and the detailed notes @sabernhardt!

Not having an h1 isn't great, however probably much less concerning than multiple h1s on the page (because h1 is considered the header for the entire document).

I still don't understand why this wasn't working in twentytwenty and required https://github.com/WordPress/twentytwenty/pull/496

Last edited 5 years ago by adamsilverstein (previous) (diff)

#8 @adamsilverstein
5 years ago

  • Keywords has-patch needs-testing reporter-feedback added

Aha! Figured it out. I was misreading the code, in twentynineteen we had && instead of || in the PR.. as in is_front_page() && is_home().

What really want is to use the h1 when either of these are true (the home page or the blog front page), eg is_front_page() || is_home() except when the home page is a static page (! is_page()) - the exact fix in https://github.com/WordPress/twentytwenty/commit/6306c74f6ecf5992811538c915c64e0585b22354

I added this in 48126.diff

@sabernhardt / @williampatton can you please give that patch a test and see if you get a single h1 on your homepage?

#9 @adamsilverstein
5 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#10 @williampatton
5 years ago

  • Keywords needs-testing reporter-feedback removed

The patch applies for me and I do not get any duplicate h1 tags with any of the test scenarious I can think of.

Thanks for taking care of this @adamsilverstein :)

#11 @sabernhardt
5 years ago

The Posts page now has an h1 on the site title; otherwise, my test pages are the same after the patch as what I previously reported. So that would still be zero doubles found, without considering child themes.

One child theme possibility: The Posts page template (home.php) uses single_post_title() to add the page name as a heading above all the posts, which would then be the second h1. Simplifying the conditional to if ( is_front_page() && ! is_page() ) could help in that case.

#12 @adamsilverstein
5 years ago

@sabernhardt does simplifying the logic that way still work for all use cases? Do we need the is_home check for the case where the posts list is the home page?

#13 @SergeyBiryukov
5 years ago

  • Summary changed from Duplicate <h1> tags in twentynineteen to Twenty Nineteen: Duplicate <h1> tags

#14 @sabernhardt
5 years ago

@adamsilverstein Well, no, my suggestion of simply removing is_home() does not work in all cases. If I use the child theme scenario I mentioned earlier, then switch back to the default home page of posts without editing/removing the home.php template, I end up with an empty h1 on the home page plus the site-title h1. (Even though this is an edge case, it does show there's a way to break it.)

I can test for another way to fix child theme scenarios, but we should also consider whether the original *needs* fixing. Twenty Fifteen and Twenty Sixteen use the same if ( is_front_page() && is_home() ) logic in their header.php files. That seems to give the site-title a heading tag only in the intended case of a blog-style home page, and I have not seen any different in Twenty Nineteen either. It's also described as the "right" way to target the default homepage template in this Stack Exchange answer:
https://wordpress.stackexchange.com/questions/224592/if-is-home-is-front-page#239838

Could someone describe an extra h1 instance with (the unpatched) Twenty Nineteen so we can test whether a patch fixes that scenario first? Understanding the problem would be worth more than my hypothetical situations :).

Last edited 5 years ago by sabernhardt (previous) (diff)

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


5 years ago

#16 @adamsilverstein
5 years ago

  • Milestone 5.3 deleted
  • Resolution set to worksforme
  • Status changed from assigned to closed

After reviewing the changes here more carefully, I agree with @sabernhardt - I don't think we need to change anything here.

The reason twentytwenty needed the change is its comparison used is_front_page() || is_home(). In twentynineteen and previous themes used is_front_page() && is_home(). With the logic this way, duplicate h1s do not occur as far as i can tell.

Closing this as worksforme. @williampatton if I have missed something or you feel there is still an issue that needs fixing here, please feel free to reopen this ticket.

#17 @williampatton
5 years ago

Closing suits me fine, so long as it works is the main thing. Thanks for looking at it and validating things worked well in TwentyNinteen :)

Note: See TracTickets for help on using tickets.