docsifyjs / docsify

🃏 A magical documentation site generator.
https://docsify.js.org
MIT License
27.78k stars 5.68k forks source link

Feat: Prevent focus on hidden sidebar #2265

Closed jhildenbiddle closed 1 year ago

jhildenbiddle commented 1 year ago

Summary

See title and original issue (below).

Related issue, if any:

2225

What kind of change does this PR introduce?

Feature

For any code change,

Does this PR introduce a breaking change?

No

Tested in the following browsers:

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 2, 2023 4:03pm
jhildenbiddle commented 1 year ago

Nice! Can it be tested?

Technically speaking, yes we can test this. I'd prefer not to implement those tests with this PR for a few reasons:

  1. Elements that are hidden not being unable to receive focus has been in the spec and reliable across browsers for ages.
  2. The current sidebar implementation is kinda wonky and will require mores tests than if it was implemented properly. See https://github.com/docsifyjs/docsify/pull/2254#discussion_r1367938848 for details.
  3. Since there is talk of refactoring/updating our method of rending HTML elements, I was hoping to postpone writing additional tests until this work begins.

I'd like to avoid this simple fix turning into a "test all the sidebar things" rabbit hole since it is (IMHO) a pretty basic fix.