Toolbar: set toolbar variant only when in block toolbar

[ciampo]

Yea, I couldn't identify why the default was applying is-alternate here, so setting undefined variant resulted in the standard shadow.

Since #51154, changes were made so that DropdownMenu and Dropdown components rendered inside Toolbar would automatically pick the toolbar variant — that is achieved through the internal components context system (see here) and it causes the popover to render with the is-alternate classname.

So, given the previous requirements, the popover is currently behaving like expected: it's rendering a black border because it's rendered inside a Toolbar component.

From what I understand, the requirements that we gathered initially were incomplete, and what we'd really want is the toolbar popover variant to be only applied in popovers opened from the block toolbar (and not from any toolbar).

Therefore, this issue might need to be resolved in a higher-level component. Otherwise, similar problems may occur when new popovers are added to the header in the future.

That is correct. I guess for now, we could keep the changes from this PR. At the same time, we could work on refining the design requirements once for all, and then apply them at the components level.

Originally posted by @ciampo in #54840 (comment)

[brookewp]

Hi @jasmussen! I saw you confirmed that the black border for the dropdown menu should only appear in the block toolbar. However, I wanted to check on a few other block-related menus to ensure we have the full requirements. My understanding is the black border is to add contrast over editor content, but some scenarios contradict my assumption:

Inline

Previews

List view settings

Small screens

The following appears to be correct, but I wanted to double-check since the toolbar border color changes on smaller screens:

[jasmussen]

Hey @brookewp, thanks for checking in!

My understanding is the black border is to add contrast over editor content

No, the text inside each menu item is what's carrying the necessary contrast, the border around both block toolbars and popovers is decorative.

To that end this is more about elevation, or materials, so to speak. For that reason, the heuristic is that the dropdown menu should be made of the same material as the container that opened it. For example:

• The floating block toolbar has a dark border, therefore all menus opened should have the same.
• The docked top toolbar has a gray border, therefore all menus opened should be the shadowed/gray-bordered menus.

Makes sense? We can flex a bit, it's more of an optimization than a rush job to get this right.

[brookewp]

the dropdown menu should be made of the same material as the container that opened it

Thanks for looking, @jasmussen! To confirm I understand - in the examples I shared above, all are correct except for the view on smaller screens since there is a black border around the menu while the Toolbar has the grey menu. Is that right?

[jasmussen]

Yep, all those are correct except the view on the smaller screen. But I understand how that one is tricky because it's technically still the block toolbar, suddenly it just shifted position to be docked to the top. If that isn't easily fixed, that's okay, because we probably need to revisit mobile separately, and some different rules might come into play.

[ciampo]

Given that the aim is to visually match the border of the dropdown, in a way the current (trunk) behaviour seems correct in that all dropdowns opened inside a Toolbar match its borders, which are dark by default.

The issue arises more from the fact that Toolbar styles are sometimes overridden around the editor — this is the case for both the header toolbar (see override), like discussed in #54840, and for the block toolbar on small viewports (see override).

A few ideas on how to move forward:

1. I guess one way could be for the Toolbar component to also expose a variant prop, so that we can specify a "borderless" variant — when the borderless variant is on, we don't change the popover border.
2. Alternatively, we could try to "override" popover styles in those two specific toolbar instances by using the context — although it feels like a hacker approach.
3. An alternative to the previous approach would be to simply adopt the solution already implemented in Fix ToolSelector popover variant #54840 — ie. override the popover variant via the popoverProps prop

[brookewp]

Thanks for sharing options, @ciampo!

when the borderless variant is on, we don't change the popover border

In this option, we'd still need context for the popover variant when the toolbar's borderless variant isn't present, right? Maybe something like:

diff --git a/packages/components/src/toolbar/toolbar/index.tsx b/packages/components/src/toolbar/toolbar/index.tsx
index b6e83dcf6d..b9c725e849 100644
--- a/packages/components/src/toolbar/toolbar/index.tsx
+++ b/packages/components/src/toolbar/toolbar/index.tsx
@@ -19,19 +19,26 @@ import type { ToolbarProps } from './types';
 import type { WordPressComponentProps } from '../../context';
 import { ContextSystemProvider } from '../../context';
 
-const CONTEXT_SYSTEM_VALUE = {
-	DropdownMenu: {
-		variant: 'toolbar',
-	},
-	Dropdown: {
-		variant: 'toolbar',
-	},
+const CONTEXT_SYSTEM_VALUE = ( variant: string | undefined ) => {
+	if ( variant !== undefined ) {
+		return {};
+	}
+
+	return {
+		DropdownMenu: {
+			variant: 'toolbar',
+		},
+		Dropdown: {
+			variant: 'toolbar',
+		},
+	};
 };
 
 function UnforwardedToolbar(
 	{
 		className,
 		label,
+		variant,
 		...props
 	}: WordPressComponentProps< ToolbarProps, 'div', false >,
 	ref: ForwardedRef< any >
@@ -55,10 +62,12 @@ function UnforwardedToolbar(
 	// `ToolbarGroup` already uses components-toolbar for compatibility reasons.
 	const finalClassName = classnames(
 		'components-accessible-toolbar',
-		className
+		className,
+		variant === undefined ? undefined : `is-${ variant }`
 	);
+
 	return (
-		<ContextSystemProvider value={ CONTEXT_SYSTEM_VALUE }>
+		<ContextSystemProvider value={ CONTEXT_SYSTEM_VALUE( variant ) }>
 			<ToolbarContainer
 				className={ finalClassName }
 				label={ label }

However, this doesn't help with the mobile scenario. In that case, I'm unsure how I'd approach it without using CSS and the specific classes/media queries already in place. 🤔 Thoughts?

[ciampo]

Maybe something like:

Yup, something like that

However, this doesn't help with the mobile scenario. In that case, I'm unsure how I'd approach it without using CSS and the specific classes/media queries already in place. 🤔 Thoughts?

We'd just need to set the variant correctly — i took a quick look and it seems like there is already a isFixed prop that we could use — something like variant={ isFixed ? VARIANT_NAME_WITHOUT_BORDERS : undefined }. In case this wasn't an option and we had to resort to reacting to the viewport size, we could use something like the useMediaQuery hook hook. What do you think ?

[brookewp]

We'd just need to set the variant correctly — i took a quick look and it seems like there is already a isFixed prop that we could use — something like variant={ isFixed ? VARIANT_NAME_WITHOUT_BORDERS : undefined }

Thank you! I was looking at the Toolbar in NavigableToolbar rather than at its usages, so this is very helpful. I've opened an issue with this solution: #55139

In case this wasn't an option and we had to resort to reacting to the viewport size, we could use something like the useMediaQuery hook hook. What do you think ?

I'm also glad you shared this info as I wasn't aware of this hook. Good to know!