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

ECHOES-568 Migration Fixes #257

Merged
merged 1 commit into from
Feb 3, 2025
Merged

ECHOES-568 Migration Fixes #257

merged 1 commit into from
Feb 3, 2025

Conversation

daniel-nagy
Copy link
Contributor

@daniel-nagy daniel-nagy commented Jan 31, 2025

ECHOES-568

There are a bunch of tests failing in the product because Mantine is adding the same label to the input element and option dropdown wrapper element.

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title Migration Fixes ECHOES-568 Migration Fixes Jan 31, 2025
Copy link

netlify bot commented Jan 31, 2025

Deploy Preview for echoes-react ready!

Name Link
🔨 Latest commit 8f7ff5e
🔍 Latest deploy log https://app.netlify.com/sites/echoes-react/deploys/679d354bcf498e00089aa5d8
😎 Deploy Preview https://deploy-preview-257--echoes-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

Comment on lines +176 to +184
label={label}
labelProps={{
// We no longer use Mantine's InputLabel component. However, if we
// do not pass a `label` prop to the Select component, Mantine will
// render an `aria-label` on the OptionsDropdown component. This
// causes the input and the dropdown to have the same label, which
// is problematic for accessibility.
labelElement: Null,
}}
Copy link
Member

@gregaubert gregaubert Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the DOM, I can see that the dropdown as an aria-labelledBy that now points to a nonexisting label id:
image

Should we actually add that id to our label? From what I can see on our previous implem this aria-labelledBy was correctly pointing to the mantine label, see:
image

Copy link
Contributor Author

@daniel-nagy daniel-nagy Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we did that we would have the same issue, that 2 elements have the same label (the input and the dropdown). I'm a little confused how the the dropdown component is even getting that label id 🤔. It's just adding the -label suffix to the id here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably inferring it from the parent element id we are passing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example on MDN, the input and dropdown have different labels. The input as an aria-controls that points to the id of the drop-down menu.

However, on https://echoes-react.netlify.app/?path=/story/echoes-select-select--default, the input's aria-controls does not match the id of the drop-down element, but in your screenshot it does? 🤔

Screenshot 2025-02-03 at 9 13 43 AM

What I can take away from this:

  1. The input should have an aria-controls that points to the id of the drop-down menu.
  2. The input and drop-down should have different labels.

Neither of these seem correct in Mantine on my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aria-controls changes once you open the drop-down menu. But it is incorrect until the drop-dowm menu opens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so given what you are saying, we want to keep the labelElement: Null to avoid having duplicate labels.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the aria-controls situation, this is weird. When looking at Mantine documentation, they actually don't provide the aria-controls when the dropdown isn't open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so given what you are saying, we want to keep the labelElement: Null to avoid having duplicate labels.

Yeah, I'm not seeing any way to provide a different label for the drop-down menu element in the source code. In that case do you think this change is acceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's acceptable

@daniel-nagy daniel-nagy merged commit 0b321c6 into form-components Feb 3, 2025
20 checks passed
@daniel-nagy daniel-nagy deleted the migration-fixes branch February 3, 2025 17:12
gregaubert pushed a commit that referenced this pull request Feb 4, 2025
gregaubert pushed a commit that referenced this pull request Feb 4, 2025
gregaubert pushed a commit that referenced this pull request Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants