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

Color picker: CopyButton is unlabeled and has buggy description and tooltip #57157

Open
afercia opened this issue Dec 18, 2023 · 13 comments · May be fixed by #57168
Open

Color picker: CopyButton is unlabeled and has buggy description and tooltip #57157

afercia opened this issue Dec 18, 2023 · 13 comments · May be fixed by #57168
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Dec 18, 2023

Description

WHen selecting a custom color via the color picker, there's a 'copy' button to copy the custom colod code.
This button has a few bugs:

  • The button is completely unlabelled: no text content, no aria-label, no accessible name provided by any other means. This appears to be one more case of a completely unlabelled control which highlights the component itself is open to misues. For no reason ever developers should be allowed to omit the accessible name. Providing proper label must be responsibility of the component Cc @ciampo
  • The button uses a tooltip based on the new ariakit implementation. By default, the ariakit toolitip sets an aria-describedby attribute with the tooltip text. The tooltip text is 'Copy'. This should be the accessible name instead. Usinfg this text as a description rather than a name is less than ideal. Also, aria-describedby is only set when hovering or focusing the button, which is less than ideal. If this is the default ariakit tooltip behavior, we should revise it.
  • The tooltip is supposed to change text to 'Copied!' after a successful copy operation. Instead, the tooltiip disappears on click even though the button has hideOnClick={ false }. As such, there is no feedbackn for users that their action was successful.
  • I'd argue this button should use visible text instead of an icon. Icons should only be used when there's not enough space for visible text. Also, there's a variety of Copy buttons in the Editor and they are all different, they all use a different confirmation mechanism, which doesn' thelp UI consistency and adds unnecessary cognitive load for users.

Step-by-step reproduction instructions

  • Select a paragraph block and click Color > Text in the settings sidebar.
  • Expand the color picker to set a custom color.
  • Inspect in the dev tools the Copy button > Accessibility tab, and observe the button is unlabelled.
  • Observe the button code in the componetn and see there's no text, no aria-label or any other labeling mechanism:

<CopyButton
size="small"
ref={ copyRef }
icon={ copy }
showTooltip={ false }
/>

  • Inspect the button in the dev tools.
  • Hover or focus the button and observe aria-describedby is set dynamically only when hovering or focusing.
  • Click the button and observe the tooltiip disappears. Expected: to stay visible and show the confirmation text Copied!.
  • Instead of clicking the button, use the keyboard and move focus to it. Press Enter and observe the tooltip does stay visible and does show the confirmation text Copied!.

Screenshots, screen recording, code snippet

Animated GIF to illustrate:

  • The button is unlabelled.
  • aria-describedby is set only when hovering (or focusing)
  • the tooltip disappears on click

color picker button

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Dec 18, 2023
@Sanyam-jain30
Copy link

Hey @afercia,
I am Sanyam, I understood the raised issue.
I have some expertise in solving these type of issues. So, can I please be assigned to the issue.
Thank you.

@afercia
Copy link
Contributor Author

afercia commented Dec 18, 2023

@Sanyam-jain30 hello. Generally, all contributors don't need to be assigned to an issue, it's just a matter of submitting a pull request :)
In this specific case I've already worked a little on this, as I also want to replace the icon button with a button that shows visible text.

@afercia afercia self-assigned this Dec 18, 2023
@afercia
Copy link
Contributor Author

afercia commented Dec 18, 2023

Worth noting that in #41222 which is a pull request that refactored a little the color picker, the tooltip seems to work correctly. See the video titled 'after' in the pull request description: it is clearly visible the tooltip text changes from 'Copy' to 'Copied!'.
It appears something has changed since Many 2022 and now the tooltip disappears.
Regardless, I'd like to not use an icon button and use visible text instead.

@Sanyam-jain30
Copy link

@Sanyam-jain30 hello. Generally, all contributors don't need to be assigned to an issue, it's just a matter of submitting a pull request :)
In this specific case I've already worked a little on this, as I also want to replace the icon button with a button that shows visible text.

Got it. Thank you for your response.

@ciampo
Copy link
Contributor

ciampo commented Dec 19, 2023

This appears to be one more case of a completely unlabelled control which highlights the component itself is open to misues. For no reason ever developers should be allowed to omit the accessible name. Providing proper label must be responsibility of the component Cc @ciampo

Button is different from input elements, since its accessible name can be set in many ways (contents, aria-label, aria-labelledby, ...); as discussed in previous occasions, it's practically impossible to prevent consumers from using it without an accessible name, if they really wanted to.

Since we already have #51740 to track this issue, I think it would be more efficient to list all of the unlabelled component instances that we come across directly in that issue's description.

(cc @WordPress/gutenberg-components )

By default, the ariakit toolitip sets an aria-describedby attribute with the tooltip text.

That's correct, and from what I understand, it's the recommended attribute to use with a tooltip — and it was also recommended in #48222 (comment).

The tooltip text is 'Copy'. This should be the accessible name instead.

I agree that "Copy" should be used as the accessible name, regardless of the tooltip — that should be as easy as adding label={ __( 'Copy' ) } to the button component.

Also, aria-describedby is only set when hovering or focusing the button, which is less than ideal. If this is the default ariakit tooltip behavior, we should revise it.

This behavior was probably introduced in #54312, which fixed a performance regression due exactly to the fact that all tooltips were always rendered to the DOM even when not visible, so I'm not sure we really have an alternative there.

  • The tooltip is supposed to change text to 'Copied!' after a successful copy operation. Instead, the tooltiip disappears on click even though the button has hideOnClick={ false }. As such, there is no feedbackn for users that their action was successful.

That is a regression that I also noticed and logged in #57210 (which I closed in favour of this PR). One interesting aspect to notice is that the tooltip shows correctly when interacting with the button using the keyboard.

  • I'd argue this button should use visible text instead of an icon. Icons should only be used when there's not enough space for visible text. Also, there's a variety of Copy buttons in the Editor and they are all different, they all use a different confirmation mechanism, which doesn' t help UI consistency and adds unnecessary cognitive load for users.

cc @WordPress/gutenberg-design

@jasmussen
Copy link
Contributor

Let's not change the button to a text-label button.

It is intentionally an icon button so as to manage its prominence. The primary flow here is one of changing colors, where copying the hex code is a tertiary action, after even pasting into the input, or selecting from the input and copying. The button exists there as a shortcut for an advanced flow, and thus needs a lower footprint.

@afercia
Copy link
Contributor Author

afercia commented Dec 21, 2023

That's correct, and from what I understand, it's the recommended attribute to use with a tooltip — and it was also recommended in #48222 (comment).

@ciampo it is not. I think I also mentioned previously that the ARIA tooltip pattern is still under discussion in teh W3C groups as there is no agreement on its implementation. We should not use it for now. The ariakit implementation has problems in at least tow aspects:

  • it sets a tabindex=0 to non-natively focusable elements when they get a tooltip. that's very arguable. For axeample, a div element with a tooltip becomes focusable. That is just wrong.
  • it sets aria-describedby dynamically, which is arguable,

Let's not change the button to a text-label button.

@jasmussen As mentioned in several other issues and PRs, icon only buttons are inherently an accessibility problem. FOr accessibility, we should avoid them and only use them when there is not enough space for visible text.

I'm reporting that this is an accessibility problem that needs to be solved but you're not providing an alternative sollution. I'm not sure just saying 'no' without providing an alternative solution is ok under all aspects.

It is intentionally an icon button so as to manage its prominence.

This is only a visual consideration and it's not relevant to solve the underlying accessibility problem. I would greatly appreciate that designers in thisi project stop thinking only visually and evolve towards a truly accessible design. Thanks.

@afercia
Copy link
Contributor Author

afercia commented Dec 21, 2023

Since we already have #51740 to track this issue, I think it would be more efficient to list all of the unlabelled component instances that we come across directly in that issue's description.

@ciampo I'm not sure running behind every instance where contributors mistakenly omitted to label a button or other interactive controls is 'efficient' in any way and I'm not sure that would be the best way to spend our time.

In the almost 7 years I've been following this project I've encountered several cases of unlabelled interactive controls. After 7 years, contributors keep coding, reviewing, and merging unlabelled controls. And they get released to users. To be fair, I'm not sure that's acceptable.

My intent isn't blaming anyone but it is a fact that some contributors to this project clearly lack the necessary education and knowledge to code in an accessible way. Labeling controls isn't rocket science. It's a basic requirement and I would say it's basic HTML. As I see it, we have two options:

  • Education and training for all contributors. I don;t think that's pragmatically feasible. Also, when components are used outside the scope of this project, we wouldn't have control on how they are used.
  • Make sure components can't be misused. To me, all components should be responsible for a correct, accessible, usage. If we don't make sure they're accessible, we're not contributing to make the web a better place.

it's practically impossible to prevent consumers from using it without an accessible name, if they really wanted to.

Honestly, I think we need to find a way. Nothing is impossible.

@ciampo
Copy link
Contributor

ciampo commented Dec 21, 2023

We should not use it for now. The ariakit implementation has problems in at least tow aspects:

Opened ariakit/ariakit#3242 to discuss directly with @diegohaz

@afercia
Copy link
Contributor Author

afercia commented Dec 21, 2023

Thanks @ciampo I would like to remind everyone that on this comment from March I did strongly recommend to not use a role=tooltip for now, and thus not use the ariakit tooltip. That ARIA role is still being discussed in the W3C groups. See all the references and links in that comment.
Apparently my feedback wasn't take into consideration and now we have a large, arguable, usage of role="tooltip" in the editor. 😞

@priethor
Copy link
Contributor

I would greatly appreciate that designers in thisi project stop thinking only visually and evolve towards a truly accessible design.

Hi Afercia, thanks for fostering this healthy discussion. I can do nothing but agree with the second part: we really need to embrace truly accessible design, and we should do so considering all accessibility needs. I am neither a UX expert nor an accessibility one, and while I absolutely agree we need to ensure visual accessibility, I believe we should also consider the effect our decisions have on users with different needs or expertise.

As said, I'm no expert in either field; hence, I'm unable to propose an informed path forward, but I felt the need to share that adding text to every button by default can result in an overwhelming UI where most possible actions fight for the user's attention, resulting also in a less accessible software for many people. 🙏

@afercia
Copy link
Contributor Author

afercia commented Dec 22, 2023

Thanks @priethor for sharing your thoughts and your commitment to universal design, I appreciate it ❤️

I think we should clarify some basic points:

  • Text is the only universal format that is understandable and accessible for everyone.
  • Icons are not.

This is not a personal opinion, it's a fact backed up by years of research, user testing, and actual data.

Icons are not universally usable.

As such, we need to avoid icons as much as possible.

I'm requesting here to change this button to use visible text.

I do realize the argument about visual prominence. Luckily, we have designers in this project that can take care of making the button with visible text less visually prominent as desired.

Using visible text wherever there is enough space to do it is an accessibility requirement. What I expect here is a new design proposal that solves both needs: accessibility and visual prominence. What I do not expect here is some feedback that just says 'no' without providing an alternative solution. Thanks.

@priethor
Copy link
Contributor

priethor commented Dec 22, 2023

Thanks again for taking the time to discuss this, Andrea! 🙇‍♂️

  • Text is the only universal format that is understandable and accessible for everyone.
  • Icons are not.

This is not a personal opinion, it's a fact backed up by years of research, user testing, and actual data.

Would you mind pointing to some of the research that backs up those facts? It would be beneficial for all contributors to see these strong statements backed up by studies and well-known accessibility organizations; I personally haven't found anything related to icon-only buttons in WCAG guidelines, for example.

However, the answers I find on the topic are more of a gray area (disclaimer again: I'm not an a11y expert), with opposing opinions, including the acceptance of icon-only buttons when their look and purpose are universally accepted, which seems applicable for the case of the copy button if we check the Google results for "copy button" (note: this is not a fact but my opinion loosely held by the Google results!). Moreover, there are recommendations to avoid cluttering the UI with text to help dyslexic users, which would mean text is not universally understandable.

What I do not expect here is some feedback that just says 'no' without providing an alternative solution.

I see your point and empathize with it; however, if the same logic is applied from the design point of view, something similar could be said about the stance that having text is a requirement, as no other solution is provided. There is no easy way forward as long as we draw these unmovable red lines; we all need to make some compromises and find some common ground.

Thinking out of the box: what if we concentrate less on finding the perfect solution for the default experience and provide customizable options so that users can decide how much verbose they want/need the UI during onboarding/through settings?

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). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants