WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 14 months ago

#37011 closed enhancement (fixed)

Don’t link custom logo if it’s displayed on the front page

Reported by: FlorianBrinkmann Owned by: joedolson
Milestone: 5.5 Priority: highest omg bbq
Severity: blocker Version: 4.6
Component: Themes Keywords: has-patch has-dev-note commit dev-reviewed
Focuses: accessibility Cc:

Description (last modified by SergeyBiryukov)

Hey!

I have looked at the get_custom_logo() function recently and noticed that the logo is linked to home even if it is displayed on the front page.

Wouldn’t it make sense to only link the logo if it is not displayed on the front page?

Rian Rietveld writes:

On the homepage the logo or site title can be put into an H1. This is not a link, because that will be a link to the page itself.

And I think it makes perfect sense :)

Attachments (15)

37011.patch (1.1 KB) - added by Soean 5 years ago.
First patch
37011.1.diff (1.2 KB) - added by audrasjb 18 months ago.
If on home/front page, don’t link the logo to home
37011.2.diff (1.6 KB) - added by sabernhardt 18 months ago.
maintaining a container element, revising conditional logic, updating function information
37011.3.diff (2.8 KB) - added by SergeyBiryukov 15 months ago.
37011.3.alt.diff (2.3 KB) - added by SergeyBiryukov 15 months ago.
twenty twenty.png (127.3 KB) - added by afercia 15 months ago.
twemty twemty headings.png (127.8 KB) - added by afercia 15 months ago.
twenty nineteen.png (155.9 KB) - added by afercia 15 months ago.
37011.4.diff (1.2 KB) - added by SergeyBiryukov 15 months ago.
37011.diff (1.8 KB) - added by audrasjb 14 months ago.
Themes: Introduce unlink-homepage-logo parameter for custom-logo theme support option and use it to determine whether to link or unlink the logo on the homepage
37011.5.diff (2.1 KB) - added by sabernhardt 14 months ago.
checks for theme support to determine default alt text as well as linking
37011.6.diff (2.4 KB) - added by sabernhardt 14 months ago.
aria-current option for linked logo
37011.7.diff (2.1 KB) - added by sabernhardt 14 months ago.
checks is_paged() to make sure link is not removed from pages of older posts
37011.8.diff (2.2 KB) - added by whyisjake 14 months ago.
37011.9.diff (1.2 KB) - added by whyisjake 14 months ago.

Download all attachments as: .zip

Change History (74)

@Soean
5 years ago

First patch

#1 @Presskopp
4 years ago

  • Keywords has-patch added

#2 @sabernhardt
18 months ago

  • Focuses accessibility added
  • Keywords needs-refresh added

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


18 months ago

#4 @joedolson
18 months ago

  • Component changed from General to Themes
  • Owner set to joedolson
  • Status changed from new to accepted

#5 @joedolson
18 months ago

  • Milestone changed from Awaiting Review to 5.5

#6 @afercia
18 months ago

This ticket was discussed during today's accessibility bug-scrub: it was somehow missed because set to the "General" component. Updating the ticket properties for better visibility and setting milestone to 5.5.

@audrasjb
18 months ago

If on home/front page, don’t link the logo to home

#7 @audrasjb
18 months ago

  • Keywords needs-refresh removed

37011.1.diff refreshes old patch and check both is_home() and is_front_page().

#8 @SergeyBiryukov
18 months ago

Thanks for the patch! Just noting that checking both is_home() and is_front_page() seems redundant here. If a page is set as a front page and another one as a posts page, then the link should only be removed on the former, not on the latter.

is_front_page() should be enough.

@sabernhardt
18 months ago

maintaining a container element, revising conditional logic, updating function information

#9 @sabernhardt
18 months ago

  • Keywords needs-testing needs-dev-note added

Related: #31027 began work toward removing the home page logo link in the Twenty Fifteen theme.

For the latest patch:

  1. The image needs a "custom-logo-link" container element so it can match the theme's styles of the linked logo on other pages. This patch keeps the same styling in at least Twenty Seventeen and Twenty Nineteen; logos in Twenty Fifteen and Twenty Twenty appear to be the same with or without the container element. Some of the themes that target the `a` element should still need updating for this change.
  2. The conditional logic is revised to keep the link on the blog page when using a static front page.
  3. The function description and since note are updated, and I'm sure they could be improved further.

It would be good to test in multisite, as well as more themes.

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


17 months ago

#11 @samful
16 months ago

Tested the Core themes (Twenty Nineteen, Twenty Seventeen & Twenty Twenty) with 37011.2.diff on standard WordPress and Multisite WordPress and no issues here.

Setting the home page as a static page or leaving it as "Your latest Posts" seems to all work, the is_front_page() function seems to be doing it's job well.

On the Twenty Nineteen theme, the logo still has an on hover effect around it (a black circle) which may confuse some users, but I think this isn't a big issue as the cursor does not change, and so clearly isn't a hyperlink.

I agree that the some of the 'a' element themes may need changing after this patch, however after testing the number 1 on that list, the "Mesmerize" theme seems to still work as expected.

The since notes seems clear enough to me @sabernhardt , I think you've done a good job here.

Last edited 16 months ago by samful (previous) (diff)

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


16 months ago

#13 follow-up: @afercia
15 months ago

Should the logo alt attribute be empty on the front page?

I'd tend to think that when it's not within the link the logo is purely "decorative" and doesn't need its purpose to be described.

If so, I guess the alt should be explicitly set to empty string e.g.:

$custom_logo_attr['alt'] = '';

and the comment block above should be updated.

#14 in reply to: ↑ 13 ; follow-up: @knutsp
15 months ago

Replying to afercia:

Should the logo alt attribute be empty on the front page?

I'd tend to think that when it's not within the link the logo is purely "decorative" and doesn't need its purpose to be described.

A logo is about visual identity, and hat is in my interpretation almost the opposite of. As I read https://www.w3.org/WAI/tutorials/images/decorative/ a logo is not such. It's not "Identified and described by surrounding text". The logo is usually there instead of the site title, usually a part of the H1 tag itself.

The alt text must be the site title.

#15 @SergeyBiryukov
15 months ago

Simplified some logic and adjusted documentation a bit.

37011.3.diff includes setting the alt attribute to an empty string for the home page, per comment:13.

37011.3.alt.diff doesn't include that change, since apparently there's no consensus yet.

#16 @audrasjb
15 months ago

  • Keywords 2nd-opinion added; needs-testing removed

Thanks for the refresh @SergeyBiryukov.
I'm personally inclined to ship 37011.3.alt.diff for now.

#17 @SergeyBiryukov
15 months ago

  • Description modified (diff)

#18 @SergeyBiryukov
15 months ago

In 48039:

Accessibility: Themes: Don't link to the home page in get_custom_logo() when it's displayed on the home page.

Props Soean, audrasjb, sabernhardt, FlorianBrinkmann, rianrietveld, afercia, joedolson, samful, knutsp, SergeyBiryukov.
See #37011.

#19 in reply to: ↑ 14 @afercia
15 months ago

Replying to knutsp:

A logo is about visual identity, and hat is in my interpretation almost the opposite of. As I read https://www.w3.org/WAI/tutorials/images/decorative/ a logo is not such. It's not "Identified and described by surrounding text". The logo is usually there instead of the site title, usually a part of the H1 tag itself.

The alt text must be the site title.

Considering the logo "in isolation", as if there was no other content, yes maybe this would be correct. However, in practice, things are a bit different.

Almost all the bundled themes, and I guess the vast majority of the themes, output the logo together with or close to the site branding text, which is often the main H1.

For example, Twenty Twenty outputs the logo inside the main H1 text (visually hidden with screen-reader-text). As a result, the site title is announced twice. This isn't ideal.

Similarly, Twenty Nineteen outputs the logo before the main H1: the site title will be read out twice when screen reader users navigate the page content.

In the cases above, the logo alt text needs to be empty.

For clarity, see screenshots below.

Overall, this is one more case where the same image should use a different alt text depending on the context. At the moment, WordPress doesn't have the ability to use contextual alt text. See ongoing discussion on #47456.

From a themes perspective, I'm wondering if the_custom_logo() and get_custom_logo() should be changed to allow to pass a text for the alt attribute.

#20 @SergeyBiryukov
15 months ago

Refreshed the empty alt approach from the previous patch, just in case (setting the alt attribute to an empty string for the home page, per comment:13).

#21 @knutsp
15 months ago

Many themes offer to show either logo or title, some both. I'm more worried that the title will not be printed (seen/announced) than twice.

From a themes perspective, I'm wondering if the_custom_logo() and get_custom_logo() should be changed to allow to pass a text for the alt attribute.

Yes, that will make it possible to handle this correctly, by either logo alt text or announcing a visually hidden title, depending on the theme author's preference in this case.

#22 follow-up: @sabernhardt
15 months ago

If you edit the alt text for the cropped image within the media library, that custom text would replace the site name in the alt attribute. But leaving it blank currently would not replace default text with an empty alt.

It's also worth noting that #36640 (in the 5.5 milestone) proposes a filter on the get_custom_logo image attributes. So that could override either no alt or the default alt text.

#23 in reply to: ↑ 22 @SergeyBiryukov
15 months ago

Replying to sabernhardt:

It's also worth noting that #36640 (in the 5.5 milestone) proposes a filter on the get_custom_logo image attributes. So that could override either no alt or the default alt text.

Thanks for pointing that out, the filter is now added in [48057].

#24 @sabernhardt
15 months ago

I started an early draft for the dev note, in which I would like to include information about the filter. One of the potential examples there addresses the Twenty Twenty alt text.

Both parts may fit well within a larger dev note, perhaps if there is a collection of miscellaneous theme-related notes.

Last edited 15 months ago by sabernhardt (previous) (diff)

#25 @afercia
15 months ago

@sabernhardt @SergeyBiryukov I see this ticket still has the 2nd-opinion keyword. Anything else here to be further discussed or clarified?

#26 @afercia
15 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 48283:

Accessibility: Themes: Use a default empty alt attribute for the non-linked custom logo on the home page.

After [48039] it became clear that the non-linked custom logo on the home page needs an empty alt attribute, as in most of the cases the logo is decorative and doesn't need its purpose to be described.

This change outputs an empty alt attribute by default for the custom logo on the home page. If necessary, it is possible to use the new 'get_custom_logo_image_attributes' filter to manipulate the default attributes for the logo image and set an alt attribute.

Props FlorianBrinkmann, Soean, sabernhardt, audrasjb, SergeyBiryukov, samful, knutsp.
See #36640.
Fixes #37011.

#27 @afercia
15 months ago

  • Keywords 2nd-opinion removed

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


14 months ago

This ticket was mentioned in Slack in #core by sabernhardt. View the logs.


14 months ago

#31 @SergeyBiryukov
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Per @johnstonphilip's comment in comment:16:ticket:36640, reopening to see if the concerns raised here are something to address for 5.5 RC2: https://make.wordpress.org/core/2020/07/28/themes-changes-related-to-get_custom_logo-in-wordpress-5-5/#comment-39108

@audrasjb
14 months ago

Themes: Introduce unlink-homepage-logo parameter for custom-logo theme support option and use it to determine whether to link or unlink the logo on the homepage

#32 @audrasjb
14 months ago

  • Keywords 2nd-opinion added
  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker

37011.diff fixes the backward compatibility issue pointed out in the dev note.

It introduces unlink-homepage-logo parameter for custom-logo theme support option and use it to determine whether to link or unlink the logo on the homepage.

By default, unlink-homepage-logo is set to false.

Theme authors can opt-in for the change using for example this snippet:

	add_theme_support(
		'custom-logo',
		array(
			'height'               => $logo_height,
			'width'                => $logo_width,
			'flex-height'          => true,
			'flex-width'           => true,
			'unlink-homepage-logo' => true, // HERE !
		)
	);

(this can be used to replace the existing bits of code in Twenty Twenty)

For 5.5, I'd suggest to commit it as it and to implement the unlink-homepage-logo parameter to bundle themes after 5.5 is released, as bundled themes can be released independantly from Core.

@SergeyBiryukov do you have any thought on this approach, as WP 5.5 Tech Lead?

#33 @johnstonphilip
14 months ago

Why should the logo not be a link to the homepage if you're on the homepage? There are a few good reasons why it should be a link even if you're on that page:

  1. The user might not know if they are actually on your homepage, and thus will want to click the logo to be sure, as it's [become convention](https://ux.stackexchange.com/questions/81727/why-is-it-standard-for-a-website-logo-to-navigate-to-the-home-page) that it links to the homepage.
  1. If you want to copy the link to a website, right clicking on the logo and copying the link is (arguably) easier than selecting the URL in the address bar, especially on certain mobile browsers and browsers like Safari, which hide the full URL in the address bar by default.
  1. As I mentioned, this is a well-known convention that has existed for a long time, and changing it in this way without any provided UX research as justification seems a bit unusual. Having it not be a link on your homepage only breaks the consistency of the element and user-expectation.

#34 @sabernhardt
14 months ago

Yes, opt-in would be better than enforcing the change.

@audrasjb Thanks for the patch! It should also check for the theme support earlier, though. Otherwise the logo could be linked with empty alt text.

@sabernhardt
14 months ago

checks for theme support to determine default alt text as well as linking

#35 @audrasjb
14 months ago

  • Keywords 2nd-opinion removed

Thanks for spotting this @sabernhardt !

