Make WordPress Core

Opened 8 years ago

Closed 4 years ago

#37011 closed enhancement (fixed)

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

Reported by: florianbrinkmann's profile FlorianBrinkmann Owned by: joedolson's profile 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 8 years ago.
First patch
37011.1.diff (1.2 KB) - added by audrasjb 4 years ago.
If on home/front page, don’t link the logo to home
37011.2.diff (1.6 KB) - added by sabernhardt 4 years ago.
maintaining a container element, revising conditional logic, updating function information
37011.3.diff (2.8 KB) - added by SergeyBiryukov 4 years ago.
37011.3.alt.diff (2.3 KB) - added by SergeyBiryukov 4 years ago.
twenty twenty.png (127.3 KB) - added by afercia 4 years ago.
twemty twemty headings.png (127.8 KB) - added by afercia 4 years ago.
twenty nineteen.png (155.9 KB) - added by afercia 4 years ago.
37011.4.diff (1.2 KB) - added by SergeyBiryukov 4 years ago.
37011.diff (1.8 KB) - added by audrasjb 4 years 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 4 years ago.
checks for theme support to determine default alt text as well as linking
37011.6.diff (2.4 KB) - added by sabernhardt 4 years ago.
aria-current option for linked logo
37011.7.diff (2.1 KB) - added by sabernhardt 4 years 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 4 years ago.
37011.9.diff (1.2 KB) - added by whyisjake 4 years ago.

Download all attachments as: .zip

Change History (74)

@Soean
8 years ago

First patch

#1 @Presskopp
7 years ago

  • Keywords has-patch added

#2 @sabernhardt
4 years ago

  • Focuses accessibility added
  • Keywords needs-refresh added

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


4 years ago

#4 @joedolson
4 years ago

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

#5 @joedolson
4 years ago

  • Milestone changed from Awaiting Review to 5.5

#6 @afercia
4 years 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
4 years ago

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

#7 @audrasjb
4 years ago

  • Keywords needs-refresh removed

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

#8 @SergeyBiryukov
4 years 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
4 years ago

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

#9 @sabernhardt
4 years 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.


4 years ago

#11 @samful
4 years 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 4 years ago by samful (previous) (diff)

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


4 years ago

#13 follow-up: @afercia
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

  • Description modified (diff)

#18 @SergeyBiryukov
4 years 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
4 years 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.

@afercia
4 years ago

#20 @SergeyBiryukov
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

I started an early draft for the dev note, in which I would like to include information about the filter.

Version 0, edited 4 years ago by sabernhardt (next)

#25 @afercia
4 years ago

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

#26 @afercia
4 years 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
4 years ago

  • Keywords 2nd-opinion removed

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


4 years ago

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


4 years ago

#31 @SergeyBiryukov
4 years 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
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

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

#35 @audrasjb
4 years ago

  • Keywords 2nd-opinion removed

Thanks for spotting this @sabernhardt !

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

#36 @sabernhardt
4 years 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.


4 years ago

@sabernhardt
4 years ago

aria-current option for linked logo

#38 @sabernhardt
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

  • Milestone changed from 5.5.1 to 5.5

Moving per comment:40.

@sabernhardt
4 years ago

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

#43 @sabernhardt
4 years ago

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

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


4 years ago

#45 @SergeyBiryukov
4 years 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
4 years ago

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

Reopening for the 5.5 branch.

#47 @whyisjake
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#48 @whyisjake
4 years 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
4 years ago

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

#50 @SergeyBiryukov
4 years ago

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

Reopening for address comment:49.

#51 @SergeyBiryukov
4 years 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
4 years ago

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

Reopening for the 5.5 branch.

#53 @whyisjake
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#54 @whyisjake
4 years ago

Awaiting for test changes before committing to 5.5.

@whyisjake
4 years ago

#55 @whyisjake
4 years ago

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

@whyisjake
4 years ago

#56 @whyisjake
4 years 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
4 years ago

  • Keywords commit added

#58 @TimothyBlynJacobs
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to me!

#59 @whyisjake
4 years 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.