Re-implement `Modal` component using HTMLDialogElement (#461) #544

bedrich-schindler · 2024-05-30T19:03:27Z

Closes #461, closes #537.

src/components/Modal/__tests__/Modal.test.jsx

src/theme.scss

src/components/Modal/Modal.module.scss

src/components/Modal/__tests__/Modal.test.jsx

bedrich-schindler · 2024-06-03T11:30:43Z

Update CSS properties in README.md.

Done ✅

adamkudrna

Nice job! 👏🏻 Even with the decision of not having the closed Modal in the DOM. 👍

I just think we cannot remove the click-on-backdrop-to-close functionality.

src/components/Modal/Modal.jsx

src/components/Modal/Modal.module.scss

src/components/Modal/_settings.scss

src/components/Modal/__tests__/Modal.test.jsx

src/components/Modal/README.md

src/components/Modal/Modal.jsx

bedrich-schindler · 2024-09-08T09:05:27Z

PR updated:

* Introduced allowCloseOnBackdropClick and allowCloseOnEscapeKey prop, both set to true by default. It allows end-user to configure modal in such way that he/she can prevent closing by clicking on backdrop or pressing Escape key.
* Improved blocking of firing click events on primary and close button as there were missing conditions checking whether the button is disabled (it was there for a long time)

The biggest problem was with propagation of events, Enter was mistakenly closing dialog in unwanted cases. I can talk about it on Monday if you wish.

While I introduces allowCloseOnBackdropClick and allowCloseOnEscapeKey, I am thinking about allowProceedOnEnterKey (or something like that) to disable Enter functionality which can be unwanted in some scenarios.

bedrich-schindler · 2024-09-08T15:39:53Z

Just for info, tests need to be updated in #545 as new test environment is necessary.

adamkudrna

I have a note regarding forms inside dialog, otherwise OK for me.

src/components/Modal/README.md

bedrich-schindler · 2024-12-06T20:22:12Z

Can @adamkudrna or anybody else help me with this?

I returned to this to complete the pull request but suddenly, I have all the texts inside of Modal in white even though there is no change in the code. master is OK. Does anybody know why? I do not want to rebased it until this is resolved.

adamkudrna · 2024-12-31T15:48:20Z

Does anybody know why? I do not want to rebased it until this is resolved.

I added a fix. It seems to happen only when the docs is switched to dark mode.

bedrich-schindler · 2025-01-13T10:12:49Z

I have rebased and squashed it. I'm gonna perform self-review first as I have seen that documentation is not up-to-date with latest changes and then I will request review from you-

…461)

