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

scroll: Set tabIndex to -1 for simplebar content wrapper. #30444

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

shubham-padia
Copy link
Member

@shubham-padia shubham-padia commented Jun 16, 2024

Fixes #30403.

Having tabIndex set to 0 led to keyboard focus being put on a scrollbar container, which led to users having to tab twice to skip a container. This commit also removes instances of tabIndex being set to -1 programatically for certain cases, since it is -1 by default now. This commit also removes outline: none for simplebar since that property is not needed anymore becuase the wrapper is not focusable anymore.

The option became configurable in Grsmto/simplebar#695

The second commit is technically large, but I think all those changes belong together. Removing the previous hacks and css along with adding the property in the same commit made more sense to me. Reviewing it should be manage-able considering most of it just adding the tab-index property. Let me know if you think it should be split up

Screenshots and screen captures:

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@zulipbot zulipbot added size: XL area: left sidebar (ui) Left sidebar buttons/interactions excluding popovers bug labels Jun 16, 2024
Copy link

sentry-io bot commented Jun 16, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: web/src/scroll_util.ts

Function Unhandled Issue
get_scroll_element TypeError: Cannot read properties of undefined (reading 'dataset') get_scroll_elemen...
Event Count: 2 Affected Users: 1

Did you find this useful? React with a 👍 or 👎

@shubham-padia shubham-padia marked this pull request as ready for review June 16, 2024 12:15
@shubham-padia shubham-padia changed the title Simplebar tabindex scroll: Set tabIndex to -1 for simplebar content wrapper. Jun 16, 2024
@timabbott timabbott requested a review from andersk June 16, 2024 15:07
@timabbott
Copy link
Member

@andersk can you review this one?

version.py Outdated Show resolved Hide resolved
We need this update to configure tabIndex for simplebar.
@shubham-padia shubham-padia added the integration review Added by maintainers when a PR may be ready for integration. label Jun 19, 2024
Fixes zulip#30403.
Having tabIndex set to 0 led to keyboard focus being put on
a scrollbar container, which led to users having to tab twice
to skip a container.
This commit also removes instances of tabIndex being set to
-1 programatically for certain cases, since it is -1 by default now.
This commit also removes `outline: none` for simplebar since
that property is not needed anymore because the wrapper is
not focusable anymore.
@timabbott timabbott merged commit 43eebbf into zulip:main Jun 20, 2024
14 checks passed
@timabbott
Copy link
Member

Merged, thanks @shubham-padia and @andersk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: left sidebar (ui) Left sidebar buttons/interactions excluding popovers bug integration review Added by maintainers when a PR may be ready for integration. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove tabIndex from simplebar scroll containers
4 participants