Global Styles: Prevent Unwanted ItemGroup List Rendering in Border Panel

[im3dabasia]

Part of: #67425

What?

Revised <ItemGroup> to use role="none" and prevent it from rendering an unnecessary role="list" when it is not semantically required.

Why?

Avoids adding unwanted list roles where a semantic list is not necessary, ensuring accessibility standards are maintained without enforcing unnecessary constraints.

How?

Updated <ItemGroup> to include role="none" and an optional as prop for flexibility. Removed instances of role="list" where a list structure wasn't required.

Testing Instructions

1. Add a new post.
2. Add a block, such as a button, that includes shadow support.
3. Inspect the "Drop Shadow" section of the button and observe the rendered ItemsGroup element.
4. Validate that the UI behavior and appearance remain unchanged.

Screenshots or screencast

Before

After

[github-actions]

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: ciampo <mciampini@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.

[afercia]

@im3dabasia thanks for your PR.
No objections to this temporary fix but fundamentally it dosn't solve the broader issue. ItemGroup is still used incorrectly and against its documented usage.

Apparently, ItemGroup has been used here only to implement a design for a bordered gray button, which is also inconsistent with the Button component default style variants. To me, this usage is entirely wrong under all aspects, also from a design perspective because it introduces one more button styling that is inconsistent and just overrides the default styles.
If such a button styling is really desired, it should be implemented in the base Button component by adding a new style variant.
Cc @WordPress/gutenberg-components @WordPress/gutenberg-design

As a quick fix, I have no objections to add role=none. I recommend to add a // TODO: inline comment to explain why there is a role=none and to mark this as something that needs a better solution in the future.

[afercia]

I suggest to add an inline comment // TODO: to explain why this role none is used here, as a temporary fix. There are a few examples of todo comments in the codebase for inspiration.

[afercia]

See comment.

[ciampo]

We on @WordPress/gutenberg-components don't currently have much time to dedicate to work on Gutenberg, but my recommendation would be to keep changes to the original component to a minimum (ideally avoid them), and instead gather info to inform the next iteration of the component (#64445)

[t-hamano]

Is there any reason why we couldn't simply refactor it to use a Button component rather than an ItemGroup component?

We're doing a similar refactoring in #68672.

There's also a proposal to abstract such components, by the way. See #68973 (comment).

[afercia]

Is there any reason why we couldn't simply refactor it to use a Button component rather than an ItemGroup component?

Ideally, yes.
It is worth considering the ItemGroup is planned to be refactored so I would think improvements to the component should wait. Hopefully, the new implementation of ItemGroup will enforce correct usage.

To me, one of the main reasons of components being used incorrectly is because design wants a new styling for Buttons or other components.

• Overriding base components styles and functionality with ad-hoc, local, CSS or code hacks. This should not happen. As mentioned in other issues, it leads to non cohesive UI, maintenance cost in the long term and inconsistencies.
• Base components should be used based on their documented intended usage.
• When a new styling is desired, it should be evaluated and implemented in the base components.
• Alternatively, as being considered for the Button component, the base component should provide a 'naked' version that can be styled ad hoc. This should be handled with care though, to not introduce dozens of ad-hoc styling.
• The fact ItemGroup is used this way just means contributors in this project don't know, don't care, about the actual rendered markup and often use components only based on the desired visual appearance but they ignore semantics, usability and accessibility. I'm surprised these ItemGroup implementations have passed a review and have been merged so lightly without some more thoughtful consideration.

The above isn't to blame individuals. Rather, as I mentioned a few times, I think the process should be improved to prevent such incorrect usage of components. Also, reviews should be more thoughtful.

That said, yes this should be a Button component. I'm not opposed to a temporary fix. A more long term fix should be improving the base components to allow the desired design or, alternatively, reconsider the design to work within the constraints of what the base components provide.

[t-hamano]

I agree that the current implementation is not ideal, but I don't think it's necessary to solve everything at once.

What is required in this PR? In my opinion, replace it with the Button component and solve the accessibility issues.

It will require some overrides with custom styles (gray border), but I think improvements to the Button and ItemGroup components can be addressed incrementally in follow-up PRs.

[afercia]

It will require some overrides with custom styles (gray border)

I think the whole point here is that Design should not implement stylings that aren't provided by the base components. This design trend in this project entirely defeats the purpose of having a library of reusable, consistent, tested, and accessible components. I'd rather change the design here and avoid any custom styles / CSS overrides.

[t-hamano]

I'd rather change the design here and avoid any custom styles / CSS overrides.

Does this mean we pause this PR and add an API to the Button component to achieve the grey border?

[ciampo]

Dropping in with some quick thoughts:

• changing the Button's APIs to add a new gray-bordered style variant just for this usecase doesn't seem like a good idea;
• I don't think that the explanation here is correct. ItemGroup was introduced to list items with a given layout / look; these items don't have to be buttons, and even when they effectively are buttons, they don't "compete" with the Button element. In this specific scenario, the list is a one-item list. And that one item happens to include a button that triggers a dropdown.
• If the current situation can't stand, and we think the correct implementation is just of have normal button semantics, the two least disruptive solutions to me feel like:
  • use ItemGroup and override roles, like suggested in this PR;
  • use a vanilla button component, and style it as it needs to look (which will avoid overriding Button styles)

[afercia]

In this specific scenario, the list is a one-item list. And that one item happens to include a button that triggers a dropdown.

@ciampo I think you are missing the point here.

the list is a one-item list

It's not a list actually. It's a broken list.

The equivalent in HTML would be as the following examples explained in the issue:

An <ul> element that contains a span and no list items:

<ul>
    <span>Some text</span>
</ul>

A button that contains an <ul> element that contains s span:

<button>
    <ul> <-- Element ul not allowed as child of element button 
            <span> <-- Element span not allowed as child of element ul 
                Some content
            </span>
    </ul>
</button>

Both are not acceptable from a semantic perspective and if this was coded in HTML it would be more evident that it's, I would say, embarrassing and unprofessional HTML. I'm not saying that lightly.

My point is that if the design wants to achieve some custom styling (which is already arguable to me), the implementation should not hack around base components and use them incorrectly. Instead, if these nrew styling are really necessary they should be implemented in the base components as that's the only way to guarantee a consistent UI, maintenance, and avoid bugs. Otherwise, these custom design should simply not be implemented.

[ciampo]

It's not a list actually. It's a broken list.

I missed that — and how difficult would it be to add an <Item> component as a child of ItemGroup, so that we re-instate correct list semantics?

(sorry for not being able to verify that on my end, I just don't happen to have much time on my hands these days)