Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URLInput now always has an ID and accessible label #40310

Merged
merged 3 commits into from
Apr 22, 2022
Merged

URLInput now always has an ID and accessible label #40310

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a57c937
URLInput now always has an ID and accessible label
iansvo Apr 13, 2022
002bcc5
PR suggestion - syntax update
iansvo Apr 14, 2022
748b3f1
Update props to fully support custom render method
iansvo Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/block-editor/src/components/url-input/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ class URLInput extends Component {

renderControl() {
const {
label,
label = null,
className,
isFullWidth,
instanceId,
Expand All @@ -435,8 +435,10 @@ class URLInput extends Component {
suggestionOptionIdPrefix,
} = this.state;

const inputId = `url-input-control-${ instanceId }`;

const controlProps = {
id: `url-input-control-${ instanceId }`,
id: inputId,
label,
className: classnames( 'block-editor-url-input', className, {
'is-full-width': isFullWidth,
Expand All @@ -453,7 +455,7 @@ class URLInput extends Component {
placeholder,
onKeyDown: this.onKeyDown,
role: 'combobox',
'aria-label': __( 'URL' ),
iansvo marked this conversation as resolved.
Show resolved Hide resolved
'aria-label': label ? undefined : __( 'URL' ), // Ensure input always has an accessible label
'aria-expanded': showSuggestions,
'aria-autocomplete': 'list',
'aria-owns': suggestionsListboxId,
Expand All @@ -468,6 +470,9 @@ class URLInput extends Component {
return renderControl( controlProps, inputProps, loading );
}

// If renderControl is falsey, set the input's ID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How necessary is this? Looks like it uses the same variable. I am not understanding what is being set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputProps does not include the id prop, it's only in controlProps. I am not exactly sure how renderControl handles these, but I would assume it's handled differently than here.

However, if that condition fails and the renderControl call isn't returned, the input does not receive an ID. controlProps doesn't actually output the id prop value anywhere on BaseControl.

You can test this by commenting it out and seeing the lack of ID on the input. The intent here was to fix the failure case and be otherwise minimally invasive. I'm not exactly sure what conditions cause renderControl to be falsey or not.

Let me know if you have any other questions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@talldan I like what I see here but I am still hesitant to approve this as I am not sure about this id thing. Do you know anything about this component? If not, can you request review from someone who might? Overall, I thought we didn't use class components so I am guessing this is one of the first created.

Copy link
Member

@Mamaduka Mamaduka Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @iansvo. Thanks for contributing.

I fear that conditionally providing input ID can cause a mismatch when the renderControl override is available. Take the example below; the label here will receive ID but not the input field.

The controlProps and inputProps allow us to set proper attributes without worrying if ID is there or not.

<URLInput
    renderControl={ ( controlProps, inputProps ) => (
        <BaseControl { ...controlProps }>
            <input { ...inputProps } />
        </BaseControl>
    ) }
/>

P.S. Can you rebase this branch on top of the current trunk? 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka I think in my original solution I was just trying to be minimally invasive so I didn't really look into that prop very much. I can see from this PR that this was added as a way to conditional override that rendering (also noted for the future!), which your code here shows.

Now that I understand the intent of this the goal here should just be to always pass the id prop to inputProps and we should have coverage for all cases, including the example case you provided.

The inputId variable is now always present in both controlProps and inputProps. Now, a custom render or a default one will always have everything needed to properly set an accessible label.

This should be fully compatible with previous implementations since the controlProps are unmodified and inputProps only has something new, which is the same value, etc.

I thought about just setting the value in control props and passing it to inputProps by reference, but I think this approach will keep the intent and purpose of the code clear.

Please review the latest commit and let me know if you have any other thoughts/feedback. Thanks again for bringing this up and helping the solution be more holistic!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, @iansvo.

The changes look good to me 👍

inputProps.id = inputId;

return (
<BaseControl { ...controlProps }>
<input { ...inputProps } />
Expand Down