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

Issue #6001 - remove label in the shortcode block #6033

Closed
wants to merge 4 commits into from
Closed

Issue #6001 - remove label in the shortcode block #6033

wants to merge 4 commits into from

Conversation

bobbingwide
Copy link
Contributor

Description

Removes the Label from inside the shortcode block.

  • The icon was wrong anyway ( code-editor rather than shortcode)
  • The text label of Shortcode was unnecessary
  • It used up space
  • It was inconsistent
  • Not required now that there's a new visual indicator for block names.

How Has This Been Tested?

Manually.

Screenshots (jpeg or gifs if applicable):

image

Types of changes

I deleted some code.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@danielbachhuber danielbachhuber requested a review from a team April 10, 2018 12:32
@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Apr 10, 2018
@danielbachhuber danielbachhuber self-requested a review April 10, 2018 12:36
@@ -66,10 +66,6 @@ export const settings = {

return (
<div className="wp-block-shortcode">
<label htmlFor={ inputId }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there's some CSS we can remove as well.

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

👍 Seems reasonable to me but I've flagged for a second review.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Code looks good, just waiting for a design check @karmatosed @jasmussen to approve.

@afercia
Copy link
Contributor

afercia commented Apr 10, 2018

If I got it correctly, this PR removes the <label> element, if so I'd like to remind once again that any UI control needs to be labeled, especially form fields.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 10, 2018
@danielbachhuber
Copy link
Member

@afercia Can you suggest how the PR should be updated to include the label in the correct manner?

@afercia
Copy link
Contributor

afercia commented Apr 10, 2018

@danielbachhuber I need a few minutes to finish something and switch VVV installation. In the meantime, Id suggest everyone to not think just visually. Labels are meant to give UI controls and form fields an accessible name. This name is used by assistive technologies and it's required.
You wouldn't every build form without properly associated <label> elements, right?

  • screen readaers use labels to announce what the field is about
  • speech recognition software use the label to empowe user to voice command click xyz to focus,f or example, a form field or click a button
  • visible text label ebnefit all users

@youknowriad
Copy link
Contributor

youknowriad commented Apr 10, 2018

Good catch @afercia I believe this could be replaced with an aria-label on the PlainText component. Probably less optimal than a visible label but A good alternative if we want to keep it visually this way.

@jasmussen
Copy link
Contributor

Noting that I polished the shortcode block, including the label, in #5605

I will rebase off whatever happens in this PR, but figured I'd share the work in progress.

@afercia
Copy link
Contributor

afercia commented Apr 10, 2018

@youknowriad unfortunately an aria-label wouldn't help so much speech recognition software users. They need a visible accessible name to be able to voice a proper command. For example, they need to know a form field accessible name (given by the label) is "address" to be able to voice a command "click address" and focus the field.

On other controls, e.g. the icon buttons, we're exposing the accessible name given by aria-label with the tooltips. That requires to make the tooltips appear to know the name, it's not 100% ideal but at least it's something.

On a form field we can't use a tooltip, I guess 🙂 There should always be a visible label that matches the accessible name. In this specific case, I'd keep the previous label. Maybe I'd consider to move it above the field, so the field can be full width.

In other cases, for example the post title, we're using a visually hidden <label> only because it mathces the visible text given by the "placeholder" (not a real placeholder indeed). That way, users can see "Add title" and the accessible name given by the label is "Add title",a nd it works.

The most simple thing in this case is to keep the label. Removing it is definitely an accessibility regression compared to the previous implementation.

@bobbingwide
Copy link
Contributor Author

The outer div for the shortcode block already has an aria-label in the same style of the outer div for a paragraph block. At present there's not much difference between these blocks other than what the user types into them. Ditto for a Heading block.

Does that mean that the textarea field should have the same aria stuff as these blocks.

Strip away the CSS padding around the text field and visually the blocks are very similar.
I know nothing at all about a11y other than what I've heard from Graham Armitage.

image

Perhaps I'll learn something at #wcldn on Contributor day. If I don't then my Countdown and CSV blocks, both shown above, will fail the test.

@afercia
Copy link
Contributor

afercia commented Apr 11, 2018

The outer div is going to change in #5709 ad needs an aria-abel because it's focusable, and it will have a proper ARIA role.
Non-editable blocks like the shortcode one are different from editable blocks, as they're made of form fields. Form fields need to be labeled. Basic HTML even before being a11y. See also my previous comment #6033 (comment)

@danielbachhuber
Copy link
Member

Closing per #6033 (comment)

The most simple thing in this case is to keep the label. Removing it is definitely an accessibility regression compared to the previous implementation.

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 Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants