-
Notifications
You must be signed in to change notification settings - Fork 15
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
ENH Better use of ARIA attributes #264
ENH Better use of ARIA attributes #264
Conversation
0efdfdd
to
0b84b86
Compare
@@ -183,14 +183,3 @@ test('dnd handler is not displayed if link field is not MultiLinkField', () => { | |||
</LinkFieldContext.Provider>); | |||
expect(container.querySelectorAll('.link-picker__drag-handle')).toHaveLength(0); | |||
}); | |||
|
|||
test('keydown on dnd handler', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this test because the aria-pressed attribute is now controlled by dndkit's attributes, not with custom code. In a jest context this fails though real-life it works fine
0b84b86
to
5a5e746
Compare
> | ||
{ (isMulti && !readonly && !disabled) && <div className="link-picker__drag-handle" | ||
tabIndex="0" | ||
role="button" | ||
aria-pressed="false" | ||
aria-pressed={attributes['aria-pressed'] || false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dndkit will set a value of undefined for aria-pressed when its not being sorted which is kind of odd and the attribute won't show at all, so I've given it a default value of false instead.
While that has been resolved, we now have the situation where there's nothing indicating for screen readers that pressing enter/space on the link element itself will open the edit modal. That's what the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work well locally but I do have one concern below and one above.
We don't to define |
052676e
to
4863774
Compare
Ahhh yup my bad I missed that button somehow 😅 |
LGTM but has a merge conflict |
4863774
to
288e75c
Compare
Done |
Last lot of conflicts. |
288e75c
to
ef75425
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, still works well in combination with the other merged PRs.
Issue #245
The fixes a few of things noted in the issue:
The Div containing the LinkField button has a role of button. The button inside there also has a the role of button.
Disabled LinkFIeld have aria-disabled="false" applied to them.
Core issue was that that the dndkit attributes were mistakenly on a div instead of child drag handle which is specifically there for accessibility
Regarding
aria-describedby is empty. It should point to the label.
- The drag handle should now have something likearia-describedby="DndDescribedBy-0"
which points to a hidden div that dndkit underneath the sortable links which will look like this