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

Add tab index to .md-search__scrollwrap in the search plugin #7193

Closed
4 tasks done
jzazo opened this issue May 18, 2024 · 11 comments
Closed
4 tasks done

Add tab index to .md-search__scrollwrap in the search plugin #7193

jzazo opened this issue May 18, 2024 · 11 comments
Labels
working as intended Issue reports behavior in line with specification

Comments

@jzazo
Copy link
Contributor

jzazo commented May 18, 2024

Bug description

I am running the https://accessibilityinsights.io/ extension and I get flagged an error: scrollable-region-focusable: Elements with scrollable content must be accessible by keyboard.

This error is flagged when enabling the search plugin.

Path: .md-search__scrollwrap

Snippet:
- <div class="md-search__scrollwrap"> <div class="md-search-result" data-md-component="search-result">
- <div class="md-search-result__meta">Type to start searching</div> <ol class="md-search-result__list" role="presentation"></ol> </div> 

Fix ONE of the following:
- Element should have focusable content
- Element should be focusable

Adding a tabindex="0" to the div section <div class="md-search__scrollwrap"> removes the error.

Related links

Reproduction

doh.zip

Steps to reproduce

  1. Unzip and poetry install
  2. mkdocs serve
  3. Download extension and fast run: https://accessibilityinsights.io/

Browser

Edge

Before submitting

@squidfunk
Copy link
Owner

Thanks for reporting. The region is accessible by keyboard ☺️ Using the up and down arrows, you can navigate search results, so I'm quite confident this is a false positive.

@jzazo
Copy link
Contributor Author

jzazo commented May 18, 2024

Thanks for the feedback. I have created a follow up issue on the accessibility app:
microsoft/accessibility-insights-web#7338

I am happy to close this issue while we wait on the other project, and reopen if necessary, or leave open in the meantime. Let me know your preference, or close the issue yourself.

@squidfunk
Copy link
Owner

Thanks! Yes, I believe this is a false positive in our case, but I don't think that this can or should be solved upstream, because the accessibility checker would need to execute JavaScript to learn that the search results can be navigated.

Why did we implement it that way? Because we want to keep the cursor in the input field, so the user can change the search input at any time. Making the results focusable would mean the user would have to leave the input field, and return to it to change the query – IMHO a mediocre user experience compared to what we currently have. If we can add aria-* attributes that offer indications of our impementation (i.e. why it's not tabbable), we're happy to consider.

Also note that when you tab, you'll leave the search.

@squidfunk
Copy link
Owner

I'm closing this as working as intended, as explained in my last comment. In case we can improve the situation with additional aria-* attributes, please open a PR that add those attributes ✌️

@squidfunk squidfunk added working as intended Issue reports behavior in line with specification and removed needs input Issue needs further input by the reporter labels May 18, 2024
@jzazo
Copy link
Contributor Author

jzazo commented May 18, 2024

Do you think adding something like aria-hidden="true" would be acceptable? Could it remove the false positive, as it is being handled differently than the checker expects?

@squidfunk
Copy link
Owner

I don't think it's acceptable, because this would hide it from screen readers. If would also silence the warning, but at the possible expense of a worse experience on a screen reader. I wouldn't recommend it.

@jzazo
Copy link
Contributor Author

jzazo commented May 19, 2024

One thing I did not correctly understand (that you may have explained). Adding tabindex="0" to the div section removes the false positive. Correcting this issue manually, I don't think the experience changes. I can start typing, use the keyboard to select some line, and continue typing without losing focus. I am not sure if adding the expression I put, alters the behavior at all, but can solve for the false positive, don't you agree?

@squidfunk
Copy link
Owner

I'm not sure if this generalizes to all browsers. For instance, in my experience, Safari has completely different tabbing semantics than Chrome. Thus, I'm not entirely sure that this change is safe to make.

@jzazo
Copy link
Contributor Author

jzazo commented May 24, 2024

so I was able to test this solution on Firefox, Edge and Safari, and they all behaved the same when typing a command, using arrow keys to navigate the menu, and then continue typing, without losing focus. To test this I modified the div section to <div class="md-search__scrollwrap" tabindex="0">. So I don't think it broke the experience in any of those browsers.

@squidfunk
Copy link
Owner

Thanks for taking the time to test this. You can create a PR if you like.

@jzazo
Copy link
Contributor Author

jzazo commented Jun 2, 2024

Thanks @squidfunk, I created this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
working as intended Issue reports behavior in line with specification
2 participants