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.

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

Adding support for defined IDs in TextControl component #52028

Conversation

andrewhayward
Copy link
Contributor

What?

This PR adds support to the TextControl component to accept an optional id prop. It currently ignores them, and generates its own.

Why?

As per #24749, it is sometimes useful to provide components with a defined ID (for example, if you need to reference the control with an aria-* attribute elsewhere). Ideally all components would work this way (but that is outside the scope of this patch).

How?

Currently, TextControl uses useInstanceId to generate an ID. As with other components (see SelectControl, for example), this is now only used if an id prop is not passed in. This retains backwards compatibility, and for most use-cases will have minimal impact; IDs will continue to be generated and applied.

Testing Instructions

Unit tests have been added to verify the expected behaviour:

  • Confirms that IDs are generated and applied, is previously
  • Confirms that ID is used if provided
  • Confirms that the <label> receives the ID and has a correct for attribute

This can also be confirmed manually:

  • Create an instance of a TextControl, providing both id and label props
  • Check that the id and for attributes match on the generated <input> and <label> respectively
  • Additionally, clicking on the <label> should put focus on the <input>

Testing Instructions for Keyboard

Although this does not impact first-line keyboard access, similar manual testing can be done using a screen reader:

  • Create an instance of a TextControl, providing both id and label props
  • Navigate to the generated <label> and action it (⌃ Control + ⌥ Option + Space with VoiceOver, for example); focus should move to the <input>
  • Navigate to the generated <input> and confirm that it has a label matching the label prop

This patch adds support for custom IDs in the `TextControl` component.

Currently any passed ID prop is ignored, and a auto-generated value is
used instead.

---
Resolves WordPress#24749
@andrewhayward andrewhayward requested a review from ajitbohra as a code owner June 28, 2023 10:19
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 28, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @andrewhayward! 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.

@ciampo ciampo requested review from mirka and ciampo June 28, 2023 11:11
@ciampo ciampo requested a review from chad1008 June 28, 2023 14:25
Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

Pending a changelog entry, this LGTM 🚀 🚢

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Congrats on your first PR, and thank you for adding tests! 💛

Comment on lines 42 to 44
const textbox = screen.getByRole( 'textbox' );
const label = screen.getByText( labelValue );
expect( textbox ).toHaveAttribute( 'id', label.getAttribute( 'for' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Not that this is inherently wrong, but I find it more idiomatic/robust within the testing-library context to simply assert that the textbox is accessibly labeled, rather than assert the implementation details.

Suggested change
const textbox = screen.getByRole( 'textbox' );
const label = screen.getByText( labelValue );
expect( textbox ).toHaveAttribute( 'id', label.getAttribute( 'for' ) );
expect( screen.getByRole( 'textbox', { name: labelValue } ) ).toBeVisible();

☝️ Something like that. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have much experience with testing-library, and I didn't know you could do this! That makes much more sense.

Comment on lines 19 to 20
function useUniqueId( idProp?: string ) {
const instanceId = useInstanceId( TextControl );
Copy link
Member

Choose a reason for hiding this comment

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

FYI we can pass the id prefix and the idProp directly into the useInstanceId() function to avoid this custom code ✨

/**
* Provides a unique instance ID.
*
* @param object Object reference to create an id for.
* @param [prefix] Prefix for the unique id.
* @param [preferredId] Default ID to use.
* @return The unique instance id.
*/
function useInstanceId(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this, and strongly considered using it. But the useUniqueId pattern seemed to be in quite a few places already, which made me second guess myself.

I'm quite happy to apply useInstanceId "properly" here though, if that's actually the more appropriate direction.

Copy link
Member

Choose a reason for hiding this comment

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

I'm quite happy to apply useInstanceId "properly" here though, if that's actually the more appropriate direction.

Yes, I think it is. I have a feeling that the existing useUniqueId patterns were written before useInstanceId was expanded to take three arguments.

@mirka mirka linked an issue Jul 3, 2023 that may be closed by this pull request
Andrew Hayward added 4 commits July 11, 2023 16:49
Changing the test setup for labels to be more `testing-library` idiomatic.
Adding line item under Enhancements to include `TextControl` changes.
Dropping the old `useUniqueId` pattern in favour of the more succinct and complete `useInstanceId` usage.
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Great work, thank you! 🎉

@mirka mirka merged commit 357afa8 into WordPress:trunk Jul 12, 2023
@github-actions
Copy link

Congratulations on your first merged pull request, @andrewhayward! 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!

@github-actions github-actions bot added this to the Gutenberg 16.3 milestone Jul 12, 2023
westonruter added a commit that referenced this pull request Jul 12, 2023
…dd/defer-script-loading-strategy

* 'trunk' of https://github.com/WordPress/gutenberg:
  Update Changelog for 16.2.0
  Adding support for defined IDs in `TextControl` component (#52028)
  Bump plugin version to 16.2.0
  Revert "Bump plugin version to 16.2.0"
  Bump plugin version to 16.2.0
  Add maxLength to LinkControl search items (#52523)
  [RNMobile] Update Editor block inserter button styles and default text input placeholder/selection styles (#52269)
  Site Editor: Reset device preview type when exiting the editing mode (#52566)
  Trim footnote anchors from excerpts (#52518)
  Add back old Navigation and File blocks JavaScript implementation when Gutenberg is not installed (#52553)
  Block Editor: Ensure synced patterns are accounted for in 'getAllowedBlocks' (#52546)
  Fix md5 class messed up with new block key (#52557)
  Fix entity cache misses for single posts due to string as recordKey (#52338)
  Make "My patterns" permanently visible (#52531)
  Hide site hub when resizing frame upwards to avoid overlap (#52180)
  Fix "Manage all patterns" link appearance (#52532)
  Update navigation menu title size & weight in detail panels (#52477)
  Site Editor Patterns: Ensure sidebar does not shrink when long pattern titles are used (#52547)
  Site Editor: Restore quick inserter 'Browse all' button (#52529)
  Patterns: update the title of Pattern block in the block inspector card  (#52010)
@mburridge mburridge added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jul 18, 2023
@properlypurple properlypurple mentioned this pull request Oct 11, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextControl: label not connecting to input if props contains an id
4 participants