Fix : Duplicative link control label and placeholder #66329
Conversation
|
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. |
getdave
added
[Type] Regression
There was a problem hiding this comment.
I appreciate the follow up here.
My main concern is that the new placeholder seems to just repeat the same text as the label. That seems superflous.
I would suggest we can remove the placeholder (by default).
MDN suggests the following definition of placeholder:
The placeholder text should provide a brief hint to the user as to the expected type of data that should be entered into the control.
Therefore using the word Search isn't terrible useful to the user as it's not an example of the type of input.
I thought about potential examples we could use. The two types of input are either:
- A search string for an entity (e.g. a Post or Page) such as
Sample Page. - A search for a URL such as
www.example.com.
However the problem is that because it's in the @wordpress/block-editor package, this control can be used outside of WordPress. This rules out any WordPress-specific examples such as www.wordpress.org or Sample Page.
As a result Im now leaning towards defaulting placeholder to null and requiring that the specific implementation defines the placeholder. So when being used in a WP context the placeholder could be set.
Thanks again for your work here 👍
There was a problem hiding this comment.
@mirka Should the <InputBase/> implement logic to force the label to match the placeholder if hideLabelFromVision is true?
That seems like it would prevent a lot of bugs and force some built-in a11y.
| return ( | ||
| <div className="block-editor-link-control__search-input-container"> | ||
| <URLInput | ||
| disableSuggestions={ currentLink?.url === value } | ||
| label={ inputLabel } | ||
| label={ label ?? __( 'Search or type URL' ) } | ||
| hideLabelFromVision={ hideLabelFromVision } | ||
| className={ className } | ||
| value={ value } | ||
| onChange={ onInputChange } | ||
| placeholder={ inputLabel } | ||
| placeholder={ placeholder } |
There was a problem hiding this comment.
From the Why section of the PR that introduced the duplicative change, the rationale is that if there is no visible label then the placeholder text must match the label. It doesn't look like <InputBase /> handles this for us, so we could do it here:
- Set a const for the inputLabel above this block:
const inputLabel = label ?? __( 'Search or type URL' ) - Apply the label:
label={ inputLabel } - Check for hideLabelFromVision before applying the placeholder:
placeholder={ hideLabelFromVision ? inputLabel : placeholder }
There was a problem hiding this comment.
👍 We don't want to end up in a situation where we have the label and placeholder showing the same thing. That's the principle bug we have to address in this PR. Visual reference:
There was a problem hiding this comment.
Yup - hopefully the sample code I added will:
- Not have the placeholder and label match unless the consumer passes strings that match.
- Force the placeholder to match the label only if hideLabelFromVision is true and we would want the placeholder and label to match.
For a low-level component, this seems a bit too heavy-handed for me since Though, I think we can start by including this accessibility guideline in the best practices section of the component docs that we're working on improving right now (for example in #66000). |
What?
LinkControlSearchInputcomponentWhy?
How?
labelprop toLinkControlSearchInputcomponentlabelprop get its value fromsearchInputLabelprop set forLinkControlcomponent