saadeghi / daisyui

🌼 🌼 🌼 🌼 🌼  The most popular, free and open-source Tailwind CSS component library
https://daisyui.com
MIT License
33.43k stars 1.28k forks source link

bug: <dialog> element items are focusable, even when <dialog> element is closed #2223

Open viktorb1 opened 1 year ago

viktorb1 commented 1 year ago

What version of daisyUI are you using?

3.5.1

Describe your issue

When you use a dialog modal such as https://daisyui.com/components/modal/#dialog-modal, everything is still tabbable even when the modal is not open. The problem seems to be caused by display: grid preventing the default behavior of the

element from being set to display: none.

What browsers are you seeing the problem on?

Chrome

Reproduction URL

Your website demos, for example

saadeghi commented 1 year ago

Thank you for reporting.
Unfortunately removing the grid (and adding it only when modal is open) disables the open animation.
I don't know if there's a way to fix that yet, but I don't want fixing one issue to create another.

viktorb1 commented 1 year ago

In my project, I dynamically add/remove the 'modal' class when the

element opens/closes and it works as expected. Decent workaround for the time being if anyone else runs into this problem.

dogagenc commented 1 year ago

@saadeghi This is being a problem both for general keyboard usage and accessibility. Is there a reason you don't want to use visibility: hidden and visibility: visible for close/open states? - which seems it's being used in unstyled components -

It shouldn't affect animation.

P.S.: I can open a PR with visibility props added if you think it's the way.

velut commented 11 months ago

A quick solution (sacrificing the open animation) to restore the default display:none behavior seems to be using open:modal instead of modal as the dialog class.

<dialog id="my_modal_1" class="open:modal">
  <!-- Change the class here   ^^^^^^^^^^ -->
</dialog>
cknight commented 10 months ago

In my project, I dynamically add/remove the 'modal' class when the element opens/closes and it works as expected. Decent workaround for the time being if anyone else runs into this problem.

I've just run into this and your workaround is good, but doesn't work if the user presses escape to close the dialog. In this case, the dialog closes, but still contains the modal class. Did you find a workaround for that?

viktorb1 commented 10 months ago

@cknight It's been a while since I've had this issue, but what is stopping you from writing a keypress event handler in javascript to remove the class?

oyvindlille commented 7 months ago

Any progress on this? This is in conflict of multiple wcag critera like 2.4.3. My fix was to use open:modal, but suggest this to be fixed on the class .modal or at least be mentioned in the documentation.

Atmos4 commented 6 months ago

Achieving this in pure CSS is from my experience very hard.

Adding visiblity:hidden to closed modal styles would disable the closing transition.

There is a new transition-behavior option for CSS transitions called allow-discrete. It basically allows to play transitions before setting visibility:hidden. Browser support is not very good though.

The solution I have thus been using with DaisyUI is to add visibility:hidden manually after the modal is being closed. This can be for example achieved by listening to the transitionend event, or some other delayed style manipulation with Javascript.

Here is a Codepen to show what I mean (no tailwind playground because they don't allow script tags πŸ™ƒ)

YouSafe commented 5 months ago

I ran into this too when I noticed that on Android Chrome the chess pieces that are behind my modal's textarea can not be dragged. I'm using lichess' chessground chess board, which checks if the TouchEvent is cancelable (see) and for the area behind the textarea the value in the event is false and everywhere else on the board true.

Adding the invisible class to the closed dialog has resolved this issue for me.

Nikhil1920 commented 4 months ago

There should be a note in the docs regarding this

Having better keyboard accessibility might be more important than opening animation for some people

acaloiaro commented 2 months ago

An additional side effect of display: grid on the modal class is that intersection observance (https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API) on elements within <dialog class="modal"> is not possible because everything within the modal will have intersected the viewport immediately upon page load.

In other words, it's not possible to lazy load content in modals if the content will be visible when the modal is visible.

acaloiaro commented 2 months ago

@Atmos4 could you clarify what about my comment you're thumbing down?

I was under the impression that I was making an objective comment about an observation. If there's something about it that's wrong, I'd appreciate a correction, and other readers of the thread will benefit from it.

Atmos4 commented 2 months ago

That is true, apologies. The reason I thumbed down is because I do not believe an intersection observer is the right way to load the content of a modal. If you wish to lazy-load a modal content, you should do either when it turns visible or right after it is loaded on the page.

Trying to use intersection observers with modals is generally a bad idea and you will face the problem you are having with many modal implementations out there πŸ™‚

But the second reason I thumbed down is because I believe your comment to be slightly off-topic and unrelated to the focus issue

acaloiaro commented 2 months ago

Thanks for the clarification.

I don't fully agree with your opinion, although I'll concede there's a chance I'm in the minority. I'm curious to see where this thread lands on the topic.

With that said, the <dialog> element, HTML's native modal element, has display: none in the unopened state (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#animating_dialogs). That means using intersection observance is a great way to lazy load content within dialogs. You did say modals more generally, and I'm specifically talking about <dialog>. I'd agree that using intersection observance in conjunction with many CSS packages' modal class is probably not a good idea because every package has different priorities, and thus implementation details to consider. That doesn't make intersection observance within dialogs a bad idea on its own. As daisyUI has obviated in this instance -- some packages are willing to prioritize package-specific goals like animations over the problems some of us have faced with daisyUI's modal grid display decision. That's what makes using intersection observance a potentially bad idea in daisyUI modals.

I'll argue along with @Nikhil1920 that as long as daisyUI prioritizes animations over other things, a lot of developers will appreciate a caveat in the modal docs. DaisyUI/maintainers are entitled to prioritize whatever achieves the best outcome for them, I'll only argue that the footgun should be called out in the docs.