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(surveys): #23937 disables interactivity on templates, fixes click targets #24420

Merged
merged 7 commits into from
Aug 20, 2024

Conversation

ctrl-alter-ego
Copy link
Contributor

@ctrl-alter-ego ctrl-alter-ego commented Aug 16, 2024

Problem

Fixes the issue of the survey templates appearing interactive when they are being presented for selection only.

Changes

Sets pointer-events to none for the templates, and sets tabindex for each container. However, this didn't solve the problem of a user still being able to tab through the individual elements of the survey templates. This also varies depending on whether user's focus is 'active' in the top navbar or the main page body. (See Loom video below).

https://www.loom.com/share/e8c4fbe83c7248808c9859f3d19bc855

To solve this, have introduced the 'inert' attribute. Interactivity and screenreader functionality won't work for the individual template elements, which I believe is ok here as context is provided by title and description. Happy to revert this bit if necessary, although users will be able to tab through child elements again IF they start with the navbar (or anything other than the main body) as active.

Also noticed that the click target for each template included its title and description, but wasn't surrounded by a red focus border, which may be confusing for users trying to click outside of the templates, or copy/paste the text etc. (See this referenced in Loom above too).

Does this work well for both Cloud and self-hosted?

Shouldn't have an impact.

How did you test this code?

Checked manually in Chrome vs current prod. Checked popular browsers using Browserstack. Ran all frontend tests with 'pnpm test:unit'.

@Twixes Twixes requested a review from a team August 19, 2024 09:24
frontend/src/custom.d.ts Outdated Show resolved Hide resolved
@dmarticus dmarticus enabled auto-merge (squash) August 20, 2024 17:37
@dmarticus
Copy link
Contributor

@ctrl-alter-ego Thanks so much for your contribution! I've approved it and am waiting on CI to pass; will get it shipped as soon as it does :)

@dmarticus dmarticus merged commit 901bb7c into PostHog:master Aug 20, 2024
91 of 92 checks passed
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