Add an outline when the color picker select box is focused

[megane9988]

What?

Add an outline when the color picker select box is focused.

Why?

Fixes #50524

How?

add CSS

In #34598, there was a problem when the border of a select box was hidden, the focus outline disappeared along with the border. Therefore, the CSS is modified so the focus outline does not disappear.

Testing Instructions

• Select a block, including the color change function.
• Open color picker
• Focus select box (Hex, RGB, HSL) to use mouse or keyboard.

Testing Instructions for Keyboard

Screenshots or screencast

storybook

before

after

editor

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @megane9988! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[ciampo]

Thank you for working on this, @megane9988 !

The select dropdown in ColorPicker is definitely missing focus styles. I'll take a look at your PR and come back with any feedback!

[ciampo]

Alright!

Since SelectControl shows focus styles correctly in isolation (see Storybook), I looked into the ColorPicker component to see if there was any anomaly in the code — and indeed I noticed that #34598 introduced styles that hide the input's backdrop, which happens to be what renders the focus outline.

Basically, we can undo your changes so far and simply remove the style overrides for the backdrop (expand to see suggested changes)

diff --git a/packages/components/src/color-picker/styles.ts b/packages/components/src/color-picker/styles.ts
index bf25707ba9..ca7a10bcfe 100644
--- a/packages/components/src/color-picker/styles.ts
+++ b/packages/components/src/color-picker/styles.ts
@@ -14,10 +14,7 @@ import { boxSizingReset } from '../utils';
 import Button from '../button';
 import { Flex } from '../flex';
 import { HStack } from '../h-stack';
-import {
-	BackdropUI,
-	Container as InputControlContainer,
-} from '../input-control/styles/input-control-styles';
+import { Container as InputControlContainer } from '../input-control/styles/input-control-styles';
 import CONFIG from '../utils/config-values';
 
 export const NumberControlWrapper = styled( NumberControl )`
@@ -29,9 +26,6 @@ export const NumberControlWrapper = styled( NumberControl )`
 export const SelectControl = styled( InnerSelectControl )`
 	margin-left: ${ space( -2 ) };
 	width: 5em;
-	${ BackdropUI } {
-		display: none;
-	}
 `;
 
 export const RangeControl = styled( InnerRangeControl )`

Here's what the component looks like after these changes:

color.picker.select.focus.mp4

Finally, could you add an entry to the package's CHANGELOG, in the "Unreleased" section? You may need to create a new "Bug Fix" sub-section.

---

cc @jasmussen, to make you aware of this fix. Side note: should we look at updating focus styles for the two circular drag handles too, so that they show the same blue ring as the rest of the UI?

[afercia]

The BackdropUI component is responsible for providing:

• The default gray border.
• The blue outline focus style.

In #34598 BackdropUI was just hidden to remove the default gray border and match the new design. Unfortunately, that removed also the focus style. It appears no keyboard testing was performed.

Ideally, we shouldn't hack around BackdropUI. If a new behavior is desired, e.g. having no default gray border, that should be handled in the BackdropUI component itself by the means of a new prop. Overriding the BackdropUI behavior by hiding it with display: none doesn't seem to be ideal and introduces one more specific customization. I share the concern about too many specific customizations pointed out in the redesign pull request.

//Cc @ciampo

If a new BackdropUI behavior / prop is not desirable, I'd like to propose a different approach: Keep the BackdropUI focus style so to ensure the focus style comes from the component is designed for that. Something along these lines:

export const SelectControl = styled( InnerSelectControl )`
	margin-left: ${ space( -2 ) };
	width: 5em;
	${ BackdropUI } {
		display: none;
	}

	select:focus ~ ${ BackdropUI } {
		display: block;
	}
