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

Adds aria-label to the search button, as accessibility enhancement #38136

Merged

Conversation

Sisanu
Copy link
Contributor

Description

Adds aria-label to the search button, as accessibility enhancement.

How has this been tested?

Tested with Lighthouse and the accessibility score went up.

Screenshots

Types of changes

Enhancement

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@github-actions
Copy link

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

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 21, 2022
@aristath aristath added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jan 24, 2022
@aristath
Copy link
Member

I can see that before this PR, the <button> contained just the <svg> without a label.
Using aria-label fixes the issue, however, I'm not certain if this would be the recommended method. 🤔
I think we have 2 choices here: Either use the aria-label attribute, OR add in the contents of the button element something like <span class="screen-reader-text">%s</span>.

Adding the a11y tag since this will need to be reviewed 👍

@Sisanu
Copy link
Contributor Author

Thank you @aristath for the feedback! Anything is better than no accessibility for the button.

@carolinan
Copy link
Contributor

carolinan commented Jan 24, 2022

I do not know if using a screen reader text or aria-label would be best.
Since this reuses the $attributes['label'] it might also make sense to use aria-labelledby? The label does not have a unique id.

@Sisanu
Copy link
Contributor Author

Since this reuses the $attributes['label'] it might also make sense to use aria-labelledby? The label does not have a unique id.

Maybe that's why aria-label is the way to go for now, without changing too much the output of the block.

@alexstine
Copy link
Contributor

I think you're only other option is VisuallyHidden from wordpress/components. Not sure this would actually be better than just a simple aria-label in this case.

@alexstine
Copy link
Contributor

https://accessible360.com/accessible360-blog/use-aria-label-screen-reader-text/

Looks like screen reader text is a better option and I've always opted for using screen reader text anyway. For this, it really is trivial as both should have the same effect.

@Sisanu I'll let you make the call on if you want to do the extra work with VisuallyHidden.

@Sisanu
Copy link
Contributor Author

Thank you @alexstine! I would keep it as simple as possible, and use just the area-label for now.

@ryelle ryelle added the [Block] Search Affects the Search Block - used to display a search field label Feb 11, 2022
@ryelle ryelle self-requested a review February 11, 2022 21:52
@ryelle ryelle force-pushed the enhancement/search-block/accessibility-attr branch from 72cc2bd to 0ef7f75 Compare February 15, 2022 21:53
@ryelle
Copy link
Contributor

I was giving this one last test when I realized the aria-label was applied to every button, regardless of icon or text. The buttons that have text don't need the label, as IIRC aria-label will override the custom text someone may have set. I made a change in 0ef7f75 to only apply the aria-label when it's an icon button (and also rebased the PR).

Copy link
Contributor

@alexstine alexstine left a comment

Choose a reason for hiding this comment

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

A thought to add.

@@ -81,17 +82,19 @@ function render_block_core_search( $attributes ) {
$button_internal_markup = wp_kses_post( $attributes['buttonText'] );
}
} else {
$aria_label = sprintf( 'aria-label="%s"', esc_attr( wp_strip_all_tags( $attributes['label'] ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

@ryelle I am not sure using aria-label in this context should be done. Honestly, I think this whole file should be refactored to include $attributes['button_aria_label'] that way users wouldn't be tempted to go out of their way to break accessibility. My problem with your approach is this.

$attributes['buttonText'] = '<span class="icon-class"></span>';

Now, the button is back to being inaccessible. Technically, it's not an icon button, but it still allows HTML right? Hints the need to strip tags for aria-label?

As I said, I would rather see an approach where accessibility related attributes are given and then we won't have to try to guess if users could make it inaccessible. Of course users can make it inaccessible, but the difference would be passing $attributes['buttonText'] vs. $attributes['button_aria_label'].

Thoughts? Does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, create a new attribute to pass in a button label? That does make sense, but I think it would be a good discussion to have in another issue. Overall I think getting this solution in place would improve things now, and it wouldn't prevent iterating on the label by introducing a new attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm good with that. I'll make a follow-up PR once this is in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, though I'd suggest making an issue first - it'll need design input for the aria label setting UI.

@ryelle ryelle merged commit 16f6a5b into WordPress:trunk Feb 16, 2022
@github-actions
Copy link

Congratulations on your first merged pull request, @Sisanu! 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 12.7 milestone Feb 16, 2022
ramonjd pushed a commit that referenced this pull request Feb 18, 2022
…38136)

* Adds aria-label to the search button, as accessibility enhancement

* Strip all tags for the aria label attribute

* Add aria label only when the button is an icon

Co-authored-by: Kelly Dwan <ryelle@users.noreply.github.com>
@carolinan carolinan added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Feb 18, 2022
@carolinan
Copy link
Contributor

Please note that there is an open trac ticket that can be closed once this is added to core https://core.trac.wordpress.org/ticket/55196

@talldan
Copy link
Contributor

@Sisanu Thanks for tackling this! I also made an issue about the lack of label when I noticed it recently - #38647.

Looking at your PR, it looks like the problem has now been solved for the front-end output of the block.

In the editor there's some separate code for the search block's icon only button that's also missing a label:

{ buttonUseIcon && (
<button
type="button"
className={ buttonClasses }
style={ buttonStyles }
>
<Icon icon={ search } />
</button>
) }

Do you think a label should be added for this too? If so, is that something you'd be willing to work on?

@Sisanu
Copy link
Contributor Author

Hi @talldan! Sure, I can take a look. Please assign me the issue.

@alexstine
Copy link
Contributor

@Sisanu Looks like I can't assign you to the Issue, but here is the link.

#38647

@Sisanu
Copy link
Contributor Author

@talldan, @alexstine, it's ready for review #38966

@talldan
Copy link
Contributor

@Sisanu If you write something like 'Fixes #38647' or 'Closes #38647' in the PR description, there's a bot that will assign you to the issue and github will automatically close the issue when the PR is merged.

@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Mar 29, 2022
Mamaduka pushed a commit that referenced this pull request Mar 29, 2022
…38136)

* Adds aria-label to the search button, as accessibility enhancement

* Strip all tags for the aria label attribute

* Add aria label only when the button is an icon

Co-authored-by: Kelly Dwan <ryelle@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Search Affects the Search Block - used to display a search field First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants