oddbird / css-toggles

CSS Toggles demo and polyfill
https://toggles.oddbird.net
BSD 3-Clause "New" or "Revised" License
59 stars 2 forks source link

Improve the behavior of toggle-visibility #1

Closed mirisuzanne closed 2 years ago

mirisuzanne commented 2 years ago

On line https://github.com/oddbird/css-toggles/blob/main/index.js#L119 we are setting the css visibility property to either hidden or visible, which doesn't quite do what we need, since the element boxes still render as part of the layout.

The simplest way to avoid rendering the box would be to use display:none instead. That would give the expected layout behavior, and might be a good temporary fix. For the sake of a11y, we really want something more like content-visibility: auto, but in this case:

I think we could achieve something close to that by making data-toggle-visibility work more like a visually-hidden but focusable element, eg:

[data-toggle-visibility='hidden']:not(:focus):not(:focus-within) {
  position: absolute !important;
  width: 1px !important;
  height: 1px !important;
  padding: 0 !important;
  margin: -1px !important; // Fix for https://github.com/twbs/bootstrap/issues/25686
  overflow: hidden !important;
  clip: rect(0, 0, 0, 0) !important;
  white-space: nowrap !important;
  border: 0 !important;
}

It might make sense for the focus-management/link-target management to live in JS, since that should also trigger a change in the toggle state - but this approach to hiding content will (mostly) eliminate the box, without removing elements from the dom.

jerivas commented 2 years ago

@mirisuzanne let me know what you think of 277b49695d5432948e773b2784eb09fd4a9b380f

mirisuzanne commented 2 years ago

Nice! This is great for the demo.

If we want to take it farther, the next step might be actually updating the toggle state if the content becomes visible because of focus, in-page search, or linking? But I'm not sure that needs to be a high priority at this point?

jerivas commented 2 years ago

Right now it seems like keyboard users are unable to skip hidden content because it always opens up if it has focusable elements inside. I think we should follow what <details> does: content remains unfocusable while tabbing until the element is opened, but <details> opens automatically on search and anchor-linking. What do you think @mirisuzanne?

mirisuzanne commented 2 years ago

Yeah, that probably makes sense. Is that something we could do with e.g. inert?

jerivas commented 2 years ago

It looks like inert will also prevent searching (docs):

The browser may also ignore page search and text selection in the element.

I'm surprised browsers still don't support it though, I remember hearing about it for a couple years now

jerivas commented 2 years ago

Did a bit of research on this and it looks like there's no way of knowing (in JS) when a user is searching for something on a page. We could try to be clever and do some sort of Ctrl+F detection but that seems out of the scope of this project and very fragile. On the other hand, with a couple of event handlers we could detect anchor-linking and expand toggles based on that.

For now my proposal is to add visibility: hidden to the injected styles to more closely match the behavior of <details>.

mirisuzanne commented 2 years ago

I think our default, if we don't have a way of adding additional magic/a11y features around it, should be display:none. I think that could replace what we have entirely, if the current approach is wrong.

I'm not sure what advantage we get from visibility:hidden. As far as I understand, they have the same a11y implications (removing elements from the a11y tree), but display:none also removes the box from the page -- which we want.

jerivas commented 2 years ago

You're right, display: none is the way to go here. I left it as an injected stylesheet instead of inlining it for authors to override it with more specific selectors if required. See 718d364b3dbbba4f6e006e7b5e5e9794592841cf