fix(SCS-70): structured-list with divider a11y issue

[evgenitsn]

SCS-70

What

1. Background - (Bug) - StructuredList with Divider has an a11y issue #709
2. What did you do - wrapped the Divider elements in list items
3. What does the reviewers should expect - no a11y errors even if the list has a role="list"

I have done:

• Written unit tests against changes
• Written functional tests against the component and/or NewsKit site
• Updated relevant documentation

I have tested manually:

• The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
• The screen reader reads and flows through the elements as expected.
• There are no new errors in the browser console coming from this PR.
• When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
• The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

[pp-serviceaccount]

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/scs-70-structured-list-dividers-a11y-issue
Storybook: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/scs-70-structured-list-dividers-a11y-issue/storybook

[mutebg]

if the parent has aria-hidden, the children hr, in that case, does not need it.
but looking into Divider I don't think is something we can support.
Have you checked if that's ok with axe and SR?

[evgenitsn]

Yes, we can't remove the aria-hidden from Divider in its current implementation.
I've added role="list" to an example for test and there were no errors on Axe and the screen reader is navigating smoothly through the list, no mentioning for dividers or any other unwanted elements.

[Vanals]

It looks good from Screen Reader point ov view. Just wondering.. HTML wise, why are we doing

<ul>
  <li> Item</li>
   <li>Divider</li>
   <li>Item</li>
   <li>Divier</li>
</ul>

and not

<ul>
  <li> 
     Item
     Divider
  </li>
  <li> 
     Item
     Divider
  </li>
</ul>

Semantically would be more correct IMO 🤔 and bring less HTML nodes in the DOM

[evgenitsn]

I think it would be hard to do it like this

 <li> 
     Item
     Divider
  </li>

because we get the items like this

const structuredListChildren = React.Children.toArray(
    children,
  ) as React.ReactElement<StructuredListItemProps>[];

so the children are in the hands of the users and we can't easily determine where to put the divider.

I get your point but it doesn't look like a straightforward thing to do and for the less DOM nodes point, isn't is the same, the Divider is still an hr element, the amount of nodes should be the same, the hr will just be nested?

[Vanals]

I guess you will have one less <li> element, as the <hr> would not need wrapping. But yeah, get it, not straightforward 👍

[Vanals]

Shall we add also role='list' in storybook scenarios? so we can capture this or other faulty scenarios.

[evgenitsn]

Shall we add also role='list' in storybook scenarios? so we can capture this or other faulty scenarios.

Don't know to be honest, the whole component has the role - list implicitly either way. So in theory no one should use role="list" in their usage. Also the StructuredList does not accept role prop according to the types. I think on render where I found this usage they were forcing it (ignoring the types).

Not sure why this a11y error is showing only after adding the role. If we want an example of that we would have to extend the StructuredList props to accept role. I am not sure if that makes sense.