themesberg / flowbite

Open-source UI component library and front-end development framework based on Tailwind CSS
https://flowbite.com
MIT License
8k stars 747 forks source link

Dont set aria hidden on large viewports #945

Open koddsson opened 3 months ago

koddsson commented 3 months ago

Hello 👋🏻

We're hitting this snag in our app where the sidebar is set with [aria-hidden="true"] when initialized. Apart from being a pretty serious accessibility issue it's also erroring in some cases for us in Chrome with the following in error:

A Chrome error that reads: 'Blocked aria-hidden on a <a>
about: srcdoc: 1
element because the element that just received focus must not be hidden from assistive technology users. Avoid using aria-hidden on a focused element or its ancestor. Consider using the inert attribute instead, which will also prevent focus. For more details, see the aria-hidden section of the WAI-ARIA specification at
https://w3c.github.io/aria/#aria-hidden.
• <a href="#" class="flex items-center p-2 tex t-base font-medium text-gray-900 rounded-lg dark: text-white hover:bg-gray-100 dark:hove
r: bg-gray-700 group">
</a>'

It looks like the sidebar is always initialized as hidden which does make sense in the context of a smaller viewport as it should render hidden and then toggled on and off. It makes less sense on larger viewports where we expect the sidebar to be rendered initially and then optionally users should be able to hide it.

This PR checks to see if we are rendering initially on a small viewport (640px or less) and sets the visibility state accordingly. I also made sure to read the CSS variable --small-viewport so that this size can be changed. I'm not sure if that's the way we'd like to do this. I thought there might be a specific Tailwind variable but didn't find one. We could also read this value from a data attribute if that makes more sense? It would mean that we'd be less likely to clash with a CSS variable set by apps.

Let me know what you think :)

Lexachoc commented 3 months ago

Here is the related issue with reproducible code and videos showing the exact issue.

https://github.com/themesberg/flowbite/issues/943#issuecomment-2274967284 https://github.com/themesberg/flowbite/issues/943#issuecomment-2278123167 https://github.com/themesberg/flowbite/issues/943#issuecomment-2278138167

zoltanszogyenyi commented 3 months ago

Hey @koddsson,

Thanks for the PR! Why do we set the visible variable to something else other than false?

That sets the visibility to false by default, regardless of smaller or larger devices because the visibility functionality should be the same. From what I understand we have the issue with the aria hidden attribute, so why not add the mobile/desktop device logic only to that?

This could very well break a lot of the base functionality of the drawer, I'll also check it locally in a few days.

Thanks, Zoltan

zoltanszogyenyi commented 3 months ago

Also, I think the main issue here is not the usage of aria hidden on larger devices, but rather the possibility to focus or not focus elements within it, so what we should actually do is remove the focus ability for the inside elements when the drawer is actually hidden, both on desktop and mobile devices :)

koddsson commented 1 month ago

This could very well break a lot of the base functionality of the drawer, I'll also check it locally in a few days.

Any news on this? We've been running this patch in production since I opened this PR and haven't had any problems.

Thanks for the PR! Why do we set the visible variable to something else other than false?

In this PR? I'm setting _visible to match the functionality of the component. When the component is initialized on a small viewport it's initially hidden and can be then toggled on and off.

Large viewport Small viewport
image image

You can see this issue in action on https://flowbite.com/docs/components/sidebar/#default-sidebar.

https://github.com/user-attachments/assets/34227579-f88e-49ff-9279-701c48b9c54d

If you look at the HTML of that demo you can see where the sidebar is marked with aria-hidden=true while it's clearly not hidden at all.

Screenshot 2024-09-30 at 09 00 48