2722 popover menu toggle variant menu items

[GCHQ-Developer-299]

Summary of the changes

• Refactor ic-menu-item to remove redundant <li> element
• Re-enable .checkA11y() calls in IcPopoverMenu.cy.tsx, and add an exception to the one that failed for 'nested-interactive' rule

This pull request doesn't resolve the accessibility failure reported by Accessibility Insights, my reasoning being that when manually testing with NVDA and Voiceover, the toggle variant menu items read out fine.

Related issue

#2722

Checklist

Accessibility

• [x] Accessibility Insights FastPass performed.
• [x] A11y unit test added and yields no issues.
• [x] A11y plug-in on Storybook yields no issues.
• [x] Manual screen reader testing performed using NVDA and VoiceOver.
• [x] Manual keyboard testing for keyboard controls and logical focus order.
• [x] Correct roles used and ARIA attributes used correctly where required.

[github-actions[bot]]

View your branch deployment here: https://mi6.github.io/ic-ui-kit/branches/2722-popover-menu-toggle-variant-menu-items/web-components View your React branch deployment here: https://mi6.github.io/ic-ui-kit/branches/2722-popover-menu-toggle-variant-menu-items/react

[GCHQ-Developer-530]

Does the <ul> in popover menu need removing as well if we're removing the <li> from it's menu items?

[GCHQ-Developer-299]

Does the <ul> in popover menu need removing as well if we're removing the <li> from it's menu items?

@GCHQ-Developer-530 the <ul> shouldn't be removed as it's got the 'role="menu"' that's the parent to the 'menuitems' nested inside. It's a good question and I will try out swapping it for a <div> or something though (since we're relying on aria roles, we might not need the semantic properties of a <ul>

[GCHQ-Developer-299]

Does the <ul> in popover menu need removing as well if we're removing the <li> from it's menu items?

@GCHQ-Developer-530 the <ul> shouldn't be removed as it's got the 'role="menu"' that's the parent to the 'menuitems' nested inside. It's a good question and I will try out swapping it for a <div> or something though (since we're relying on aria roles, we might not need the semantic properties of a <ul>

@GCHQ-Developer-530 have swapped the <ul> for a div (and updated PR description) - while it doesn't seem to affect SR interaction thanks to aria-roles, it is cleaner to read and removed a couple lines of CSS.

[GCHQ-Developer-299]

I'm not sure whether it's a big deal but most menu examples I've come across seem to use the ul and li structure we had previously. This solution looks like it will be functionally fine as you're still using the menu and menu item roles but I just wanted to flag that it seems more common to get around this sort of issue by making the problematic list items presentational (as in role=presentation). I think this would also fix the accessibility insights failure.

@GCHQ-Developer-718 i've had a go at using role="presentation" but it doesn't resolve the issue. According to this github issue conversation there's no way to prevent the 'button' element inside 'ic-button' registering as an interactive element. I'm more confused as to why all the other menu-items aren't triggering this issue, and why it's only the checkbox one.

I don't think, without removing ic-button, we can remove this reported error.

[GCHQ-Developer-718]

I'm not sure whether it's a big deal but most menu examples I've come across seem to use the ul and li structure we had previously. This solution looks like it will be functionally fine as you're still using the menu and menu item roles but I just wanted to flag that it seems more common to get around this sort of issue by making the problematic list items presentational (as in role=presentation). I think this would also fix the accessibility insights failure.

@GCHQ-Developer-718 i've had a go at using role="presentation" but it doesn't resolve the issue. According to this github issue conversation there's no way to prevent the 'button' element inside 'ic-button' registering as an interactive element. I'm more confused as to why all the other menu-items aren't triggering this issue, and why it's only the checkbox one.

I don't think, without removing ic-button, we can remove this reported error.

Makes sense, I've approved. :)