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

Use a consistent color for all controls and inputs labels #66454

Open
2 of 6 tasks
afercia opened this issue Oct 25, 2024 · 11 comments · May be fixed by #67436
Open
2 of 6 tasks

Use a consistent color for all controls and inputs labels #66454

afercia opened this issue Oct 25, 2024 · 11 comments · May be fixed by #67436
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Needs design efforts. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Oct 25, 2024

Description

It appears there's some inconsistency with the color used for various labels in the editor. I'm not sure what is the value of using different shades of gray, I'd tend to think all labels should use the same color.

Also, for accessibility, it would be good to not just aim to be compliant with the minimum required color contrast ratio. It would be good to use a gray that is darker than #757575 ($gray-700). On a white background, #757575 produces a contrast ratio of 4.61:1 that is just a little above the minimum required of 4.5:1. I'd argue that $gray-700 may be used for non-essential information e.g. descriptions but labels should be more readable.

A few examples taken from the Styles > Background UI. There may be more cases, these are just examples.

Image

  1. $gray-900 - #1e1e1e - I would like this to be the standard for all labels as it's the shade of gray that provides better readability.
  2. #3c434a - This is inherited from the body, it comes from the wp-admin common.css. I'd say this is a bug because the editor shouldn't rely on colors provided by external stylesheets.
  3. $gray-700 - #757575 - This is unnecessarily too light.

The ToggleGroupControl case

It could be argued that the non-selected options in the ToggleGroupControl use a lighter gray because they are 'inactive'. To me, that's not correct. These are enabled controls that can be selected. Rather than being 'inactive', they are just not-selected. Under the hood, the ToggleGroupControl is the equivalent of a group of radio buttons. The labels of a group of radio buttons use the same color. There's no need to communicate the 'selected' state by the means of color because the selection state is inherently communicated by the visuals of the selected radio button. In the same way, there's no need to use a lighter gray in the ToggleGroupControl because the selected option is visually distinguished by a a very noticeable inversion of colors:

Image

On top of that, for accessibility reasons, there's no reason to make the not-selected options less readable. I would expect these to use the normal gray used for other labels.

Also, when there is no option selected, I'd argue that they all look 'disabled' because of the lighter gray. Screenshot:

Image

Image

For consistency and better accessibility, I'd rather consider to standardize the color for all labels.

Step-by-step reproduction instructions

  • Go through the examples above.
  • Observe the labels use different shades of gray.
  • Optionally check other labels of other controls.

Screenshots, screen recording, code snippet

No response

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

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended Needs Design Needs design efforts. labels Oct 25, 2024
valerogarte added a commit to valerogarte/gutenberg that referenced this issue Nov 29, 2024
@valerogarte valerogarte linked a pull request Nov 29, 2024 that will close this issue
@t-hamano
Copy link
Contributor

I think this issue potentially contains a broader concern. This means that there are components whose label color or text changes depending on where they are placed.

Let's use the ToggleGroupControl as an example.

The label of this component does not have a color. Therefore, in the background image popover, its color is #3c434a, as it inherits the default text color from the WP-Admin style:

Image

On the other hand, in the block sidebar, the label color will be #1e1e1e ($gray-900) due to the definition here:

Image

As a result, the label color changes depending on where this component is placed.

On the other hand, for example, the label of the InputControl component is based on the Text component and has a defined text color, so they will not be overridden by WP-Admin styles.

Maybe we need to review the labels on all components and apply a uniform color. Also, not only the labels but also the text of the CheckboxControl, RadioControl, and ToggleControl components do not have color, so we might need to consider them at the same time.

cc @WordPress/gutenberg-components

@t-hamano t-hamano added the [Package] Components /packages/components label Nov 30, 2024
@afercia
Copy link
Contributor Author

afercia commented Dec 2, 2024

Also, not only the labels but also the text of the CheckboxControl, RadioControl, and ToggleControl components do not have color,

Good catch. And yes, I'd agree the case where color isn't set and thus inherits from the WP admin context should be prevented.

@valerogarte
Copy link
Contributor

I agree that unifying label colors is essential for enhancing accessibility and visual consistency. It's also crucial to review all components to ensure they function as expected. However, I wonder if it would be more effective to implement this change at a global level to ensure it applies universally. This is my second PR in WordPress, and I'm open to your suggestions.

@ciampo
Copy link
Contributor

ciampo commented Dec 11, 2024

I agree that this warrants a broader conversation. Should every component define its text color, or should it inherit it from its context / CSS cascade?

We would probably prefer the former, also given that we have plans to implement theming (which would easily allow consumers to tweak text color anyway). But IMO the most important thing would be to ensure consistency across the package.

@valerogarte
Copy link
Contributor

To move forward with this issue and unblock the task, how should we proceed? Where and how is this topic being discussed?

@mirka
Copy link
Member

mirka commented Dec 19, 2024

I suggest we start with an audit of our main "control" components to see which ones need fixing.

These are listed in Storybook under Components ▸ Selection & Input ▸ Common and Components (Experimental) ▸ Selection & Input.

A quick way to check if labels have explicit colors set is to use the experimental theme switcher tool. If the label is not visible in dark mode, it likely doesn't have an explicit color set. (Or does not a have a theme-ready color variable set, which we should take this opportunity to fix!)

CleanShot.2024-12-20.at.04.21.32.mp4

@t-hamano
Copy link
Contributor

I looked into components whose labels become invisible in dark mode, including components with the __experimental prefix. Some components are combinations of several components, so the number of components we have to deal with should actually be less than it actually is.

AnglePickerControl

Image

CustomGradientPicker

Image

FocalPointPicker

Image

GradientPicker

Image

InputControl

Image

NumberControl

Image

PaletteEdit

Image

QueryControl

Image

SelectControl

Image

TreeSelect

Image

UnitControl

Image

@ciampo
Copy link
Contributor

ciampo commented Dec 20, 2024

Hopefully this should be as easy as changing the text color to --wp-components-color-foreground for the time being.

In the future, it would be nice if we could have all (most) controls use BaseControl for better consistency (related: #60836)

@im3dabasia
Copy link
Contributor

I looked into components whose labels become invisible in dark mode.

Hey @t-hamano , Thanks for pointing this out. Can I raise a PR for this fix? I can create a new issue or link this comment in the PR description.

Looking forward to your thoughts.

@t-hamano
Copy link
Contributor

@im3dabasia Thanks for taking an interest in this issue.

Can I raise a PR for this fix? I can create a new issue or link this #66454 (comment) in the PR description.

The problem I raised is part of this issue, so I think it would be fine to just submit a PR instead of submitting a new issue.

@tyxla
Copy link
Member

tyxla commented Dec 30, 2024

#68349 has just landed, thanks @im3dabasia for the help!

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). Needs Design Needs design efforts. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
7 participants