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

URL Input outputs with no ID if renderControl undefined #39999

Closed
iansvo opened this issue Apr 3, 2022 · 3 comments · Fixed by #40310
Closed

URL Input outputs with no ID if renderControl undefined #39999

iansvo opened this issue Apr 3, 2022 · 3 comments · Fixed by #40310
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Technical Feedback Needs testing from a developer perspective. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@iansvo
Copy link
Contributor

iansvo commented Apr 3, 2022

Description

URLInput does not always output an ID on the input itself, so in certain situations the label doesn't maintain it's relationship to the input.

See below snippet from: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/url-input/index.js (Line 438)

		const controlProps = {
			id: `url-input-control-${ instanceId }`,
			label,
			className: classnames( 'block-editor-url-input', className, {
				'is-full-width': isFullWidth,
			} ),
		};

		const inputProps = {
			value,
			required: true,
			className: 'block-editor-url-input__input',
			type: 'text',
			onChange: this.onChange,
			onFocus: this.onFocus,
			placeholder,
			onKeyDown: this.onKeyDown,
			role: 'combobox',
			'aria-label': __( 'URL' ),
			'aria-expanded': showSuggestions,
			'aria-autocomplete': 'list',
			'aria-owns': suggestionsListboxId,
			'aria-activedescendant':
				selectedSuggestion !== null
					? `${ suggestionOptionIdPrefix }-${ selectedSuggestion }`
					: undefined,
			ref: this.inputRef,
		};

		if ( renderControl ) {
			return renderControl( controlProps, inputProps, loading );
		}

		return (
			<BaseControl { ...controlProps }>
				<input { ...inputProps } />
				{ loading && <Spinner /> }
			</BaseControl>
		);

The input's ID string is only set on controlProps and if the experimental method isn't found it will return the input without an ID at all, as the BaseControl doesn't appear to pass the ID prop down to the input (even as part of the child render).

Proposed Solution
The ID value should be explicitly set on the input if it's not being output via renderControl. Every input should have an ID or at least an accessible label (which this control currently doesn't).

Step-by-step reproduction instructions

  1. View any block that has a URLInput in the block editor (or create a new one)
  2. Inspect the input and label, note the lack of matching ID reference

Screenshots, screen recording, code snippet

Screenshot: https://d.pr/i/Y25JB1

Environment info

  • Confirmed on WordPress 5.9

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ndiego ndiego added the Needs Technical Feedback Needs testing from a developer perspective. label Apr 3, 2022
@talldan talldan added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components [a11y] Labelling labels Apr 4, 2022
@talldan
Copy link
Contributor

talldan commented Apr 4, 2022

@iansvo Thanks for reporting this. If this is a fix you'd be interested in working on, contributions are very welcome. It seems like you've already done some good work identifying the problem.

@iansvo
Copy link
Contributor Author

iansvo commented Apr 12, 2022

I'm going to be working on a PR for this and hopefully will have something in this week!

@iansvo
Copy link
Contributor Author

iansvo commented Apr 13, 2022

@talldan @fabiankaegy PR has been added for this issue: #40310

I'm an OOS noob so please let me know if I missed any steps. I made sure the build/tests all passed.

@fabiankaegy fabiankaegy linked a pull request Apr 13, 2022 that will close this issue
@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Labelling labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Technical Feedback Needs testing from a developer perspective. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants