Admin UI: Fix type mismatch between Page title and NavigableRegion ariaLabel#75899
Conversation
…iaLabel Page accepts title as React.ReactNode but passes it directly to NavigableRegion's ariaLabel, which expects a string and renders it as an aria-label DOM attribute. Passing JSX as title would result in [object Object] in the DOM, breaking accessibility. Add an explicit ariaLabel prop to Page for callers that use rich content in title. When ariaLabel is not provided, fall back to title only if it's a string. Make NavigableRegion's ariaLabel optional so it gracefully handles undefined values.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Can you also update the changelog? |
e3e354b to
dde0800
Compare
|
Is making the ariaLabel optional in NavigableRegion the right move? Isn't the main idea of that component is to be an accessible aria region and without a label, it's a bug basically? |
That's a good point. I suppose there should always be a label. I'm not sure what's the best way to go about it ; I didn't want to make it mandatory because I wasn't sure it was a good idea, since there are already components that rely on |
|
Are there components that rely on this component that don't provide the label? I feel like if it's the case, these components should be updated to provide one no? |
|
I would agree, and we can do that in the Gutenberg codebase (I've just done so), and we can then make the new prop mandatory, but how can we account for third-parties using the package and not knowing about those changes? Is it okay to introduce a new mandatory prop like this? |
Add explicit ariaLabel to all Page consumers, removing the implicit title-based fallback. This ensures every navigable region has a deliberate accessibility label.
I actually thought the prop existed in the component before and was mistyped, but it seems the prop didn't exist but instead "aria-label" prop was being used. what do you think is best? keep using aria-label or introduce a new ariaLabel. Both options seem possible as long as we keep the previous aria-label passing working (which seems to be the case now) |
packages/admin-ui/src/page/index.tsx
Outdated
|
|
||
| return ( | ||
| <NavigableRegion className={ classes } ariaLabel={ title }> | ||
| <NavigableRegion className={ classes } ariaLabel={ ariaLabel }> |
There was a problem hiding this comment.
Where I have trouble with this PR is that I don't think we should force "Page" users to provide two titles (a title and an ariaLabel). Why can't we use the title as ariaLabel? or maybe the ariaLabel should be optional when we want these two things to be different.
There was a problem hiding this comment.
+1, as long as title is a string, it should be enough:
<Page title={ <PageNameWithLogo /> } ariaLabel="Page name" />
and
<Page title="Page name" />
There was a problem hiding this comment.
Why can't we use the title as ariaLabel? or maybe the ariaLabel should be optional when we want these two things to be different.
I pushed a commit to do just that, let me know what you think.
| <Page title={ __( 'Route not found' ) } hasPadding> | ||
| <Page | ||
| title={ __( 'Route not found' ) } | ||
| ariaLabel={ __( 'Route not found' ) } |
There was a problem hiding this comment.
This comment is very visible here, why as a consumer I have to prove my title twice.
When title is a string, use it as the aria-label automatically. This avoids forcing callers to duplicate the same value in both title and ariaLabel. An explicit ariaLabel is only needed when title is JSX or a different label is desired.
…iaLabel (WordPress#75899) Page accepts title as React.ReactNode but passes it directly to NavigableRegion's ariaLabel, which expects a string and renders it as an aria-label DOM attribute. Passing JSX as title would result in [object Object] in the DOM, breaking accessibility. Add an explicit ariaLabel prop to Page for callers that use rich content in title. When ariaLabel is not provided, fall back to title only if it's a string. Make NavigableRegion's ariaLabel optional so it gracefully handles undefined values. Co-authored-by: jeherve <jeherve@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: simison <simison@git.wordpress.org>
What?
Closes #75898
Add an explicit
ariaLabelprop to thePagecomponent and makeNavigableRegion'sariaLabeloptional, fixing a type mismatch between the two components.Why?
PageacceptstitleasReact.ReactNodebut passes it directly toNavigableRegionasariaLabel, which expects astringand renders it as anaria-labelDOM attribute. If a caller passes JSX astitle(e.g., a title with an icon), thearia-labelrenders as[object Object], breaking accessibility for screen readers.How?
ariaLabel?: stringprop toPage.NavigableRegion'sariaLabel:ariaLabelprop if provided.titleif it's a plain string.undefined, which omits thearia-labelattribute from the DOM.NavigableRegion'sariaLabelprop optional (string | undefined). React naturally omits the attribute when the value isundefined.All existing callers pass string titles and don't use the new
ariaLabelprop, so behavior is unchanged.Testing Instructions
Pagecomponent (e.g., Templates, Patterns, Styles). Inspect the wrapping region element — it should still have a validaria-labelmatching the page title.Pagecomponent with a JSX title and an explicitariaLabel:aria-labelshould be"My Page", not[object Object].Pagewith a JSX title and noariaLabel. The region element should have noaria-labelattribute rather than a broken one.Testing Instructions for Keyboard
No UI changes — this PR fixes a prop type and adds an optional prop. Keyboard interaction is unaffected. Accessibility is improved: screen readers will no longer encounter
[object Object]as a region label.Screenshots or screencast
N/A — this is a type/API fix with no visual changes.