URLInput now always has an ID and accessible label

[iansvo]

What?

Related Issue: #39999

This PR ensures the URLInput block control always has a correct ID attribute and accessible label. Previously there were scenarios in which this input would have no ID even with a specified label or where the input would receive 2 labels (one aria-label from the component, the other the label prop value).

Why?

Currently, this input is not properly accessible. If a developer is using this component and provides a label prop, the label outputs but it's for attribute points to an ID that doesn't exist.

renderControl is an experimental method and I imagine the condition fails to call it because of this, which is what creates the bug we're observing here.

How?

• Label initialized to null for simpler falsey comparisons
• inputID is now applied to the inputProps if renderControl is falsey
• Duplicate aria-label no longer outputs if a label prop is specified.

Testing Instructions

Default

1. Create a new test page
2. Add a Button block
3. Inspect the HTML and note the presence of ID attribute and aria-label

Custom

1. Create a custom block that implements URLInput in the editing experience
2. Specify a label prop
3. Use the block in the editor and note the label for the attribute matches the input's ID

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @iansvo! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[alexstine]

Hello @iansvo , thank you for your pull request!

I left one small note below but this is looking good to me.

Please let me know what you are thinking. 👍

[alexstine]

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

[iansvo]

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.

[alexstine]

@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.

[Mamaduka]

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? 🙇

[iansvo]

@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!

[Mamaduka]

Thanks for the update, @iansvo.

The changes look good to me 👍

[github-actions]

Congratulations on your first merged pull request, @iansvo! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

[Mamaduka]

Thanks for contributing, @iansvo, and congrats on your merged PR 🎉

[iansvo]

Thanks again @Mamaduka! I've updated my profile to include my GitHub user as well.