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

Remove wrapping label element to improve accessibility #6461

Merged
merged 7 commits into from
Jun 27, 2022

Conversation

FlorianBoe
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets
Related issues/PRs #6444
License MIT
Documentation PR

What's in this PR?

Some input elements in the admin interface have two associated label elements. Besides the visible label (with a for="/xyz"), input elements are also nested within a label element.

two-labels

Why?

This enclosing label is problematic because icons and buttons within the wrapper are added to input name. This problem is particularly problematic for assistive technologies such as screen readers.

To Do

  • Update snapshots
  • Update the type of the labelRef

I am not quite sure what is a good solution to adjust the typing accordingly. Suggestions are very welcome!

@@ -121,7 +121,7 @@ export default class Input<T: ?string | ?number> extends React.PureComponent<Inp

return (
<Fragment>
<label
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

as far as i see, the labelRef prop of this component is only used by the ColorPicker and SingleAutoComplete to position their popover. so it would be okay in the core to change this as the other components dont need a <label> element.

but if we change this, we should rename labelClass and labelRef. maybe to something like inputContainerClass?

Copy link
Contributor

Choose a reason for hiding this comment

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

regarding to the type of the ref - i think i would just use ElementRef<*> then 🙂

Copy link
Member

Choose a reason for hiding this comment

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

@niklasnatter I'm not completely sure about your comment. Can we remove the labelRef? Or what is it used for?

Copy link
Contributor

@niklasnatter niklasnatter Apr 5, 2022

Choose a reason for hiding this comment

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

the labelRef is used by the SingleAutoComplete to display the popover at the correct position. so we should rename it to something like inputContainerRef and use that in the SingleAutoComplete 🙂

@alexander-schranz alexander-schranz added the UX Affecting the end user label Feb 23, 2022
@alexander-schranz
Copy link
Member

Reminder for myself we need to check the checkbox and the checkbox toggler type work still like expected.

@alexander-schranz alexander-schranz force-pushed the bugfix/duplicate-input-label branch 3 times, most recently from 4c9ad50 to f98ff64 Compare April 5, 2022 11:38
@niklasnatter
Copy link
Contributor

@alexander-schranz Is this ready for review? 🙂

@alexander-schranz
Copy link
Member

@niklasnatter look like something is still failing in the tests. But if you want to have a look at it or take it over its fine for me :)

Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up!

@niklasnatter niklasnatter changed the title Remove wrapping label element Remove wrapping label element to improve accessibility Jun 27, 2022
@niklasnatter niklasnatter merged commit cb51e4d into sulu:2.5 Jun 27, 2022
@FlorianBoe FlorianBoe deleted the bugfix/duplicate-input-label branch July 28, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UX Affecting the end user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants