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

fix(web,a11y): remove autofocus from input fields #8857

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

ben-basten
Copy link
Member

@ben-basten ben-basten commented Apr 16, 2024

Description

Remove all usages of the autofocus attribute, and replace them with alternatives where necessary.

Reasoning for this change:

Also in this PR:

The people filtering in the advanced search filters relied on this autofocus functionality. Previously, clicking the "See all people" button would expand the people menu, show the text input field, and automatically move focus to the text input field.

What I propose here is to make the text input field always visible. This provides the following benefits:

  • allows users to have the choice between browsing through an expanded list of people, or quickly filtering for the person they want
  • removes the need to jump focus, and potentially create a confusing experience for screen reader users
  • provides more visibility/predictability to the text filtering feature

Screenshots

n/a

How Has This Been Tested?

Scenario 1: changing name

  1. Go to the "people" page
  2. Choose a person
  3. Click the person name to start editing it. Focus should automatically be moved to the input field.

Scenario 2: filtering

  1. Open the advanced search filters
  2. Try filtering people with and without the "See all people" expanded

Checklist:

  • npm run lint (linting via ESLint)
  • npm run format (formatting via Prettier)
  • npm run check:svelte (Type checking via SvelteKit)
  • npm test (unit tests)

The autofocus attribute can cause the keyboard to unexpectedly appear
for mobile users, and override any other focus management that the
application is doing programatically.
@alextran1502 alextran1502 merged commit 1071396 into immich-app:main Apr 17, 2024
23 of 24 checks passed
@waclaw66
Copy link
Contributor

The autofocus feature is very useful on desktop devices, would it be possible to enable it on desktop only and disable it in mobile web browser?

@ben-basten ben-basten deleted the fix/autofocus branch April 17, 2024 12:25
@ben-basten
Copy link
Member Author

The autofocus feature is very useful on desktop devices, would it be possible to enable it on desktop only and disable it in mobile web browser?

@waclaw66 Yes, there are still scenarios where moving focus to input fields is needed! In those scenarios, I'd recommend doing it intentionally using focus() instead to prevent unexpected behavior down the road.

For example, see this change I made to the edit name field:

https://github.com/immich-app/immich/pull/8857/files#diff-c0c073edbfa07ce75111cbadfb1ab931d366e5926faf0148a683faed5ce7f5de

In my opinion, if it comes down to differentiating between desktop and mobile, a different design should be considered that works for both. For example, it could be cool if clicking a sidebar link automatically focuses the header of the section that it just opened, and that would have a similar outcome as autofocusing the album/people filtering input field previously did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants