Skip to content

[material-ui][Dialog] Should not close until the IME is cancelled #39713

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.

Sign up for GitHub

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

Merged
mj12albert merged 1 commit into from
Nov 6, 2023
Merged

[material-ui][Dialog] Should not close until the IME is cancelled #39713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/mui-base/src/unstable_useModal/useModal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,11 @@ export function useModal(parameters: UseModalParameters): UseModalReturnValue {
// clicking a checkbox to check it, hitting a button to submit a form,
// and hitting left arrow to move the cursor in a text input etc.
// Only special HTML elements have these default behaviors.
if (event.key !== 'Escape' || !isTopModal()) {
if (
event.key !== 'Escape' ||
event.which === 229 || // Wait until IME is settled.
Copy link
Member

@DiegoAndai DiegoAndai Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.which is deprecated. Could we use .isComposing instead?

Copy link
Member

@mj12albert mj12albert Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I found in an old issue about isComposing with an odd behavior in macOS Safari: #19435 (comment)

Copy link
Contributor Author

@megos megos Nov 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the issue comment above, it needs to use which instead of isComposing for Safari.
This behavior is not fixed on the latest (17.1) Safari version.

!isTopModal()
) {
return;
}

Expand Down
18 changes: 18 additions & 0 deletions packages/mui-material/src/Dialog/Dialog.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,24 @@ describe('<Dialog />', () => {
expect(queryByRole('dialog')).to.equal(null);
});

it('should not close until the IME is cancelled', () => {
const onClose = spy();
const { getByRole } = render(
<Dialog open transitionDuration={0} onClose={onClose}>
<input type="text" autoFocus />
</Dialog>,
);
const textbox = getByRole('textbox');

// Actual Behavior when "あ" (Japanese) is entered and press the Esc for IME cancellation.
fireEvent.change(textbox, { target: { value: 'あ' } });
fireEvent.keyDown(textbox, { key: 'Esc', keyCode: 229 });
expect(onClose.callCount).to.equal(0);

fireEvent.keyDown(textbox, { key: 'Esc' });
Copy link
Member

@oliviertassinari oliviertassinari Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks off, fixing it in #43961.

expect(onClose.callCount).to.equal(1);
});

it('can ignore backdrop click and Esc keydown', () => {
function DialogWithBackdropClickDisabled(props) {
const { onClose, ...other } = props;
Expand Down