fix(Fieldset): Apply name and description correctly

[TomasEng]

Summary

When trying to update to the latest version of The Design System, I noticed that the fieldset description becomes a part of the element's name in the accessibility tree. This breaks several tests within Altinn Studio.

---

The images below shows how Chrome interprets it:

---

This is how I expect the browser to interpret it:

---

As it turns out, this happens because of code that only executes when a <ds-field> element is used. Consequently, the tests of the React Fieldset component run green because there is no <ds-field> in the test component. In this pull request, I have fixed this test and added some new ones to make sure this works as expected. Then I have fixed the code that causes the bug and split it up into smaller parts to make it more maintainable.

Note

I have tried to make every check pass, but I am not sure how to fix the preview deploy job. I believe it is out of my control since it fails on "az login". I believe this is also the reason why other checks are being skipped. I have run relevant automated tests locally.

Checks

• I have read the contribution guidelines
• I have added a changeset (run pnpm changeset if relevant)

[changeset-bot]

🦋 Changeset detected

Latest commit: f8aba42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@digdir/designsystemet-web	Patch
@digdir/designsystemet-react	Patch
@digdir/designsystemet	Patch
@digdir/designsystemet-css	Patch
@digdir/designsystemet-types	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[eirikbacker]

Is this tested with all screen readers? The current implementation (connecting everything with aria-labelledby) is inpired by Aksel, and is used so that both label and description is announced when screen reader-focus enter the fieldset element. Unsure if aria-describedby is announced when screen reader-focus enters the fieldset in all screen readers?

[TomasEng]

I am testing with the screen reader that is built-in in Windows. I see that in the case you describe, it announces the legend only. I am not familiar with what screen reader users expect, but if the description should be announced as well, this sounds to me like a bug in the screen reader software. Applying ARIA attributes incorrectly just to make this work does not appear quite right. As a consequence, we cannot rely on the automated testing tools we use to ensure that our solutions are accessible. This concerns The Design System's own tests as well; the fieldset test named has correct legend and description passes only because there is no <ds-field> within the test component, so this mutation is not applied.

[eirikbacker]

Hehe, yeah, there are many issues with screen reader software, or rather - the dance between browser, OS accessibility API and the assistive technology. Many moving parts, and lots of software that gets less attention then i.e. other "cooler" web APIs. The whole https://u-elements.github.io/u-elements/ projects is an example of something created to close the gap between spec, and what actually works for the end user 😌 We also use aria-describedby instead of aria-errormessage in Designsystemet, to ensure all users gets the error announced. Using "the correct" attribute is nice, but hinders usability still.

This makes the Designsystemet proejct extra important - this type of testing should not be duplicated and implemented differently across many companies ☺️

Oh well, you say "we cannot rely on the automated testing tools " - could you elaborate, does your test tool say the current fieldset implementation is inncorrect? ☺️

Very good spotted the issue with has correct legend and description test in Designsystemet 🫶

[TomasEng]

That's an interesting point of view! As a consumer of The Design System, I appreciate that you take these challenges into account.

The problem is that tests like these will fail:

  it('Applies the legend to the fieldset', async () => {
    renderFieldset();
    await expect.element(page.getByRole('group')).toHaveAccessibleName(legend);
  });

  it('Applies the description to the fieldset', async () => {
    renderFieldset();
    await expect
      .element(page.getByRole('group'))
      .toHaveAccessibleDescription(description);
  });

(See the whole file for reference.)

These fail in both Vitest, Jest and Playwright (the example is Vitest, but all these tools allow similar test scripts), so it's not limited to one single tool. The same happens when using the name option in getByRole. So, how can the developer know that there is nothing wrong with the code?

[eirikbacker]

:) I think the Designsystemet tests should be checking

it('Applies the legend to the fieldset', async () => {
  renderFieldset();
  // Description is added in `aria-label` to ensure
  // announcement when entering fieldset with screen reader
  await expect.element(page.getByRole('group')).toHaveAccessibleName(`${legend} ${description}`);
});

...as this is the intended rendering?

You write "This breaks several tests within Altinn Studio." - does this mean Altinn Studio is testing i Designsystemet is doing its job correctly? Or are you relying on accessible names to identify elements? (in that case, should you use a data-testid instead as the accessible name is something Designsystemet should be able to adjust based on screen reader / OS / browser support?) :)

[TomasEng]

Will we? The tests are there to make sure everything works as expected, thus also ensuring a good user experience. But we can't use automated tests if we can't trust them.

Say we keep the code as-is. In Altinn Studio, we may surely create workarounds for the tests in question, now that we know why they behave like they do. But what about other consumers that use these tools to test their code? They will need to waste time debugging, only to find out that the issue is not within their own code. Or maybe they will just remove the tests, allowing bugs to pass through unnoticed.

Of course, the best would be to find a solution that solves both problems. Unfortunately I am not familiar with how screen readers interpret the code, but maybe there are other ways to make them announce the expected content?

In any case, I would be cautious about discouraging consumers from using conventional, user-centered test patterns.

[eirikbacker]

No, these tests are to make sure aria attributes are added 😌 Unfortunately, this is not 1:1 with a good user experience. I suggest the Digdir team enters this discussion ☺️

[TomasEng]

No, these tests are to make sure aria attributes are added

I'm speaking about tests in general. In order to make sure that everything we make in Altinn Studio works as expected, we must be able to trust what the tests are telling us.

[TomasEng]

By the way, I find it peculiar that the ds-field element manipulates the DOM of other elements in the document. This is fragile, as it is difficult to debug, and it may conflict with things that the consumer is controlling. Have you considered solving this in a way that preserves encapsulation, like creating a web component that acts as a fieldset?

[eirikbacker]

Very good point and we absolutely agree! 😌 We have considered this back and forth, but landed on enhancing fieldset since fieldset has a lot of built in functionality that we would need to re-implement, and some of it (for instance that disabled is automatically inherited by inputs inside fieldset) would be a huge performance hit.

That aside, we are currently doing more testing on aria-describedby and seems current support makes an easier solution possible 🎉 ☺️ We'll get back to you, and thanks for good reporting and headsup 🤗