WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 112 minutes ago

#37011 reopened enhancement

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
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 (12)

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

Download all attachments as: .zip

Change History (54)

@Soean
4 years ago

First patch

#1 @Presskopp
3 years ago

  • Keywords has-patch added

#2 @sabernhardt
4 months ago

  • Focuses accessibility added
  • Keywords needs-refresh added

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


4 months ago

#4 @joedolson
4 months ago

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

#5 @joedolson
4 months ago

  • Milestone changed from Awaiting Review to 5.5

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

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

#7 @audrasjb
4 months ago

  • Keywords needs-refresh removed

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

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

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

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


4 months ago

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

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


8 weeks ago

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

  • Description modified (diff)

#18 @SergeyBiryukov
7 weeks 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
7 weeks 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
7 weeks ago

#20 @SergeyBiryukov
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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
6 weeks 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 6 weeks ago by sabernhardt (previous) (diff)

#25 @afercia
5 weeks ago

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

#26 @afercia
5 weeks 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
5 weeks ago

  • Keywords 2nd-opinion removed

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


8 days ago

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


7 days ago

#31 @SergeyBiryukov
6 days 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
6 days 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
6 days 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
6 days 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
6 days 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
6 days ago

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

#35 @audrasjb
6 days ago

  • Keywords 2nd-opinion removed

Thanks for spotting this @sabernhardt !

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

#36 @sabernhardt
6 days 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.


5 days ago

@sabernhardt
4 days ago

aria-current option for linked logo

#38 @sabernhardt
4 days 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
24 hours 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
23 hours 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 hours 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
112 minutes ago

  • Milestone changed from 5.5.1 to 5.5

Moving per comment:40.

Note: See TracTickets for help on using tickets.