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

Ltd 4637 add aria labelledby to ecju textarea #1764

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

stuaxo
Copy link
Contributor

@stuaxo stuaxo commented Jan 31, 2024

Add aria-labelledby to the reason for closing textarea, this references the hint widget.

Added a unit test to verify this, as well as the existing aria-describedby attribute.

@stuaxo stuaxo requested review from hnryjmes and depsiatwal January 31, 2024 17:24
@stuaxo stuaxo force-pushed the LTD-4637-add-aria-labelledby-to-ecju-textarea branch from 6b2eadd to 2e5772d Compare January 31, 2024 17:26
Copy link
Contributor

@kevincarrogan kevincarrogan left a comment

Choose a reason for hiding this comment

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

This doesn't seem like the right fix to the problem to me personally.

If we are pointing at this element and calling it a label then it probably should just be the label.

Alternatively I think we could use aria-label but it seems very strange to have an element be both the aria label and description, which suggests to me we're getting something else wrong.

@stuaxo
Copy link
Contributor Author

stuaxo commented Feb 1, 2024

I need to check out the ARIA docs.

Was hoping for some of the feedback re:description vs label. The description, we get for free.

If we are pointing at this element and calling it a label then it probably should just be the label.

To clarify - to get here would removing the aria-describedby be enough ? It looks like there used to be a preference for labelledby vs label, but that was a while ago, e.g. https://stackoverflow.com/a/19616909 - then again I don't know how often people update their screenreaders.

@stuaxo
Copy link
Contributor Author

stuaxo commented Feb 1, 2024

Wrote that + realised you probably meant using the field label ?

@stuaxo
Copy link
Contributor Author

stuaxo commented Feb 1, 2024

In this case it looks like the label of the field is deliberately blank, so it might make sense using labelledby and removing the describedby.

@stuaxo
Copy link
Contributor Author

stuaxo commented Feb 1, 2024

I wonder if having the <label> but making it not visible would be screenreader friendly.

@kevincarrogan
Copy link
Contributor

What I'm saying though is, why isn't the help text just the label? We're literally saying it's a label via the aria element so why not just make it the label?

Copy link
Contributor

@hnryjmes hnryjmes left a comment

Choose a reason for hiding this comment

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

test coverage looks good. also I'm pretty sure that this solves the issue, as the help text is now associated with the textarea when looking using the Chrome DevTools Accessibility tool, and it wasn't before

@stuaxo
Copy link
Contributor Author

stuaxo commented Feb 1, 2024

@kevincarrogan making the help text a label, is the most straightforward thing. The default styling for labels, is to have black text + slightly different spacing, can we live with that or should we change the style.

On the side of living with it: these styles come from the govuk design system, we should leave them.

On the side of changing it to grey and changing the spacing: We aren't changing the design (though makes me wonder if the users would care) - might see what Colin thinks.

@stuaxo stuaxo force-pushed the LTD-4637-add-aria-labelledby-to-ecju-textarea branch 2 times, most recently from 510df51 to 1a68f62 Compare February 1, 2024 16:56
@stuaxo stuaxo requested a review from kevincarrogan February 2, 2024 15:32
@stuaxo stuaxo force-pushed the LTD-4637-add-aria-labelledby-to-ecju-textarea branch from 1a68f62 to 945e7fe Compare February 6, 2024 10:51
Copy link

codecov bot commented Feb 6, 2024

Codecov upload limit reached ⚠️

This org is currently on the free Basic Plan; which includes 250 free private repo uploads each rolling month. This limit has been reached and additional reports cannot be generated. For unlimited uploads, upgrade to our pro plan.

Do you have questions or need help? Connect with our sales team today at [email protected]

@stuaxo stuaxo merged commit c88e48f into dev Feb 6, 2024
14 checks passed
@stuaxo stuaxo deleted the LTD-4637-add-aria-labelledby-to-ecju-textarea branch February 6, 2024 11:25
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.

3 participants