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

ENH Better use of ARIA attributes #264

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Mar 28, 2024

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.
  • A DOM manipulation being done in react

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 like aria-describedby="DndDescribedBy-0" which points to a hidden div that dndkit underneath the sortable links which will look like this

<div id="DndDescribedBy-0" style="display: none;">
  To pick up a draggable item, press the space bar.
  While dragging, use the arrow keys to move the item.
  Press space again to drop the item in its new position, or press escape to cancel.
</div>

@@ -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 () => {
Copy link
Member Author

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

>
{ (isMulti && !readonly && !disabled) && <div className="link-picker__drag-handle"
tabIndex="0"
role="button"
aria-pressed="false"
aria-pressed={attributes['aria-pressed'] || false}
Copy link
Member Author

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.

@GuySartorelli
Copy link
Member

The Div containing the LinkField button has a role of button. The button inside there also has a the role of button.

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 aria-role="button" was there for.

Copy link
Member

@GuySartorelli GuySartorelli left a 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.

@emteknetnz
Copy link
Member Author

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 aria-role="button" was there for.

We don't to define role="button" because for the link itself there's an html <button> which is bound to the onclick handler

@emteknetnz emteknetnz force-pushed the pulls/4/a-aria branch 2 times, most recently from 052676e to 4863774 Compare April 2, 2024 23:19
@GuySartorelli
Copy link
Member

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 aria-role="button" was there for.

We don't to define role="button" because for the link itself there's an html <button> which is bound to the onclick handler

Ahhh yup my bad I missed that button somehow 😅

@GuySartorelli
Copy link
Member

LGTM but has a merge conflict

@emteknetnz
Copy link
Member Author

Done

@GuySartorelli
Copy link
Member

Last lot of conflicts.

Copy link
Member

@GuySartorelli GuySartorelli left a 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.

@GuySartorelli GuySartorelli merged commit 976781d into silverstripe:4 Apr 4, 2024
12 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4/a-aria branch April 4, 2024 20:44
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.

2 participants