Re-implement Modal component using HTMLDialogElement (#461)

[bedrich-schindler]

Closes #461, closes #537.

[bedrich-schindler]

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.

[bedrich-schindler]

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]

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.

[bedrich-schindler]

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]

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]

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-

[bedrich-schindler]

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]

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]

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]

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.

[bedrich-schindler]

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

[bedrich-schindler]

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.

[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 research and I see it wrong.

Thank you!

[bedrich-schindler]

Mention that we are using native and that we do not want to have hidden dialog element, we want remove it when closed ...

[bedrich-schindler]

Everything should be done as agreed on Monday.

[adamkudrna]

Nice! Please fix the auto-size CSS, the rest of my comments is just suggestions. Thank you!