Adding keyboard navigation and screen reader support for Issue #182 #259
Conversation
|
|
||
| // Modal accessibility: handle modal state | ||
| if (this._modalOpen) { | ||
| this.setAttribute('aria-modal', 'true'); |
There was a problem hiding this comment.
Can these be directly set on the bc-modal component?
There was a problem hiding this comment.
They were removed with the other aria attributes to focus this pull request solely on keyboard navigation
| } | ||
|
|
||
| // Trap focus when modal is open | ||
| protected _trapFocusInModal() { |
There was a problem hiding this comment.
Can this also be moved to the modal component? (if it is here, the code is applied to all components)
There was a problem hiding this comment.
The focus trapping was removed as it doesn't work as expected with the way modals currently work
src/components/bc-button.ts
Outdated
| @@ -75,6 +81,14 @@ export class Button extends withTwind()(BitcoinConnectElement) { | |||
| private _onClick() { | |||
| launchModal(); | |||
| } | |||
|
|
|||
| // Handle keyboard events for accessibility (Enter/Space key triggers click) | |||
| public override _handleKeydown(event: KeyboardEvent) { | |||
There was a problem hiding this comment.
I am worried about using a key listener, why cannot this be tabbed to and then enter pressed like normal buttons?
There was a problem hiding this comment.
Removed as the included bci-button is in fact a button element and receives the events normally
|
@chebizarro thank you for the PR! I think this is a huge change and we need to split it up, and focus on one thing per PR and make sure everything works. Currently I think the most important thing is standard keyboard accessibility as normal keyboard power-users would find this useful. I ran your PR and opened the dev mode ( To more easily test this, I would suggest we add a new html file just with one button so we can easily test there.
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Vite + Lit + TS</title>
<style>
body {
font-family: Arial, Helvetica, sans-serif;
margin: 0px;
@media (prefers-color-scheme: dark) {
background-color: black;
color: white;
}
}
.container {
padding: 20px;
max-width: 448px;
}
</style>
<script type="module">
import '../../src/index.ts';
</script>
</head>
<body>
<div class="container">
<bc-button></bc-button>
</div>
</body>
</html>If I load the page and press tab once this is what I see (incorrect item focused):
If I then open the modal I would expect the first connector to be focused by default. And if I tab forward once, it should select the next connector. If I tab backwards once I would expect it to go to the Do you think we could address this as a first step and maybe open a new PR for this alone? Once I connect I think it's strange that it focuses on the currency by default. I guess this would not be such an issue if #100 is implemented (actually, I will see if I can get a bounty created for this issue too). |
chebizarro
force-pushed
the
issue-#182
branch
3 times, most recently
from
f98dd23 to
508a76e
Compare
chebizarro
force-pushed
the
issue-#182
branch
from
5c19ff4 to
dc37c69
Compare
|
I refactored the branch and removed the |
|
Hi @rolznz, just wanted to bump this PR and see where you wanted to go with this? |
|
Hi @chebizarro the focusing is a lot better! I still found some issues:
|
This pull request aims to address issue #182 by improving accessibility for users by enabling keyboard navigation and screen reader feedback.
Also fixes #170
The majority of the changes are simple additions of
ariatags to the rendered html as well as extra handlers for keyboard navigation.These changes will need more thorough testing as there is no simple way to test each component in isolation that I can see.