Skip to content

Added aria label to the button block with icon#38966

Merged
talldan merged 6 commits into
WordPress:trunkfrom
Sisanu:enhancement/search-block/accessibility-attr-editor
Feb 25, 2022
Merged

Added aria label to the button block with icon#38966
talldan merged 6 commits into
WordPress:trunkfrom
Sisanu:enhancement/search-block/accessibility-attr-editor

Conversation

@Sisanu

@Sisanu Sisanu commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Description

Testing Instructions

Screenshots

Types of changes

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.

@Sisanu Sisanu requested a review from ajitbohra as a code owner February 21, 2022 18:45

@alexstine alexstine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sisanu Thanks for the PR. Please see my comment below. Otherwise, looking great.

Comment thread packages/block-library/src/search/edit.js Outdated
type="button"
className={ buttonClasses }
style={ buttonStyles }
aria-label={ label ? label : __( 'Search' ) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sisanu Probably going to want to make this buttonLabel. Need to figure out how to remove all the HTML tags though. Not sure if we have package for it or not.

import { __unstableStripHTML } from '@wordpress/dom';

That may be safe to use. I saw another PR get merged using it. Still I am unsure why it is marked unstable.

@Sisanu Sisanu Feb 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alexstine, are you aiming for something like this? As far as I understand we have a buttonText attribute.
aria-label={ buttonText ? stripHTML( buttonText ) : __( 'Search' ) }

https://capture.dropbox.com/B5BduQpKH0HJy4Fe

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sisanu Yes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Committed, please review.

@alexstine alexstine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sisanu looks good.

@talldan can you give this a quick look? I am confident this is done well but would like to get a review on the unstable strip HTML.

@talldan

talldan commented Feb 24, 2022

Copy link
Copy Markdown
Contributor

Thanks for working on this!

The only thing I'd mention is that we're using the label attribute in PHP for the front-end of a site (as changed in #38136) and the buttonText attribute in JavaScript for the editor.

I think it'd be good if the same attribute could be used in both places.

@alexstine

Copy link
Copy Markdown
Contributor

@talldan Nice catch. I totally thought we were using buttonText in that other PR, but guess not. This is super confusing. Should buttonText be used in both places since it is an aria-label for the button? If not, I am sure one quick commit can switch back to label for this PR.

Sorry for the confusion, I just don't understand very well how this side of the editor works and tries to tie in with old WordPress search.

@talldan talldan added [Block] Buttons Affects the Buttons Block [Type] Bug An existing feature does not function as intended labels Feb 25, 2022

@talldan talldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @Sisanu - the changes look good now.

@talldan talldan merged commit 9c5549e into WordPress:trunk Feb 25, 2022
@talldan talldan changed the title Added aria label to the button with icon Added aria label to the button block with icon Feb 25, 2022
@github-actions github-actions Bot added this to the Gutenberg 12.8 milestone Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Buttons Affects the Buttons Block [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants