Skip to content
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

Merged
merged 6 commits into from
Feb 23, 2024
Merged

[4.x] Change Inertia Modal.vue component to use a native <dialog> #1431

merged 6 commits into from
Feb 23, 2024

Conversation

Smef
Copy link
Contributor

@Smef Smef commented Jan 15, 2024

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.

@jessarcher jessarcher self-requested a review January 15, 2024 22:54
@jessarcher
Copy link
Member

Thanks! I've been meaning to do this. Marking as draft while I review.

@jessarcher jessarcher marked this pull request as draft January 15, 2024 22:56
@jessarcher
Copy link
Member

I've tested this out locally, and it works great! I've confirmed that the teleport is not required to break out of any overflow: hidden or opacity settings on a parent element due to the browser moving it to the top layer.

I was initially confused by the setTimeout, but I can see that it's required to let the fade animation complete.

Two things:

  1. It would also be good to implement this for the Livewire stack to keep things consistent. Are you able to include that in this PR? If not, I can take a look.

  2. It would be nice to use the native backdrop and animations/transitions instead of using the <dialog> as the backdrop with a bunch of divs and Vue transitions inside. However, it looks like the animation/transition support is not fully supported on Firefox yet, so I'm happy for this to stay as-is for now and we can further improve it later.

@Smef
Copy link
Contributor Author

Smef commented Jan 17, 2024

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.

@Smef
Copy link
Contributor Author

Smef commented Jan 17, 2024

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.

@jessarcher
Copy link
Member

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 don't mind comments explaining things that aren't obvious.

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.

Tested in Firefox and once the modal is closed, the page becomes unresponsive to clicks. I thought it might be something to do with @alpinejs/focus plugin's trap feature, but the problem persists if I remove it. Any ideas?

@Smef
Copy link
Contributor Author

Smef commented Jan 19, 2024

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 ConfirmsPasswords class's stopConfirmingPassword(). The Log Out Browsers modal, however, is just updating state.

Clicking the background or pressing escape in the 2FA modal also does not trigger stopConfirmingPassword() as that function call is tied to the cancel button exclusively.

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.

@jessarcher
Copy link
Member

Hey @Smef, I can't figure this one out either. It's like the <dialog> is still blocking, even though the close method has been called, making it no longer visible.

If I manually get the dialog DOM element from the browser inspector and call close, nothing changes. But if I call showModal and then close, everything is fine.

Also, if I delete the dialog DOM element, then everything is fine also.

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.

@Smef
Copy link
Contributor Author

Smef commented Feb 1, 2024

I've reverted the Livewire implementation

@Smef
Copy link
Contributor Author

Smef commented Feb 1, 2024

If I had to guess what was happening with this, it would be this:

  1. <dialog> is opened
  2. Livewire request
  3. Livewire response
  4. <dialog> gets removed from the DOM by Livewire
  5. A new <dialog open> is added to the DOM by Livewire, already in the open state
  6. New <dialog> is closed
  7. The original <dialog> is left open, blocking input even though the element has been removed

…and disappearing, preventing modal size shifting
@Smef Smef marked this pull request as ready for review February 22, 2024 21:44
@taylorotwell taylorotwell merged commit c0e19ca into laravel:4.x Feb 23, 2024
7 checks passed
}
});

const close = () => {
if (props.closeable) {
emit('close');
e.preventDefault();
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

@Smef Smef Feb 29, 2024

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

@Smef Smef deleted the change-modal-to-native-dialog branch February 29, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants