get_custom_logo does not apply the aria-current attribute in all cases

[bschneidewind]

When reading settings have a posts page set but not a page set for the homepage - get_custom_logo does not apply the aria-current attribute to the logo link on the homepage.

When reading settings have a posts page set but not a page set for the homepage - get_custom_logo does not apply the aria-current attribute to the logo link on the homepage.

Trac ticket: https://core.trac.wordpress.org/ticket/62879

[audrasjb]

Hello, thanks for the ticket and the patch!
Self assigning for review.

### Test report

Settings:

#### Before applying the PR: no aria-current attribute.

#### After applying the PR: aria-current attribute displayed.

### Test results

This PR fixes the issue 👍

[siliconforks]

Replying to bschneidewind:

When reading settings have a posts page set but not a page set for the homepage

Some questions:

1. Is that actually considered a valid, supported configuration (to have a posts page set but not a page set for the homepage)? That doesn't really seem like it would be useful combination of settings to me. I see that the WordPress admin section does in fact allow you to configure things that way, which is a bit odd and might be a bug?

2. Assuming that this is a configuration that should be supported by WordPress, doesn't that imply that is_front_page() and WP_Query::is_front_page() are broken in that configuration? It appears that they will never return true in that configuration, which doesn't really make sense (obviously the site still has a front page).

3. The current proposed changes to support this configuration involve adding some rather complex conditional logic. It is currently proposed that this logic be copied into multiple different places in the code?
   • ​https://github.com/WordPress/wordpress-develop/pull/8206 - this fixes get_custom_logo()
   • ​https://github.com/WordPress/wordpress-develop/pull/8237 - this fixes the twentytwenty theme (#62895)
4. Copying this complex code into multiple places doesn't seem very ​DRY. Is this going to need to get copied into other places too? What about all the other themes besides twentytwenty? Fixing is_front_page() could perhaps eliminate the need for this.

[bschneidewind]

@siliconforks - my thoughts on things:

1 - The UX already supports this in the admin area, I don't think it's the most logical approach overall but how could you force users to add a 'homepage' that may have already set it up this way? The render_block_core_home_link function already has support for this edge case as well.

2 - Agreed, it would be better if is_front_page always rang true when it's the root page but there are edge cases that need to be accounted for and there is some debate on whether is_front_page should remain true. Ex: if it's a paginated page... from an accessibility point it shouldn't, but the argument may differ in other aspects.

3 - This PR fixes get_custom_logo(). The TwentyTwenty update is to address the specific template function where the text version is missing the aria-current attribute. Yes, I am proposing this in multiple places to fix older themes and in core to address block based themes.

4 - I am proposing a similar approach be added in some of the older themes here where site logo support does not currently exist: #61766

Additional theme updates will be needed for bundled themes prior to TwentyTwentyTwo because of how things were built on each theme. Even if the is_front_page() was updated to account for the edge case, a theme update would be needed on those bundled themes that do not have support for aria-current on the site title links.

With TwentyTwentyTwo and up using block based approach, the aria-current attribute is applied correctly there on the text links for the site-title.

It does create some repeated code between TwentyTwentyOne and earlier bundled themes and the update to get_custom_logo, however I think it makes sense in these cases as the theme updates are only needed because of a different approach to building in TwentyTwentyOne and prior.

[bschneidewind]

Related, ​https://github.com/WordPress/wordpress-develop/pull/8237 - has been updated to account for all bundled themes (#62895)

[siliconforks]

Replying to bschneidewind:

Ex: if it's a paginated page... from an accessibility point it shouldn't, but the argument may differ in other aspects.

Yes, the pagination is another edge case, but the behavior of is_front_page() has always been to ignore pagination, so I'm not suggesting changing that here. I'm just proposing that is_front_page() handle the case where WordPress is configured without a page set for the homepage (assuming it is decided that this is actually a supported configuration).

Additional theme updates will be needed for bundled themes prior to TwentyTwentyTwo because of how things were built on each theme. Even if the is_front_page() was updated to account for the edge case, a theme update would be needed on those bundled themes that do not have support for aria-current on the site title links.

My point is that if is_front_page() is fixed, the theme update will be much simpler and require less duplicated code.

[bschneidewind]

@siliconforks - Good points - it's been discussed before it looks like #59252

I did throw a patch up to see if it gets anywhere and will adjust any other PR's if it makes progress.

[sabernhardt]

Is that actually considered a valid, supported configuration (to have a posts page set but not a page set for the homepage)? That doesn't really seem like it would be useful combination of settings to me.

The edge case might be fine to allow, but I do not think the special posts page should have aria-current (or current-menu-item classes) on home links.

When I assigned the page of posts without a static home page, the two URLs gave me duplicate content. With the Site Title block, I had the same link attributes at both URLs, too:

<p class="wp-block-site-title"><a href="http://localhost/wp671" target="_self" rel="home" aria-current="page">WP 6.7.1</a></p>

The home links' conditions for current-page ARIA and class names might need to check the URL (or $wp->request) instead of is_front_page() and/or is_home(). The logo link already adds aria-current incorrectly on the network signup and activation pages. Comments on #43536 note that wp-signup.php and wp-activate.php return true for both is_front_page() and is_home(), and is_home() potentially can return true as a fallback in other edge cases.

History for aria-current on home links:

• [48749] get_custom_logo()
• GB35880 Site Title block
• GB51478 Home Link block

[sabernhardt]

I do not think the special posts page should have aria-current (or current-menu-item classes) on home links.

My first sentence was not highly appropriate on this ticket. The conditions in PR 8206 would add aria-current on the home page without adding the attribute on the extra page. However, the conditions might need changes for other cases.

[siliconforks]

Replying to sabernhardt:

Is that actually considered a valid, supported configuration (to have a posts page set but not a page set for the homepage)? That doesn't really seem like it would be useful combination of settings to me.

The edge case might be fine to allow, but I do not think the special posts page should have aria-current (or current-menu-item classes) on home links.

When I assigned the page of posts without a static home page, the two URLs gave me duplicate content. With the Site Title block, I had the same link attributes at both URLs, too:

<p class="wp-block-site-title"><a href="http://localhost/wp671" target="_self" rel="home" aria-current="page">WP 6.7.1</a></p>

Does this have anything to do with the edge case configuration, though? What happens if you do assign a static home page (in addition to a page of posts)? Won't you have the same issue on the posts page? (i.e., the link says aria-current="page" when it shouldn't)

This looks like it's a separate issue, with the Site Title block.

[sabernhardt]

Sorry, I have managed to make this quite confusing. Parts of comment:9 relate better to the Site Title and Home Link blocks.

Regarding get_custom_logo():

1. The front-page conditions used in [48749] already created false positives for home logo links (and the unlink option) on the two network pages.
2. Introducing an is_home() condition theoretically can add aria-current in ​unknown edge cases, though parse_query() corrects some situations in the lines following $this->is_home = true (so it might not be a major concern).

What happens if you do assign a static home page (in addition to a page of posts)?

The static home page would match is_front_page(), and aria-current is appropriate there (before or after PR 8206).

[audrasjb]

Hello, my opinion here is that the proposed changeset is not so complex (a one-line change) and that it's worth adding this fix even for such a edge case.

[faisal03]

The PR effectively fixes the issue, but considering that this is an edge case, it might be worth evaluating the performance implications before merging.

[shailu25]

Test Report

Patch Tested: ​https://github.com/WordPress/wordpress-develop/pull/8206

Environment:
WordPress - 6.8-alpha-20250202.040014
OS - Windows
Browser - Firefox
Theme: Twenty Twenty
PHP - 7.4.31
Active Plugin: None

Actual Results:

• Issue Resolved With Patch.✅

Screenshots:

• Added Attachment

[shailu25]

Before Patch

[shailu25]

After Patch

[audrasjb]

Marking this for commit.

Thanks for pushing the test cases @peterwilsoncc !

[audrasjb]

In 60062:

General: Improve aria-current management in get_custom_logo().

This changeset fixes a edge case in get_custom_logo() where a page was set for the homepage without any front page in Settings > Reading and the aria-current attribute wasn't present on the logo link.

Props bschneidewind, audrasjb, siliconforks, sabernhardt, faisal03, shailu25, peterwilsoncc.
Fixes #62879.