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

ensure all popovers have aria-describedBy #3186

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Conversation

peetucket
Copy link
Member

@peetucket peetucket commented Jul 5, 2023

Why was this change made? 🤔

Fixes one accessibility item in #3171

Note, after looking at https://inclusive-components.design/tooltips-toggletips/#inclusivetoggletips will leave this as aria-describedby tags, but have now moved the element they are attached to, in some cases to the form input. In some cases, the tip is describing a section, so left on the label or legend in those cases.

How was this change tested? 🤨

Localhost

Copy link
Contributor

@justinlittman justinlittman left a comment

Choose a reason for hiding this comment

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

Also, can you add comments to PopoverComponent describing the accessibility-correct way to use it.

@@ -2,7 +2,7 @@
<header>Details *</header>
<div class="mb-3 row">
<div class="col-sm-2">
<%= form.label :name, "Collection name *", class: "col-form-label" %>
<%= form.label :name, "Collection name *", class: "col-form-label", "aria-describedby": "popover-collection.name" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the aria-describedby be on the label or on the input?

Copy link
Member Author

Choose a reason for hiding this comment

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

good question - i was assuming the description was of the input, but i could be convinced it was the label too... we can verify with an expert

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is correct ... it is and should be the label, this is what is the tooltip is right next to and is what it is tied to

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but the label is tied to the input. So if the label is describing the input then isn't the tooltip also describing the input?

@ndushay ndushay added the accessibility accessibility improvements label Jul 6, 2023
@peetucket peetucket force-pushed the 3171-welcome-accessibility-2 branch from 9e0f2e8 to bb4a1c2 Compare July 6, 2023 21:33
@peetucket
Copy link
Member Author

Also, can you add comments to PopoverComponent describing the accessibility-correct way to use it.

I added a comment to the top of PopoverComponent explaining how to use it.

Copy link
Contributor

@justinlittman justinlittman left a comment

Choose a reason for hiding this comment

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

Approve and defer to you on determining whether popover should reference the label or input.

@peetucket peetucket force-pushed the 3171-welcome-accessibility-2 branch from 267bec3 to 8aa8e33 Compare July 10, 2023 19:25
@peetucket peetucket force-pushed the 3171-welcome-accessibility-2 branch from 8aa8e33 to 7ee7f02 Compare July 11, 2023 22:34
@peetucket
Copy link
Member Author

Note, after looking at https://inclusive-components.design/tooltips-toggletips/#inclusivetoggletips will leave this as is with aria-describedby tags, but have now moved the element they are attached to in some cases. Where it made sense, I moved to the form input being described. In some cases the tip is describing a section, so left on the label or legend in those cases.

@peetucket peetucket force-pushed the 3171-welcome-accessibility-2 branch from 09538eb to 130135a Compare July 17, 2023 22:07
@peetucket peetucket merged commit 619ba15 into main Jul 17, 2023
@peetucket peetucket deleted the 3171-welcome-accessibility-2 branch July 17, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility accessibility improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants