Make WordPress Core

Opened 7 years ago

Last modified 5 weeks ago

#44656 accepted defect (bug)

Multiple themes: Empty site title leaves empty anchor tag in header

Reported by: tsquez's profile tsquez Owned by: joedolson's profile joedolson
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-test-info changes-requested
Focuses: accessibility Cc:

Description

I just noticed something weird the other day with Twenty Twelve. I removed the title all together (no I did not hide it) and when I looked at the source code this is what I saw:

<h1 class="site-title">
	<a href="http://localhost/dev/" rel="home"></a>
</h1>

It's an H1 tag with an empty anchor? This shouldn't happen I don't think.

So I download a few other themes and did the same thing and sure enough all that was left was an empty H1 tag with an empty anchor tag.

I do not know why anyone would want to do this BUT If the site title is removed completely, meaning not hidden and not entered not entered either in "Settings" or "Site Identity" in the customizer, then the site H1 site title tag should not be displayed.

I am not sure if this is a bug so I thought I would enter it in. I looked all over and could not find any information or any other tickets regarding this.

I apologize if its a waste of time.

Thanks in advance.

Attachments (3)

44656.diff (1.2 KB) - added by audrasjb 7 years ago.
Adds site-title check before displaying it
44656.1.diff (1.1 KB) - added by sabernhardt 4 years ago.
checks for site title and tagline in Twenty Twelve header
#44656.patch (1.2 KB) - added by rehanali 3 years ago.
Patch Added

Download all attachments as: .zip

Change History (24)

#1 @westonruter
7 years ago

  • Component changed from Customize to Bundled Theme

@audrasjb
7 years ago

Adds site-title check before displaying it

#2 @audrasjb
7 years ago

  • Keywords has-patch added

Hi and welcome to WordPress Trac, @tsquez !

Thanks for the ticket.

Even if it's a small edge case, I'd say we can easily solve it, so let's fix the issue :)

44656.diff adds a condition to check if get_bloginfo( 'name' ) is empty or not.

Cheers,

Jb

#3 @audrasjb
7 years ago

Oops, I didn't noticed you were talking about Twenty Twelve.

My patch is for Twenty Seventeen.

Of course, I can fix the issue on all bundled themes but I'm not sure we are still supporting old bundled themes for such a edge case… (cc @davidakennedy)

#4 @desrosj
6 years ago

  • Keywords needs-patch added; has-patch removed

@laurelfulford Do you have any thoughts as to whether this is something that should be fixed? It seems reasonable to move to Future Release, in my opinion.

#5 @laurelfulford
6 years ago

  • Milestone changed from Awaiting Review to Future Release

@desrosj Agreed! :)

Thanks for the ping -- I've been moving through these tickets backwards, but it's slow going!

#6 @nielslange
6 years ago

  • Keywords needs-testing added; needs-patch removed

#7 @ianbelanger
5 years ago

  • Summary changed from Empty site title leaves empty anchor tag in header to Twenty Twelve: Empty site title leaves empty anchor tag in header
  • Version 4.9.7 deleted

@sabernhardt
4 years ago

checks for site title and tagline in Twenty Twelve header

#8 @sabernhardt
4 years ago

  • Keywords has-patch added

44656.1.diff has the same conditional @audrasjb used for the site title, and it also checks the tagline/description.

@rehanali
3 years ago

Patch Added

#9 @sabernhardt
2 years ago

  • Keywords needs-patch added; needs-testing has-patch removed
  • Summary changed from Twenty Twelve: Empty site title leaves empty anchor tag in header to Multiple themes: Empty site title leaves empty anchor tag in header

I do not like allowing empty links or headings; however, (as noted above) most of the classic bundled themes have given superfluous markup when sites have not included a site title and/or tagline. If we fix it for Twenty Twelve, we probably should edit other themes, too.

Twenty Ten creates an empty link inside a heading if the site has no title. An empty tagline results in an empty div (which usually is not a problem).

<h1 id="site-title">
	<span>
		<a href="https://example.com/" rel="home"></a>
	</span>
</h1>
<div id="site-description"></div>

Twenty Eleven gives an empty link and/or heading.

<hgroup>
	<h1 id="site-title"><span><a href="https://example.com/" rel="home"></a></span></h1>
	<h2 id="site-description"></h2>
</hgroup>

Twenty Twelve likewise gives an empty link and/or heading.

<hgroup>
	<h1 class="site-title"><a href="https://example.com/" rel="home"></a></h1>
	<h2 class="site-description"></h2>
</hgroup>

Twenty Thirteen adds empty headings inside of a link.

<a class="home-link" href="https://example.com/" rel="home">
	<h1 class="site-title"></h1>
	<h2 class="site-description"></h2>
</a>

Twenty Fourteen, Twenty Fifteen, Twenty Sixteen and Twenty Seventeen all add a link inside a heading on the expectation that the site has a title. These four themes will, however, skip adding markup without a tagline.

<h1 class="site-title"><a href="https://example.com/" rel="home"></a></h1>

Twenty Twenty has slightly different markup, but it similarly leaves an empty link inside a heading when the site lacks a site title (and the theme does not include anything for an empty tagline).

<div class="header-titles">
	<h1 class="site-title"><a href="https://example.com/"></a></h1>
</div>

Twenty Nineteen and Twenty Twenty-One do not have any markup for either an empty site title or an empty tagline.

<div class="site-branding">
</div>

The block themes do not have any output for empty site titles either.

#10 @sabernhardt
12 months ago

#61768 was marked as a duplicate.

#11 @sabernhardt
12 months ago

  • Focuses accessibility added

#12 @joedolson
4 months ago

  • Owner set to joedolson
  • Status changed from new to accepted

#13 @joedolson
4 months ago

  • Milestone changed from Future Release to 6.9

This ticket was mentioned in PR #8582 on WordPress/wordpress-develop by @sukhendu2002.


4 months ago
#14

  • Keywords has-patch added; needs-patch removed

#15 @dilipbheda
2 months ago

  • Keywords has-test-info added

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8582

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 136.0.0.0
  • OS: Linux
  • Theme: Twenty Eleven 4.9
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.
  2. ✅ This patch prevents the display of an empty site title in the Twenty Eleven theme.

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


7 weeks ago

#17 @joedolson
7 weeks ago

  • Keywords needs-testing added

@dilipbheda Your test report indicates that it was tested on Twenty Eleven, but this patch applies to Twenty Ten, Twenty Eleven, Twenty Twelve, Twenty Thirteen, Twenty Fourteen, Twenty Fifteen, Twenty Sixteen, Twenty Seventeen, and Twenty Twenty.

Can you confirm this for all relevant themes?

#18 @SirLouen
7 weeks ago

  • Keywords changes-requested added; needs-testing removed

Test Report

Description

🟠 This report validates that the indicated patch works as expected with one caveat (check Additional Notes)

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8582.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 136.0.0.0
  • OS: Windows 10/11
  • Theme: *
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

* Tested Themes:

  1. Twenty Ten 4.4
  2. Twenty Eleven 4.9
  3. Twenty Twelve 4.5
  4. Twenty Thirteen 4.4
  5. Twenty Fourteen 4.2
  6. Twenty Fifteen 4.0
  7. Twenty Sixteen 3.5
  8. Twenty Seventeen 3.9
  9. Twenty Twenty 2.9

Reproduction Steps

  1. In Settings > General > Remove the Site Title and the Tag Line
  2. Go to the home page
  3. Check HTML code and see if you can find an <h1> with the title and in some themes an <h2> or <div> with the Tag line
  4. 🐞 Found h1 in all 9 themes, <h2> not in all themes only 2011, 2012 and 2013 and <div> only in 2010

Actual Results

  1. 🟠 Issue resolved with patch in all 9 themes, but code could be a little improved

Additional Notes

  • @sukhendu2002, I've requested some changes in PR 8582 to improve the code.
Last edited 7 weeks ago by SirLouen (previous) (diff)

#19 @joedolson
6 weeks ago

@sukhendu2002 Requested some additional changes. Thanks!

@sabernhardt commented on PR #8582:


6 weeks ago
#20

The display filter in bloginfo() ran the name and description through wptexturize and convert_chars in addition to esc_html.

In the event that filtering might empty the string, adding display in the variable could help:

$site_name        = get_bloginfo( 'name', 'display' );
$site_description = get_bloginfo( 'description', 'display' );

PHPCS might complain about <?php echo $site_name; ?>, but the PR passes the coding standards GitHub action. I would prefer not to add the `phpcs:ignore` comments.

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


5 weeks ago

Note: See TracTickets for help on using tickets.