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

a11y improvements for template to-do list #1207

Merged
merged 6 commits into from
Apr 28, 2021

Conversation

geoffrich
Copy link
Member

@geoffrich geoffrich commented Apr 24, 2021

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

This PR improves the accessibility of the to-do list in the starter template.

  • Add label to to-do list input
  • Add label to to-do list edit field
  • Show the save button when focused. Before this change, the save button would be focused while navigating with the keyboard but would not be visible
  • Make delete button focus state the same as hover state. Before this change, the delete button was hard to see when focused.
  • Programmatically manage focus on delete. Since the focused element is removed from the DOM, we should explicitly focus an element since browsers can be inconsistent. I chose to focus the heading at the top of the list. EDIT: Removed this, it was adding too much complexity for not enough benefit.

@Rich-Harris
Copy link
Member

Thank you! Can you explain the rationale for focusing the h1 after deleting a todo? I would have thought it was better to keep the focus in the same part of the document, so you can easily tab/shift-tab to the next/previous item

@pngwn
Copy link
Member

pngwn commented Apr 24, 2021

I think we should probably be focusing the next item in the list (the item that follows the deleted item).

@geoffrich
Copy link
Member Author

Thanks for the feedback--I originally focused the h1 because it was simplest. But I agree that it makes sense to keep focus in the same part of the document. I updated the logic to focus the next edit input if it exists, or the previous edit input if it's the last todo in the list, or the "add a todo" field if there are no todos remaining. I used bind:this in the each block so I could easily reference the next/previous input, not sure if there's a more idiomatic way.

I also had to wait a tick in the handleFocus function because the array of todo inputs wasn't updated right away in some cases and focus was being moved to an element that didn't exist.

@Rich-Harris
Copy link
Member

This is adding a ton of complexity to something that is supposed to be an easy-to-follow demo. Isn't the current focusing behaviour okay? After deleting a todo, Tab will focus the toggle button in the next todo, Shift-Tab will focus the delete button in the previous todo, which is what I'd expect. Automatically focusing a text input that I hadn't tabbed to or otherwise selected feels surprising.

Could we just keep the aria-label additions and style changes and get rid of the focus stuff? Unless there's authoritative guidance (from WAI-ARIA or similar) that confirms focusing an adjacent text input is the correct behaviour. Thanks!

@geoffrich
Copy link
Member Author

That's fair -- I've removed the focus management behavior from the PR. It was definitely getting a little complex for a demo.

@Rich-Harris Rich-Harris merged commit c44f231 into sveltejs:master Apr 28, 2021
@Rich-Harris
Copy link
Member

Thank you!

@geoffrich geoffrich deleted the todo-a11y branch April 28, 2021 21:45
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