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 | 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 )
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?
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)
Change History (74)
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
#4
@
5 years ago
- Component changed from General to Themes
- Owner set to joedolson
- Status changed from new to accepted
#6
@
5 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.
#7
@
5 years ago
- Keywords needs-refresh removed
37011.1.diff refreshes old patch and check both is_home()
and is_front_page()
.
#8
@
5 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.
@
5 years ago
maintaining a container element, revising conditional logic, updating function information
#9
@
5 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:
- 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.
- The conditional logic is revised to keep the link on the blog page when using a static front page.
- 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
@
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.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
4 years ago
#13
follow-up:
↓ 14
@
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:
↓ 19
@
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
@
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
@
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.
#19
in reply to:
↑ 14
@
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.
#20
@
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
@
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:
↓ 23
@
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
@
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
@
4 years 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.
#25
@
4 years ago
@sabernhardt @SergeyBiryukov I see this ticket still has the 2nd-opinion
keyword. Anything else here to be further discussed or clarified?
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
@
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
@
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
@
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
@
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:
- 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.
- 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.
- 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
@
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.
#35
@
4 years ago
- Keywords 2nd-opinion removed
Thanks for spotting this @sabernhardt !
I think the current implementation is good to go for RC2.
#36
@
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
#38
@
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
@
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
@
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
@
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!
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
4 years ago
#46
@
4 years ago
- Keywords commit dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for the 5.5 branch.
#49
@
4 years ago
It'd be good to add unlink-homepage-logo
to the create_initial_theme_features
registration for the feature.
#50
@
4 years ago
- Keywords commit dev-reviewed removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for address comment:49.
First patch