`;

[ciampo]

Thank you for the comment, @afercia.

I agree that we should try to limit the ad-hoc style overrides when we use components, and instead aim for consistency. Would it be ok for the SelectControl component to display borders when used in ColorPicker? If not, should we consider never showing borders for SelectControl, so that we have a coherent look across Gutenberg and less overrides?

As an alternative (although definitely not my favorite option), we could enable SelectControl to expose something like a variant prop, and we could prepare a variant of the component that doesn't show the border, while still showing focus styles correctly.

[megane9988]

@ciampo @afercia

Appreciate your comment.

As a first confirmation, we should give an outline when we focus.
Can we agree on this one? Is that correct?

Then, from the discussion between the two of you

• Whenever possible, no specific customization should be made to the component.
• If customization is essential, a prop should be used.

These are the opinions that have been expressed.

So possible ways to outline when focusing are

• Customize using a prop.
• Remove the CSS that is erasing the existing outline

It is one of these two.

The latter is considered less time-consuming to implement, but since this one is #34598 and will modify the design itself as suggested by others, we may need to check with the person who was in charge of the design.

In addition, in that case, this PR should be put on hold,
We should erect a separate issue to discuss whether the outer frame should be removed.

This would take a lot of time for discussion.

First, should we use this PR to realize the display of the outer frame only when the focus is on it?
Or, even if it takes time, should we discuss handling the outer frame first?

Please let us know which you think is better.

[afercia]

@ciampo I guess we posted our previous comments almost at the same time :D

Would it be ok for the SelectControl component to display borders when used in ColorPicker?

If you ask me, that would be the preferred option to me. Any interactive control should be clearly distinguishable which in most cases translates to using a shape e.g. a border. In this specific case, the redesign in #34598 asked to remove the border... 😭
There is a chevron icon that helps identifying this as an interactive control, otherwise that would look like just text. However, I'd prefer to keep the borders. If that's OK from a design perspective, I'd vote for just removing the CSS override. Hacking around / overriding components with ad-hoc customizations doesn't seem a good practice to me. That defeats the prupose of having reusable components in the first place.

[ciampo]

@jasmussen do you have any feedback related to this situation? I tried to propose a few potential alternatives in a previous message

[jasmussen]

Just to be sure I'm getting this right, it's a matter of ensuring that the Hex dropdown has a focus style on select, just like the copy button does?

If yes, then that seems a totally valid fix for an oversight (thanks for the PR!). Just make sure you use the right focus style, there are some mixins you can use, include @button-style__focus(); seems valid here. But you might want to also add the border-radius: $radius-block-ui; so it gets the rounded corners everything else has.

I would avoid the border around the select box, as it's meant to be a very lightweight dropdown.

[afercia]

there are some mixins you can use, include @button-style__focus(); seems valid here

The underlying technical point here is that the inputControl component (and BackdropUI) should provide the default style and the focus style our-of-the-box. We should avoid to override components styles with ad-hoc customizations. Also from a consistency perspective, I'm not sure why all the select dropdowns are expected to havre a default gray border and this specific dropdown doesn't. To me, UI consistency is more important than aesthetics.

Note: the styles of this component are provided via styled components (JS) and can't use scss mixins.

[jasmussen]

Understood. Agreed that this should use base componentry with no overrides.

I don't see this as a regular dropdown, though, for me it is comparable to an ellipsis menu. We can remove the down chevron if that helps. It's a toggle moreso than a select.

[ciampo]

Let's approach this iteratively.

The urgent part is to make sure that ColorPicker is accessible — and therefore, I suggest we apply this small tweak which will preserve the absence of border around SelectControl, while enabling focus styles

diff --git a/packages/components/src/color-picker/styles.ts b/packages/components/src/color-picker/styles.ts
index bf25707ba9..f78db13a8f 100644
--- a/packages/components/src/color-picker/styles.ts
+++ b/packages/components/src/color-picker/styles.ts
@@ -29,8 +29,13 @@ export const NumberControlWrapper = styled( NumberControl )`
 export const SelectControl = styled( InnerSelectControl )`
 	margin-left: ${ space( -2 ) };
 	width: 5em;
-	${ BackdropUI } {
-		display: none;
+	/*
+	 * Remove border, but preserve focus styles
+	 * TODO: this override should be removed,
+	 * see https://github.com/WordPress/gutenberg/pull/50609
+	 */
+	select:not( :focus ) ~ ${ BackdropUI }${ BackdropUI }${ BackdropUI } {
+		border-color: transparent;
 	}
 `;

It is hacky, but it will get the job done.

Separately, we can discuss better about the long-term solution:

• Should the color "mode" selection UI be a dropdown or a toggle?
• In any scenario, we will then have to understand if we can reuse existing components without tweaks. If tweaks are necessary, we can discuss how to come up with the best solution API-wise.

What do folks think ??

[megane9988]

I want to follow @ciampo 's idea.
I'll rewrite the PR.

[megane9988]

@afercia @jasmussen

I am following @ciampo 's idea, and I rewrote a PR again.

Of course, it's necessary to reconsider the relevant component.
However, at the moment, when the select box is focused, it is better to have the outline appear than not to have it.

What do you think? Please give us your opinion.

[ciampo]

I think this is a good compromise for the short term, thank you!

I will leave a final review once conflicts are solved

[megane9988]

@ciampo, thank you. I added an empty line.

[ciampo]

LGTM, thank everyone else for the collaboration!

@megane9988 feel free to ping me if you ever feel like doing more work on the @wordpress/components, always happy to help / review