Global Styles: Fix incorrect usage of ItemGroup in the Background image panel #68631

im3dabasia · 2025-01-13T09:45:57Z

Part of: #67425

What?

Improved the ItemGroup component to handle role attributes correctly based on its rendered HTML element. When rendered as a span, it now applies role="none" to prevent incorrect semantic meaning, and when rendered as a button, it correctly applies role="button".

Why?

The previous implementation incorrectly applied role="list" in scenarios where a semantic list wasn't appropriate, causing accessibility issues. This change ensures we maintain proper accessibility standards by applying the correct ARIA roles based on the actual HTML element being rendered.

How?

Updated the component to conditionally set the role attribute based on the as prop:

When as="span", it sets role="none" to remove any implied semantic meaning
Otherwise, it applies role="button" for proper accessibility

Testing Instructions

Navigate to Appearance > Editor
Navigate to Global Styles > Background.
Inspect rendered Button labeled 'Add background image' and confirm they no longer include role="list" when unnecessary.
Validate that UI behavior and appearance remain unaffected.

Before	After

|

github-actions · 2025-01-13T09:46:11Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <im3dabasia1@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

im3dabasia · 2025-02-06T07:27:44Z

Hey @afercia,

Whenever you have a moment, could you please review my PR? It addresses another instance of incorrect usage of the ItemGroup component in the Background Image panel.

I’d really appreciate your feedback!

afercia · 2025-02-06T08:28:30Z

packages/block-editor/src/components/background-image-control/index.js

