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>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

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

@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.

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)

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).

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.

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.

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.

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?

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)

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.

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)

how difficult would it be to add an component as a child of ItemGroup, so that we re-instate correct list semantics?

@ciampo ItemGroup already ships with an Item sub-component that by default has a role="listitem" that comes from useItem.

The ItemGroup documentation clearly states that:

ItemGroup should be used in combination with the Item sub-component.

However, the component doesn't enforce this and is open to misuse. In fact, contributors to this project did use ItemGroup incorrectly a few times for example wrapping it around a Button just to have borders and provide a different styling. I'm surprised that such incorrect usage has been overlooked during code reviews, merged and released so lightly. This kind of process leads to releasing subpar code quality and that's just not OK.

One more consideration. Not to be picky, But even if we would 'fix' such incorrect usages by adding the missing Item so that the HTML would be something like list > listitem > button, I would argue that's semantically incorrect as well because a list with just one item is not a list in the first place.

Hey @afercia, @t-hamano,

What would be the ideal implementation of this PR we are expecting?

Currently, as a temporary fix, I have added the role="none" which solves the wrong HTML and also helps improve the a11y. Is this approach OK for this PR?

Let me know what you think.

• 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)

It will take more longer time to solve the problems that have been discussed here, so the realistic approach is either of the above for now.

• Personally, using role=none feels more hacky than overriding the CSS.
• Other similar UIs use a CSS override approach (Example).

• Personally, using role=none feels more hacky than overriding the CSS.
• Other similar UIs use a CSS override approach (Example).

Thank you for the feedback. Since role=none seems like a hacky solution, I have removed the ItemGroup and added custom CSS to achieve similar design requirements. I understand that we are gradually moving away from ItemGroup in GB. This solution should help us align with that direction as well

Attaching the screenshot for reference.

Hey @t-hamano,

I've updated the PR with the suggestions and feedback you provided. This PR now aligns with the approach used in the example you shared earlier. When you have a chance, would you please review it?

I believe this change is similar to the one in #68672.

I look forward to your feedback.

I've made the requested changes.

Since you've reviewed this PR, I wanted to mention I solved a similar issue here: #68631. Would appreciate your thoughts on that approach as well.