…lement (#461)

bedrich-schindler · 2025-01-14T12:57:52Z

Since rebase:

I fixed submitting dialog when form fields are present. It is not working either on master. Formerly, it was part of auto focus hook which I have modified. It was also containing focus trap that it can be removed now. Now, this functionality is part of dialogOnKeyDownHandler.js.

Then I have improved documentation and did minor fixes.

bedrich-schindler · 2025-01-14T13:04:59Z

I made separate commit (2514fd7) to demonstrate example with native form. Personally, I would not encourage this approach as every application differs in the way how it handles forms, state, validations and so. But it is true that it can be useful for example in that way, that native form will not be submitted until HTML validations are passing.

We had 4 examples of different kind, so I added native form as fifth and atypically explained it in the example. Even though it is atypical, I would leave it there as it is only place how we can describe example with modal with creating explicit section. And we do not want to handle all possibilities in the documentation, so it compromise I think.

@adamkudrna @mbohal, you can review the pull request and tell whether to include this native form example or drop it.

bedrich-schindler · 2025-01-15T15:35:41Z

I fixed submitting dialog when form fields are present. It is not working either on master. Formerly, it was part of auto focus hook which I have modified. It was also containing focus trap that it can be removed now. Now, this functionality is part of dialogOnKeyDownHandler.js.

I just want to mention (it will be part of commit message) that even though HTML Dialog states that it automatically focuses first focusable element in dialog, it does not work as our solution. The problem is that it autofocuses only some elements and for example states when there are only buttons and no input fields do not work. So we have to keep this functionality and we are not able to remove it!

The only question is: Do we want to have autoFocus always enabled? Personally, I would suggest removing autoFocus prop and make it mandatory as native solution would automatically focuses some elements even when our autoFocus is off.

mbohal · 2025-01-20T12:41:42Z

The only question is: Do we want to have autoFocus always enabled? Personally, I would suggest removing autoFocus prop and make it mandatory as native solution would automatically focuses some elements even when our autoFocus is off.

Agreed we want to make autoFocus always on.

src/components/Modal/Modal.jsx

src/components/Modal/_hooks/useModalFocus.js

…` tests (#461) `happy-dom` is used due to missing HtmlDialogElement support in `jest`. Just to mention, `happy-dom` provides partial support for dialog element, so not all test can be implemented.

bedrich-schindler · 2025-01-21T13:25:51Z

I have merged PR with happy-dom into this PR.

bedrich-schindler · 2025-01-21T13:36:05Z

After discussion with @mbohal, we decided not to change our autoFocus functionality. Even though native autofocus functionality was introduced into , it does not work as good as out autoFocus functionality because it selects first item which is typically close button in our case, so that's the reason we want to leave it as it is.

I dug deep into in again @mbohal and I found out, that with autoFocus disabled (through props, not through deleting whole functionality), our autoFocus internally forces focus on element, so it still can prevent native behavior. I had been testing it with completely removed functionality and probably cached page, so I had been obtaining wrong results before.

…DialogElement (#461)

…ng HTMLDialogElement (#461)

adamkudrna

Well done with beautiful demos! I only have two major requests/comments:

Make a new "Forms" section. Forms in dialogs are a common use case. This is why I click "Request changes" here.
Consider if we can get more out of the native functionality of the <dialog>. But maybe you have done an in-depth reserach and I see it wrong.

Thank you!

adamkudrna · 2025-01-22T11:24:50Z

src/components/Modal/Modal.module.scss

-        height: 100vh;
+    .root[open] {
+        display: flex;
+        animation: fade-in theme.$animation-duration ease-out;


Please:

add a note the animation is currently only supported in Chrome, Edge and Opera (Blink rendering engine),

wrap the animation into @media (prefers-reduced-motion: no-preference).

adamkudrna · 2025-01-22T17:56:32Z

src/components/Modal/Modal.module.scss

        background: theme.$backdrop-background;
+        animation: inherit;


+1 for inheritance. Just wrap the line into @media (prefers-reduced-motion: no-preference) please.

adamkudrna · 2025-01-22T18:08:55Z

src/components/Modal/README.md

  this feature off, set the `autofocus` prop to `false`.

+- Modal **submits the form when the user presses the `Enter` key** . The primary
+  button is clicked in this case. To turn this feature off, set the


I think the click is just emulated, isn't it?

It is not. click() function is called directly on HTML button, it is not emulated, it's real click that also triggers its own click event. So I would not call it emulated.

@mbohal can correct me, but emulated event would be dispatchEvent(event), but not click().

adamkudrna · 2025-01-22T18:09:21Z

src/components/Modal/README.md

+  `allowPrimaryActionOnEnterKey` prop to `false`.
+
+- Modal **closes when the user presses the `Escape` key**. The close button is
+  clicked in this case. To turn this feature off, set the `allowCloseOnEscapeKey`


The same about "clicking" here.

adamkudrna · 2025-01-22T18:10:32Z

src/components/Modal/README.md

@@ -225,9 +237,33 @@ React.createElement(() => {
            </ModalHeader>
            <ModalBody>
              <ModalContent>
-                <FormLayout fieldLayout="horizontal">
-                  <TextField label="Username" />
+                <FormLayout fieldLayout="horizontal" labelWidth="limited">


Thanks for extending the form with new fields!

adamkudrna · 2025-01-22T18:11:17Z

src/components/Modal/README.md

+                  connect it with the form.
+                </p>
+                <p>
+                  Although we do not encourage using this approach, it is still


Why don't we encourage the native solution? Is there something wrong when combining it with React? I'd like to see the reason mentioned here.

adamkudrna · 2025-01-22T18:37:10Z

src/components/Modal/README.md

+      <Button
+        label="Launch modal as native form"
+        onClick={() => setModalOpen(5)}
+      />


I think this section has become too long and deserves splitting into more blocks. From my experience, using the modal dialogs with forms is a frequent use case and it deserves a prominent place in the design system docs. This is how I see it:

Create a new section called "Forms" right before the "Mouse and Keyboard Control" section.

Explain shortly the Modal can be used with or without the native <form> element. If you can also tell what are the use cases, even better.

Extract the text from the last example (native form) so it's right in the document and not hidden in the demo.

In the original section, please move the "Launch modal as dialog" example to the first place. Ever since we have the Modal component, I always click on the first button and immendiately hate myself for having to wait for the blocking modal to close 🙈.

In this case, let's discuss this on Monday. We explicitly approved this on last meeting where you were supposed to be ...

TLDR; I do not encourage users to use native form behavior as it is good for plain HTML apps without JS (or with minimal JS), but not for React apps where the principles are different.

If we create Forms section, what will we do with this section? There are supported to be typical examples. If we create separate section, I would be duplicated ...

src/components/Modal/_helpers/dialogOnCancelHandler.js

adamkudrna · 2025-01-22T18:50:04Z

src/components/Modal/_helpers/dialogOnClickHandler.js

+  const dialogRect = dialogRef.current.getBoundingClientRect();
+  const isClickedOnBackdrop = dialogRef.current === e.target && (
+    e.clientX < dialogRect.left
+    || e.clientX > dialogRect.right
+    || e.clientY < dialogRect.top
+    || e.clientY > dialogRect.bottom
+  );


Do we still need this with native dialog? The <dialog> triggers the close event so I was wondering if we could listen on it/use it?

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#handling_the_return_value_from_the_dialog

If it wasn't needed, it wouldn't be here. Neither close nor cancel event is called on backdrop click. If you want to close dialog by clicking on backdrop, you have to listen to click event and detect user clicked outside of dialog element or use different method of detection. However, this is most common approach.

It is quite hard to decide what to comment. This is default behavior/knowledge and developer is supposed to know this if he/she want to dig into it. Now, each line of each event handler is commented. But I would add docblock for each function to make it crystal clear.

adamkudrna · 2025-01-22T18:54:17Z

webpack.config.babel.js

@@ -4,7 +4,7 @@ const StyleLintPlugin = require('stylelint-webpack-plugin');
 const TerserPlugin = require('terser-webpack-plugin');
 const VisualizerPlugin = require('webpack-visualizer-plugin2');

-const MAX_DEVELOPMENT_OUTPUT_SIZE = 3300000;
+const MAX_DEVELOPMENT_OUTPUT_SIZE = 3400000;


How come? 🤔

bedrich-schindler added BC Breaking change feature labels May 30, 2024

bedrich-schindler self-assigned this May 30, 2024

bedrich-schindler requested review from adamkudrna and mbohal as code owners May 30, 2024 19:03

bedrich-schindler requested a review from dacerondrej May 30, 2024 19:04

bedrich-schindler commented May 31, 2024

View reviewed changes

src/components/Modal/__tests__/Modal.test.jsx Outdated Show resolved Hide resolved

mbohal approved these changes Jun 3, 2024

View reviewed changes

mbohal self-requested a review June 3, 2024 08:43

mbohal requested changes Jun 3, 2024

View reviewed changes

src/theme.scss Outdated Show resolved Hide resolved

dacerondrej suggested changes Jun 3, 2024

View reviewed changes

src/components/Modal/Modal.module.scss Outdated Show resolved Hide resolved

src/components/Modal/__tests__/Modal.test.jsx Outdated Show resolved Hide resolved

bedrich-schindler requested review from mbohal and dacerondrej June 6, 2024 05:25

mbohal approved these changes Jun 6, 2024

View reviewed changes

dacerondrej approved these changes Jun 12, 2024

View reviewed changes

adamkudrna requested changes Jun 17, 2024

View reviewed changes

adamkudrna mentioned this pull request Aug 21, 2024

In docs Modal is not dismissed by pressing Esc #537

Open

adamkudrna reviewed Nov 4, 2024

View reviewed changes

src/components/Modal/README.md Show resolved Hide resolved

Re-implement Modal component using HTMLDialogElement (#461)

ec8d690

bedrich-schindler force-pushed the bc/461 branch from b7ca42f to ec8d690 Compare January 13, 2025 10:09

bedrich-schindler added 4 commits January 13, 2025 13:27

fixup! Re-implement Modal component using HTMLDialogElement (#461)

bf704ca

fixup! fixup! Re-implement Modal component using HTMLDialogElement (#…

61bcff4

…461)

fixup! fixup! fixup! Re-implement Modal component using HTMLDialogE…

fe13200

…lement (#461)

Native form example

2514fd7

bedrich-schindler requested review from adamkudrna and mbohal January 14, 2025 13:05

mbohal requested changes Jan 20, 2025

View reviewed changes

src/components/Modal/Modal.jsx Show resolved Hide resolved

src/components/Modal/_hooks/useModalFocus.js Outdated Show resolved Hide resolved

Replace jsdom with happy-dom testing environment to enable `Modal…

157b858

…` tests (#461) `happy-dom` is used due to missing HtmlDialogElement support in `jest`. Just to mention, `happy-dom` provides partial support for dialog element, so not all test can be implemented.

fixup! fixup! fixup! fixup! Re-implement Modal component using HTML…

5776dcb

…DialogElement (#461)

bedrich-schindler requested a review from mbohal January 21, 2025 13:43

fixup! fixup! fixup! fixup! fixup! Re-implement Modal component usi…

715e2e5

…ng HTMLDialogElement (#461)

adamkudrna requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-implement `Modal` component using HTMLDialogElement (#461) #544

Re-implement `Modal` component using HTMLDialogElement (#461) #544

bedrich-schindler commented May 30, 2024 •

edited by adamkudrna

Loading

bedrich-schindler commented Jun 3, 2024 •

edited

Loading

adamkudrna left a comment

bedrich-schindler commented Sep 8, 2024

bedrich-schindler commented Sep 8, 2024

adamkudrna left a comment

bedrich-schindler commented Dec 6, 2024

adamkudrna commented Dec 31, 2024

bedrich-schindler commented Jan 13, 2025

bedrich-schindler commented Jan 14, 2025

bedrich-schindler commented Jan 14, 2025

bedrich-schindler commented Jan 15, 2025

mbohal commented Jan 20, 2025

bedrich-schindler commented Jan 21, 2025

bedrich-schindler commented Jan 21, 2025

adamkudrna left a comment

adamkudrna Jan 22, 2025

adamkudrna Jan 22, 2025

adamkudrna Jan 22, 2025

bedrich-schindler Jan 24, 2025

adamkudrna Jan 22, 2025

adamkudrna Jan 22, 2025

adamkudrna Jan 22, 2025

adamkudrna Jan 22, 2025

bedrich-schindler Jan 24, 2025

adamkudrna Jan 22, 2025 •

edited

Loading

bedrich-schindler Jan 24, 2025

adamkudrna Jan 22, 2025

Re-implement Modal component using HTMLDialogElement (#461) #544

Are you sure you want to change the base?

Re-implement Modal component using HTMLDialogElement (#461) #544

Conversation

bedrich-schindler commented May 30, 2024 • edited by adamkudrna Loading

bedrich-schindler commented Jun 3, 2024 • edited Loading

adamkudrna left a comment

Choose a reason for hiding this comment

bedrich-schindler commented Sep 8, 2024

bedrich-schindler commented Sep 8, 2024

adamkudrna left a comment

Choose a reason for hiding this comment

bedrich-schindler commented Dec 6, 2024

adamkudrna commented Dec 31, 2024

bedrich-schindler commented Jan 13, 2025

bedrich-schindler commented Jan 14, 2025

bedrich-schindler commented Jan 14, 2025

bedrich-schindler commented Jan 15, 2025

mbohal commented Jan 20, 2025

bedrich-schindler commented Jan 21, 2025

bedrich-schindler commented Jan 21, 2025

adamkudrna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkudrna Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Re-implement `Modal` component using HTMLDialogElement (#461) #544

Re-implement `Modal` component using HTMLDialogElement (#461) #544

bedrich-schindler commented May 30, 2024 •

edited by adamkudrna

Loading

bedrich-schindler commented Jun 3, 2024 •

edited

Loading

adamkudrna Jan 22, 2025 •

edited

Loading