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.

Sign up for GitHub

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

Jump to bottom

About the 2d slider #152

Open
diegohaz opened this issue Aug 10, 2021 · 6 comments · May be fixed by #159
Open

About the 2d slider #152

diegohaz opened this issue Aug 10, 2021 · 6 comments · May be fixed by #159
Labels
help wanted Extra attention is needed

Comments

@diegohaz
Copy link

diegohaz commented Aug 10, 2021 •
edited

Currently, the saturation and brightness picker is using a single role="slider" element, which is supposed to be one-dimensional, but is being used to control a two-dimensional pane.

One of the problems with this approach is the lack of aria-valuenow, which is a required attribute on sliders, but can't be set here because it's dealing with two different axes.

This may be also confusing for screen reader users who expect the slider to work as described in the WAI-ARIA docs.

One of the solutions is using a role="grid" element. This has been discussed on w3c/aria#432. The markup could be something like this:

<div role="grid" aria-label="Saturation and Brightness" aria-rowcount="100">
  <!-- 80% brightness -->
  <div role="row" aria-label="Brightness 80%" aria-rowindex="80">
    <!-- 20% saturation -->
    <div role="gridcell" aria-label="Saturation 20%" aria-colcount="100" aria-colindex="20" />
  </div>
</div>

Another solution would be having two sliders:

<div role="slider" aria-orientation="vertical" aria-valuenow="80" aria-label="Brightness" />
<div role="slider" aria-orientation="horizontal" aria-valuenow="20" aria-label="Saturation" />

Moving the arrow keys perpendicularly to the currently focused slider would move focus onto the other slider.

None of these solutions should affect the current layouts.
@diegohaz diegohaz mentioned this issue Aug 10, 2021
Merged
7 tasks
@omgovich
Copy link
Owner

omgovich commented Aug 11, 2021 •
edited

Hi! 👋 Thanks for using react-colorful and for the issue. I'll dig into the problem and try to roll out an approach that would provide better a11y and keep the bundle size as small as possible 👌

@eGene
Copy link

eGene commented Sep 14, 2021 •
edited

Saturation and Hue components should accept props from user, so that we could override aria-* attributes. This way I could just pass something like

saturationInputProp={{
  'aria-valuenow': '#12,34,56',
  'aria-label': 'Цвет'
}}

This also solves issue with hardcoded strings if I need non-english labels.

@omgovich
Copy link
Owner

omgovich commented Sep 24, 2021 •
edited

Hey guys! Don't think I forget about you. I've been discussing the issue with guys that know more about a11y than me.

The thing that bothers me much and I still didn't get — which element must take the focus? grid or gridcell?

@omgovich omgovich linked a pull request Sep 24, 2021 that will close this issue
Open
@omgovich
Copy link
Owner

omgovich commented Sep 24, 2021 •
edited

Hey @diegohaz. Could you please check https://omgovich.github.io/react-colorful/

I deployed a version with role="grid" (see the PR) there but seems to me that it doesn't work well: My macOS VoiceOver doesn't say anything when I change value.

@diegohaz
Copy link
Author

Hey @diegohaz. Could you please check https://omgovich.github.io/react-colorful/

I deployed a version with role="grid" (see the PR) there but seems to me that it doesn't work well: My macOS VoiceOver doesn't say anything when I change value.

Yeah! I can see the same behavior. VoiceOver also has its own shortcut keys to navigate grids, and they don't seem to work with this virtualized grid approach.

Have you tried the solution with two sliders?

@omgovich
Copy link
Owner

Not sure I understand how to make it work 😞

The thing is we need to set focus on the entire brightness/saturation area, but I don't understand how to do that if there are two separate div-s with role="slider"...

@omgovich omgovich added the help wanted Extra attention is needed label Nov 16, 2021
@aitchiss aitchiss mentioned this issue Feb 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Better a11y: Use grid role in Saturation component omgovich/react-colorful
3 participants
@omgovich @diegohaz @eGene