-
Notifications
You must be signed in to change notification settings - Fork 845
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
[4.x] Change Inertia Modal.vue component to use a native <dialog> #1431
Conversation
Thanks! I've been meaning to do this. Marking as draft while I review. |
I've tested this out locally, and it works great! I've confirmed that the teleport is not required to break out of any I was initially confused by the Two things:
|
Do you guys like comments on things? I like to put comments for things like the setTimeout which seem weird to explain why something is being done. Is that good to include in things like this? I can do the Livewire side as well. |
Alright, I haven't done Livewire stuff in a long time, but take a look at the update and let me know if that works. There's probably a nicer way to separate out that watch function than doing it all inline. |
I don't mind comments explaining things that aren't obvious.
Tested in Firefox and once the modal is closed, the page becomes unresponsive to clicks. I thought it might be something to do with |
It doesn't look like this is a firefox issue, as I can reproduce it in Chrome as well. I did some testing on this and noticed a difference in behavior between two buttons on the profile page. If you click the Two Factor Authentication -> Enable button and then click the background or press escape, everything is fine. If you click the cancel button you get the unresponsive behavior. Interestingly, this is not an issue for the "Log Out Other Browser Sessions" button and then click cancel on the modal which opens up there. It looks like this is related to what is happening with the function call with the cancel button of the 2FA modal. When you click that it's calling the Clicking the background or pressing escape in the 2FA modal also does not trigger This is a Livewire behavior and not something I'm going to be good at diagnosing, I don't think, but hopefully this information can help. |
Hey @Smef, I can't figure this one out either. It's like the If I manually get the Also, if I delete the All I can think is that it's a conflict with Livewire's morphdom. A solution might be to keep the modal state purely client-side, but the code for this partially exists in the package and partially in the application code, so it would be a major change. I'd suggest removing the Livewire changes and then we can see what Taylor thinks of just applying this improvement to the Vue version. I definitely think it's a worthwhile improvement and have had others asking for it. |
I've reverted the Livewire implementation |
If I had to guess what was happening with this, it would be this:
|
…and disappearing, preventing modal size shifting
} | ||
}); | ||
|
||
const close = () => { | ||
if (props.closeable) { | ||
emit('close'); | ||
e.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test on a fresh installation. I just tried to update my already existing component and got this:
Uncaught ReferenceError: e is not defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's meant to be called in closeOnEscape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's really necessary at all. I've removed it and submitted a new PR. #1444
Currently, the Modal component uses a
<div>
element as the root with the Teleport feature. A modal like this does not properly trap tabs, allowing the user to tab to elements in the background which are supposed to be blocked by the modal.Changing to use a
<dialog>
element improves accessibility by locking the tab order to the modal and prevents interaction with the background using the keyboard by taking advantage of native browser features.This update should not affect any style or other features beyond the
<dialog>
behavior.