@@ -125,7 +125,12 @@ function InspectorImagePreviewItem( {
 		}
 	}, [ toggleProps?.isOpen, onToggleCallback ] );
 	return (
-		<ItemGroup as={ as } className={ className } { ...toggleProps }>
+		<ItemGroup
+			role="none"


When the background image is set, ItemGroup is rendered as a button element. The role=none would override the native button role, which isn't okay.

Maybe we can solve this with imgUrl ? "none" : "button". I'll try it out and update you.

Or maybe based on the as prop. However, this would be just a temporary workaround.
This component is still used incorrectly and against its documentation. We can implement a temporary fix but the long term goal is to not allow this kind of incorrect usage.

afercia

@im3dabasia thanks for your PR. As mentioned in #67425 (comment) when the background image is set, this ItemGroup becomes a button element. Setting role=none on the button would override the native button role and assistive technology would not communicate this as a button.
One option could be setting the role conditionally but it wouldn't be great having a button element with role=button.
I'd rather remove the ItemGroup entirely and explore a design change.

im3dabasia · 2025-02-06T09:51:50Z

when the background image is set, this ItemGroup becomes a button element.

Thank you for your feedback @afercia .

I agree that this is happening, but the button has role="list", which I believe is incorrect. While redundant, it would be better to use role="button" rather than having a button element with role="list".

As a temporary fix, I’d like to propose two possible solutions. Let me know which one you think works best:

Add role prop value conditionally

<ItemGroup
  role={ as === 'span' ? 'none' : 'button' }
  as={as}
  className={className}
  {...toggleProps}
/>

This would add role="none" by default when a span is rendered and otherwise apply role="button".

Add role prop only when as === 'span'

<ItemGroup
  {...(as === 'span' ? { role: 'none' } : {})}
  as={as}
  className={className}
  {...toggleProps}
/>

This approach adds the role prop only when a span is rendered; otherwise, it allows the default behavior to prevail (i.e., a button with role="list").

Personally, I believe the first approach is better as it improves accessibility overall for this issue.

Looking forward to here your thoughts @afercia

afercia · 2025-02-06T10:39:48Z

a button with role="list"

We should avoid that so the first approach is better. However, this can only be acceptable as a temporary solution.
Please add an inline // TODO: comment to explain why there is a temporary fix for the role. It would be good to use the same 'todo' comment requested also for #68633

im3dabasia · 2025-03-10T18:22:22Z

Hi @afercia,

Sorry for the delay. I have updated the PR with the relevant changes as discussed. I have also added a TODO message for future reference. Please review my PR once again and provide any feedback.

Thank you for your time. I appreciate it.

im3dabasia · 2025-03-26T05:05:06Z

@t-hamano ,

When you have a moment please review this PR as well it solves a similar problem to the one just merged in trunk recently.

Thank you in advance!

t-hamano

We need to explore a temporary fix here, but I don't think simply changing the role attribute is enough.

Here are what I consider to be the problems.

The focus outline style is slightly different than that of the Button component:

The focus outline color is black:

These issues seem to be caused by customizing the ItemGroup component.

I would suggest, similar to #68633, to remove the ItemGroup component completely and use the Button component. We'll probably need to adjust the implementation of the InspectorImagePreviewItem component. What do you think?

im3dabasia · 2025-03-27T09:41:12Z

I would suggest, similar to #68633, to remove the ItemGroup component completely and use the Button component. We'll probably need to adjust the implementation of the InspectorImagePreviewItem component. What do you think?

Correct! I have adjusted the code in InspectorImagePreviewItem.

I have replaced ItemGroup with Button and am rendering the button conditionally when we have an Image URL. This prevents the issue of having 2 buttons piled up when no image exists (which happens when we initially come to the page, before uploading any image - as shown in the screenshot below).

This approach solves our problem and also eliminates the ItemGroup usage. Let me know what you think!

im3dabasia · 2025-03-28T06:52:27Z

@t-hamano ,

When you get a chance. Please review this PR. I feel we are close to shipping this 🚢

t-hamano

Thanks for the update!

Can you fix the button when the background image panel is open? It seems that the button elements are nested:

Additionally, we might be able to extract FlexItem:

function InspectorImagePreviewItem( {} ) {
	// ...
	const renderLabelContent = () => (
		<FlexItem>
			<Truncate>{ label }</Truncate>
			{/* ... */}
		</FlexItem>
	);

	return imgUrl ? (
		<Button>
			<HStack className="block-editor-global-styles-background-panel__inspector-preview-inner">
				{ imgUrl && {/* ... */} }
				{ renderLabelContent() }
			</HStack>
		</Button>
	) : (
		renderLabelContent()
	);
}

im3dabasia · 2025-03-28T08:50:40Z

Thank you @t-hamano for correctly pointing out these improvements. I appreciate your feedback.

I have implemented the changes and added them to the PR. Please take a look when you have a chance.

Video reference:

Screen.Recording.2025-03-28.at.2.16.34.PM.mov

t-hamano

@im3dabasia Can you investigate and fix the browser console error occurring when adding a background image? This error doesn't occur on trunk:

6d2b8da786d27d867d85b56e45454a29.mp4

im3dabasia · 2025-03-28T10:04:04Z

@t-hamano , The console error was occurring because the isOpen prop was passed to the Button component and was directly passed onto the button element. I have fixed this issue.

Please check once again.

t-hamano

LGTM!

Finally, can we do a small refactor like this?

const { isOpen, ...restToggleProps } = toggleProps;

useEffect( () => {
	if ( typeof isOpen !== 'undefined' ) {
		onToggleCallback( isOpen );
	}
}, [ isOpen, onToggleCallback ] );

im3dabasia · 2025-03-28T13:51:53Z

@t-hamano . Thanks for the continuous iterations of feedback. Please check when you have a moment

…ge panel (WordPress#68631) * : a11y issue in background image panel * fix: Role of button when rendered in span/button * revert: Fix unwanted storybook change * fix: Replace ItemGroup with Button * fix: Breading button nesting in popover * chore: Change render function name * fix: Error related to isOpen prop in button * chore: Code refactor Co-authored-by: im3dabasia <im3dabasia1@git.wordpress.org> Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>

im3dabasia requested a review from ellatrix as a code owner January 13, 2025 09:45

Mamaduka 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 Jan 13, 2025

afercia reviewed Feb 6, 2025

View reviewed changes

afercia requested changes Feb 6, 2025

View reviewed changes

im3dabasia requested a review from ajitbohra as a code owner March 10, 2025 18:13

im3dabasia force-pushed the fix/item-group-a11y branch from c763ce2 to fcbb2a9 Compare March 10, 2025 18:20

im3dabasia requested a review from afercia March 10, 2025 18:22

im3dabasia added 3 commits March 25, 2025 15:29

: a11y issue in background image panel

d8ad1a6

fix: Role of button when rendered in span/button

f8bfa30

revert: Fix unwanted storybook change

839cd48

im3dabasia force-pushed the fix/item-group-a11y branch from fcbb2a9 to 839cd48 Compare March 25, 2025 10:00

im3dabasia mentioned this pull request Mar 26, 2025

Global Styles: Prevent Unwanted ItemGroup List Rendering in Border Panel #68633

Merged

t-hamano reviewed Mar 27, 2025

View reviewed changes

fix: Replace ItemGroup with Button

e8269a9

im3dabasia requested a review from t-hamano March 27, 2025 09:43

t-hamano reviewed Mar 28, 2025

View reviewed changes

fix: Breading button nesting in popover

506389d

im3dabasia requested a review from t-hamano March 28, 2025 08:50

chore: Change render function name

8757ff9

t-hamano reviewed Mar 28, 2025

View reviewed changes

fix: Error related to isOpen prop in button

82ec635

im3dabasia requested a review from t-hamano March 28, 2025 10:07

t-hamano approved these changes Mar 28, 2025

View reviewed changes

chore: Code refactor

a09a54d

t-hamano merged commit d8545b2 into WordPress:trunk Mar 28, 2025
59 checks passed

github-actions bot added this to the Gutenberg 20.7 milestone Mar 28, 2025

t-hamano mentioned this pull request Mar 28, 2025

Background Image Panel: fix focus loss #69745

Closed

Global Styles: Fix incorrect usage of ItemGroup in the Background image panel #68631

Global Styles: Fix incorrect usage of ItemGroup in the Background image panel #68631

Uh oh!

Conversation

im3dabasia commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

How?

Testing Instructions

Uh oh!

github-actions bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

im3dabasia commented Feb 6, 2025

Uh oh!

afercia Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

im3dabasia Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afercia Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

afercia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Feb 6, 2025

Uh oh!

afercia commented Feb 6, 2025

Uh oh!

im3dabasia commented Mar 10, 2025

Uh oh!

im3dabasia commented Mar 26, 2025

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Mar 27, 2025

Uh oh!

im3dabasia commented Mar 28, 2025

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Mar 28, 2025

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Mar 28, 2025

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

im3dabasia commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

im3dabasia Feb 6, 2025 •

edited

Loading

afercia left a comment •

edited

Loading