Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ItemGroup can be misused and render unsemantic not accessible markup #67425

Open
2 of 6 tasks
afercia opened this issue Nov 29, 2024 · 5 comments
Open
2 of 6 tasks

ItemGroup can be misused and render unsemantic not accessible markup #67425

afercia opened this issue Nov 29, 2024 · 5 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Nov 29, 2024

Description

<ItemGroup> is supposed to 'display a list of Items grouped and styled together'.

The items are supposed to be visually and semantically grouped because the <ItemGroup> renders a div element with an ARIA role=list'. This is basically equivalent to an

    element. As such, it is supposed to only containsub-components that render adivelement withrole=listitem. This is basically equivalent to
  • ` elements.

    As such, <ItemGroup> is the ARIA equivalent of an unordered list with list items.

    The documentation clearly states that ItemGroup should be used in combination with the Item sub-component. The documentation example is pretty clear as well.

    However, in practice, ItemGroup accepts any children. There are several examples in the codebase where children other than <Item> are used within an <ItemGroup>. This is semantically incorrect and not accessible. It's basically the equivalent of an <ul> element that contains elements other than <li> elements.

    Unfortunately, contributors to this project seem to not read the documentation and misuse this component only to get the 'visual aspect' of the <ItemGroup> not realizing they're going to render unsemantic and not accessible markup.

    On the other hand, the <ItemGroup> component doesn't enforce its children to be <Item> sub-components. I would argue that it shouldm given its intended usage and its documentation.

    One example of this misuse is in the block bindings UI:

    Image

    <ItemGroup>
    <Text variant="muted">
    { __(
    'Attributes connected to custom fields or other dynamic data.'
    ) }
    </Text>
    </ItemGroup>

    where the text highlighted in the screenshot is a <span> element inside a <div> element with role=list. The equivalent in HTML would be something like the following:

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

    For the case in the screenshot above, that text should not use an <ItemGroup> in the first place. There are other usage cases in the editor that need to be investigated though.

    Still, this component is intended and documented to render a semantic list, while in practice it allows to be misused.

    About the parent where <ItemGroup> is used into

    Similar concerns raise when the <ItemGroup> is used within elements that can't contain a list.

    Again, the <ItemGroup> is the equivalent of an <ul> unordered list. For example, an <ul> can't be used within a <button> element (or any element with role=button). If that was a HTML <ul>, it would be invalid HTML. Even if it's a div with role=list it's still semantically invalid.

    One example of this is the 'Add background image' control in the global Styles > Background:

    Image

    In this case, there is a <button> element rendered by Dropdown, that contains an InspectorImagePreviewItem component which is an <ItemGroup> that contains span elements. In terms of equivalent HTML this would be:

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

    Step-by-step reproduction instructions

    • Go to Site Editor > Styles > Background
    • Inspect the source of the 'Add background image' control.
    • Observe it's a button element that contains an ItemGroup with role="list" which in turns contains only <span> elements.

    Screenshots, screen recording, code snippet

    No response

    Environment info

    No response

    Please confirm that you have searched existing issues in the repo.

    • Yes

    Please confirm that you have tested with all plugins deactivated except Gutenberg.

    • Yes

    Please confirm which theme type you used for testing.

    • Block
    • Classic
    • Hybrid (e.g. classic with theme.json)
    • Not sure
@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Nov 29, 2024
@afercia
Copy link
Contributor Author

afercia commented Nov 29, 2024

Cc @WordPress/gutenberg-components

@afercia
Copy link
Contributor Author

afercia commented Dec 2, 2024

One ore example if incorrect usage is the Image duotone filter control in the settings panel, where the ItemGroup is used only for visual purposes, to draw a grey border aroung the button:

<ItemGroup isBordered isSeparated>
<Button
__next40pxDefaultSize
{ ...toggleProps }
>
<LabeledColorIndicator
indicator={ duotone }
label={ __( 'Duotone' ) }
/>
</Button>
</ItemGroup>

Image

Again, in terms of equivalent HTML, this is the equivalent of an unordered list that contains a button:

<ul>
    <button type="button" ... />
<ul>

@afercia
Copy link
Contributor Author

afercia commented Dec 2, 2024

Noting that ItemGroup is a polymorphic component, as in: it accepts an as prpp to determine the HTML element it will use. There are cases where, for example, it's a <span> element. The role=list is still rendered unless it's explicitly set to ` different value.

One way to fix the semantics could be to pass a prop role="none" for all the cases where the ItemGroup is not used as equivalent of an unordered list. However, it appears there are many cases where ItemGroup is used only to draw a grey border around another component by the means of its isBordered prop.

@mirka
Copy link
Member

mirka commented Dec 2, 2024

Related: #64445 (We should keep these problems in mind when overhauling the component)

@afercia
Copy link
Contributor Author

afercia commented Dec 3, 2024

One more example: The 'Add background image' in the global styles > Background:

Image

When an image is not set yet:

  • ItemGroup is rendered as a span element with role="list"
  • it contains span elements, no list items
  • it's wrapped within a button element, so basically this is a button that contains a <ul> with no list items.

When an image is set:

  • ItemGroup is rendered as a button element with role="list"
  • it contains span elements, no list items

Again, this appears one more case where ItemGroup is only used for visual purposes, to draw a gray border around other elements. No care at all has been taken into looking at the actually rendered markup and semantics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants