-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Properly associate the spacer height input label. #6922
Conversation
970e9d7
to
afc7cfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
Tested it and the label works as expected.
Thanks for the PR, but a bit of a comment on the PR description: it makes sense and it's handy to understand what we should be doing with labels re: accessibility, but there are two issues with the PR description here:
- the lesson makes it hard to parse what the PR is actually doing and what we should be checking
- the description doesn't actually prevent anyone new to the project, going forward, with making the same mistake
Looks like what should actually happen is that we should require an ID or make one if one isn't supplied and there's a single child. I've filed #6989 as a follow-up for this, but this is good to merge IMO.
core-blocks/spacer/index.js
Outdated
minHeight="20" | ||
handleClasses={ { | ||
top: 'block-spacer__resize-handler-top', | ||
bottom: 'block-spacer__resize-handler-bottom', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these should be core-blocks-spacer__*
instead
Changed the classNames with
Thanks for your suggestion @tofumatt. Next time, I will certainly avoid the "lesson". After all, since the start of this project, it's just the 19th time (I'm maintaining a list and counted the occurrences) I'm opening an issue or pushing a PR to fix such a simple thing like HTML labels. Since I've explained what the issue is so many times, I'd agree it's absolutely pointless to try to explain it again the next time a missing or improperly associated label is merged in the codebase. Also, worth noting there was an issue proposing to introduce some form of automated test for the labels, see #6098. It was then closed as "not essential for 5.0" so I'm expecting all developers involved in this project will know what to do with labels without having to rely on the help of some automated test. |
To be clear: I don't think it's pointless nor unimportant. But the component in question has a confusing API: that was the point of me filing #6989–a label is created using the component and it appears. Creating an unlinked label here is an easy mistake to make, and I think it's not on developers not caring but the component having an API that is both unclear (no error when no ID is provided) and that creates the wrong expectation (the I don't think any developers are setting out to create inaccessible code, but it looks like in spots we've made it easy. The best way to fix that. going forward, would be to address code quality issues like #6989. |
This seems to have broken the resize handlers (blue dots). |
Proposed fix at #7024 |
Description
Properly associates the existing
<label>
element to the spacer height input field.I'd like to remind, once again, that any UI control must be properly labeled. In order fo preference:
<label>
element associated with for/id attributes<label>
A visible, properly associated label is preferable because:
Currently, the
<label>
element is just a stray label. Not associated with anything. This way, it's just text in the page and doesn't have any purpose. Additionally, the input field has anaria-label
attribute that basically repeats the visible label and doesn't add any relevant information:Height in pixels
Height for the spacer element in pixels.
This PR:
edit
removing thewp-
prefixWorth noting the
aria-hidden
attribute rendered in the front-end is pointless because an empty<div>
is not exposed in any way to assistive technologies. Ideally, it should be removed but it doesn't harm anything.Fixes #6918