I think the current implementation is good to go for RC2.

#36 @sabernhardt
14 months ago

It's very likely too late to add this in time for 5.5, but if and when the logo is linked on the homepage, the link could have the aria-current attribute.

		} else {
			$aria_current = is_front_page() ? ' aria-current="page"' : '';
			$html = sprintf(
				'<a href="%1$s" class="custom-logo-link" rel="home"%2$s>%3$s</a>',
				esc_url( home_url( '/' ) ),
				$aria_current,
				$image
			);
		}

I could add a separate ticket for later discussion, if desired.

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


14 months ago

@sabernhardt
14 months ago

aria-current option for linked logo

#38 @sabernhardt
14 months ago

Since @ryokuhi expressed some interest in the aria-current attribute for 5.5, I uploaded a patch including that. If that addition should wait, the previous patch would be fine.

#39 @whyisjake
14 months ago

  • Milestone changed from 5.5 to 5.5.1

Going to move this to the 5.5.1 milestone so that it can be addressed in the next release.

#40 @sabernhardt
14 months ago

@whyisjake The aria-current part could wait until later (I'll make a separate ticket for that today), but the backward compatibility fix in 37011.5.diff would belong earlier.

#41 @demetris
14 months ago

I think we need one more check in order to decide whether to remove the link.

Right now we only check for is_front_page. I think we should also check if, in addition to that, we are on the first page using is_paged.

That is:

if (is_front_page() && !is_paged()) {
   /* Unlink the logo. */
}

Am I right or am I missing something?

Cheers!

#42 @SergeyBiryukov
14 months ago

  • Milestone changed from 5.5.1 to 5.5

Moving per comment:40.

@sabernhardt
14 months ago

checks is_paged() to make sure link is not removed from pages of older posts

#43 @sabernhardt
14 months ago

Thanks @demetris! The conditional does need to check for pagination.

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


14 months ago

#45 @SergeyBiryukov
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48749:

Accessibility: Themes: Only unlink custom logo on the home page if the theme declares support for that.

To accommodate for the change, the custom-logo theme feature now accepts the unlink-homepage-logo parameter.

If and when the logo is linked on the home page, the link has the aria-current attribute for better accessibility.

Follow-up to [48039], [48283].

Props sabernhardt, audrasjb, johnstonphilip, demetris.
Fixes #37011.

#46 @SergeyBiryukov
14 months ago

  • Keywords commit dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#47 @whyisjake
14 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#48 @whyisjake
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48755:

Accessibility: Themes: Only unlink custom logo on the home page if the theme declares support for that.

To accommodate for the change, the custom-logo theme feature now accepts the unlink-homepage-logo parameter.

If and when the logo is linked on the home page, the link has the aria-current attribute for better accessibility.

Follow-up to [48039], [48283], see [48749].

This brings the changes to the 5.5 branch.

Props sabernhardt, audrasjb, johnstonphilip, demetris, SergeyBiryukov.
Fixes #37011.

#49 @TimothyBlynJacobs
14 months ago

It'd be good to add unlink-homepage-logo to the create_initial_theme_features registration for the feature.

#50 @SergeyBiryukov
14 months ago

  • Keywords commit dev-reviewed removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for address comment:49.

#51 @SergeyBiryukov
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48757:

Themes: Add unlink-homepage-logo to the create_initial_theme_features() registration for the custom-logo theme feature.

Follow-up to [48039], [48283], [48749].

Props TimothyBlynJacobs.
Fixes #37011.

#52 @SergeyBiryukov
14 months ago

  • Keywords commit dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#53 @whyisjake
14 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#54 @whyisjake
14 months ago

Awaiting for test changes before committing to 5.5.

@whyisjake
14 months ago

#55 @whyisjake
14 months ago

  • Keywords dev-feedback added; commit dev-reviewed removed

@whyisjake
14 months ago

#56 @whyisjake
14 months ago

In 48758:

Themes: Update the test_theme_supports_custom_logo to check for the unlink-homepage-logo property.

See #37011, [48757].
Props TimothyBlynJacobs, whyisjake.

#57 @whyisjake
14 months ago

  • Keywords commit added

#58 @TimothyBlynJacobs
14 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to me!

#59 @whyisjake
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48759:

Themes: Add unlink-homepage-logo to the create_initial_theme_features() registration for the custom-logo theme feature.
Follow-up to [48039], [48283], [48749], [48757], [48758].

This brings the changes to the 5.5 branch.

Props TimothyBlynJacobs, SergeyBiryukov, whyisjake.
Fixes #37011.

Note: See TracTickets for help on using tickets.