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

Accessibility Fix: Add tooltips to import, export, and modal close buttons #667

Closed

Conversation

aurooba
Copy link
Member

@aurooba aurooba commented Oct 7, 2023

What is this PR doing?

This PR adds tooltips to the import, export, and modal close buttons.

What problem is it solving?

While they already have aria-label visually users still need the tooltips so they can understand what these icon buttons do without interacting with them first.

How is the problem addressed?

A title attribute has been added to each button element that has the same text as the aria-label helper text.

Testing Instructions

  1. Run the playground
  2. Hover on the import or export icons in the header, and see the tooltip appear with the helper text.

Screenshot

screenshot showing the tooltip when you hover on the import button

@aurooba
Copy link
Member Author

aurooba commented Oct 7, 2023

Note: these should really be translation ready. Is there something already included within Playground that we can wrap this in, some kind of i18n function?

Copy link
Collaborator

@dmsnell dmsnell 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 the insight, @aurooba

I've got a question about duplicating the aria-label value when the title exists, but otherwise this seems like a good change we can merge quickly.

unfortunately there's no system in place yet here for translation strings, unless I'm mistaken.

return (
<button
id="export-playground--btn"
className={css.btn}
aria-label="Download Playground export as ZIP file"
aria-label={exportButtonAriaLabel}
title={exportButtonAriaLabel}
Copy link
Collaborator

Choose a reason for hiding this comment

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

with the title set is the aria-label necessary? I was led to believe that it becomes redundant at that point, and only matters if the aria-label value is different than the title, in which case its value is preferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Although screen-readers will read the title if it's there (in some cases only), they prefer aria-label and users can set preferences where only aria-label is read. So it's considered a better practice to include both. Here's some more information: https://www.a11yproject.com/posts/title-attributes/

@aurooba aurooba requested a review from dmsnell October 7, 2023 23:37
@alexstine
Copy link

Actually, the title attribute is treated differently in every screen reader even in today's time. Some screen readers will only read it under very specific conditions, some will ignore, and others will read duplicate information with the aria-label and title. I think this will likely cause an accessibility regression so let's find another solution here.

Thanks.

Copy link

@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.

While we're here, some labeling notes.

@@ -9,11 +9,15 @@ export default function ExportButton() {
if (!playground) {
return null;
}

const exportButtonAriaLabel = 'Download Playground export as ZIP file';

Choose a reason for hiding this comment

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

Save verbosity?

Suggested change
const exportButtonAriaLabel = 'Download Playground export as ZIP file';
const exportButtonAriaLabel = 'Download configuration .zip file';

Copy link
Member Author

Choose a reason for hiding this comment

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

I would suggest a further change, Export configuration .zip file

@@ -8,6 +8,7 @@ export default function ImportButton() {
const [isOpen, setOpen] = useState(false);
const openModal = () => setOpen(true);
const closeModal = () => setOpen(false);
const importButtonAriaLabel = 'Open Playground import window';

Choose a reason for hiding this comment

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

This text is redundant.

Suggested change
const importButtonAriaLabel = 'Open Playground import window';
const importButtonAriaLabel = 'Import configuration';

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

export default function Modal(props: ModalProps) {
const closeBtnAriaLabel = 'Close import window';

Choose a reason for hiding this comment

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

Since the modal provides current context, can shorten to just Close.

Suggested change
const closeBtnAriaLabel = 'Close import window';
const closeBtnAriaLabel = 'Close';

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

@aurooba
Copy link
Member Author

aurooba commented Oct 8, 2023

@alexstine Good call on changing the labels, I didn't want to express any opinions there but I agree that the actual language could be updated, and I agree with your suggestions.

Regarding the title and aria-label issue, thanks for the additional insight! My own experiments and research are obviously a little more limited compared to your experience so that's really helpful!

Should we skip using title and use a javascript library for the tooltip that uses the aria-label? Because it really does need a tooltip for sighted users, I found figuring out the icons without helper text very frustrating and had to inspect the code for the aria-label 😅.

For reference, in the Block Editor, the tooltip component uses the @ariakit/react Tooltip component

aurooba and others added 2 commits October 8, 2023 08:12
Co-authored-by: aurooba <aurooba@auroobamakes.com
Co-authored-by: alexstine <alex.stine@yourtechadvisors.com>
@alexstine
Copy link

@aurooba Why not use the one from Gutenberg WordPress components?

https://github.com/WordPress/gutenberg/tree/trunk/packages/components/src/tooltip

@adamziel
Copy link
Collaborator

adamziel commented Oct 9, 2023

This is lovely @aurooba, thank you so much!

It really does need a tooltip for sighted users

Yeah, the button could be much clearer about its purpose. Let's take a step back and think of what a clearer button would look like.

Maybe let's replacing these icons with textual buttons like import and export or save and restore? The layout won't look good on mobile, but maybe hiding the buttons on mobile devices would be fine? Uploading a Playground zip bundle instance from an iPhone seems highly unlikely.

adamziel added a commit that referenced this pull request Nov 15, 2023
## What is this PR doing?

Puts additional actions in a single, accessible dropdown menu instead of
jamming more ambiguous icons in the "toolbar".

Closes #667
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants