sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.75k stars 4.13k forks source link

Allow bind:open with <dialog> #4723

Open tvanc opened 4 years ago

tvanc commented 4 years ago

Attempting to use bind:open with <dialog> results in an error:

<!-- Dialog.svelte -->
<script>
  export let open = false
</script>

<dialog bind:open />
<!--
Error: Module build failed (from ./node_modules/svelte-loader/index.js):
Error: ValidationError: 'open' binding can only be used with <details> (17:46)
-->

To Reproduce View REPL

Expected behavior I expected bind to work with <dialog> as well as <details>.

Information about your Svelte project: Svelte v3.21.0

Severity Annoying.

tvanc commented 4 years ago

I'd love to make a PR but I'm not sure how to write the test.

If anyone could look at my attempt and tell me what I'm doing wrong, that'd be swell.

https://github.com/sveltejs/svelte/compare/master...tvanc:openBindingForDialog

tanhauhau commented 4 years ago

interestingly, based on the MDN docs, https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement, there's no event we can listen to determine when a dialog is open.

there's close to know when it is closed. but no way we can tell if it is opened.

tvanc commented 4 years ago

Is having an event necessary for the directive and the test? Maybe we can use MutationObserver as a workaround

dimfeld commented 4 years ago

I don't have a strong opinion on if Svelte should support this, but here's an example I found of using a MutationObserver to fire an event when the dialog opens: https://jsfiddle.net/vmj8d79q/. Looks like it works fine.

ZerdoX-x commented 4 years ago

I really would like to have the ability to bind open property for dialogs in svelte

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ZerdoX-x commented 3 years ago

Why details has this option but dialog does not? I don't think it's that hard. I'll try to implement this later

tvanc commented 3 years ago

@ZerdoX-x I'd love to see how you do it. I don't know svelte internals very well. I couldn't figure it out.

antony commented 2 years ago

The lack of a close event could be problematic. I'm also running into this issue, but I wonder if it's actually solveable.

joepetrakovich commented 2 years ago

I'm not sure if I should make a separate issue for this but I noticed something in the same area.

I'm using a dialog like in the below code, which works fine, but if I try to use the <form method='dialog'> technique for closing the dialog based on button clicks, it makes it so the dialog cannot be reopened. Clicking the close button will close the dialog, but subsequent calls to .$set({ open: true}) will not reopen it. I'm guessing because the way the browser implements the form close is destroying the attribute in some way so svelte can't use it.

If I remove the form and just use svelte reactivity, it works fine and allows the dialog to be reopened.

<script>
    export let open = true;
</script>

<dialog {open}>
   <form method='dialog'>
        <button value="cancel">Cancel</button>  //doesn't allow reopening
  </form>
</dialog>

<dialog {open}>
        <button value="cancel" on:click={() => open = false}>Cancel</button> //allows reopening
</dialog>
KTibow commented 1 year ago

idk how bindings internally work but this could be implemented by

danielniccoli commented 1 year ago

The lack of a close event could be problematic. I'm also running into this issue, but I wonder if it's actually solveable.

HTMLDialogElement has a close and cancel event (although that may not have been true at the time you wrote your comment)

sprataa commented 10 months ago

I'm not sure if I should make a separate issue for this but I noticed something in the same area.

I'm using a dialog like in the below code, which works fine, but if I try to use the <form method='dialog'> technique for closing the dialog based on button clicks, it makes it so the dialog cannot be reopened. Clicking the close button will close the dialog, but subsequent calls to .$set({ open: true}) will not reopen it. I'm guessing because the way the browser implements the form close is destroying the attribute in some way so svelte can't use it.

If I remove the form and just use svelte reactivity, it works fine and allows the dialog to be reopened.

<script>
    export let open = true;
</script>

<dialog {open}>
   <form method='dialog'>
        <button value="cancel">Cancel</button>  //doesn't allow reopening
  </form>
</dialog>

<dialog {open}>
        <button value="cancel" on:click={() => open = false}>Cancel</button> //allows reopening
</dialog>

try this out: <dialog {open} on:close={() => open = false}> this is how I am currently working around dialog's missing bind for open

KTibow commented 10 months ago

The problem with that is that it doesn't enable the backdrop

brunnerh commented 10 months ago

That is besides the point of this issue and also not the default behavior.

If in plain HTML adding open opens the dialog non-modal, then it would be highly inconsistent if Svelte just decides to do something else. Personally, I find it much easier to interact with dialog components imperatively anyway, you can just throw them away after being closed and keep everything clean.

Manually calling showModal() really should not be a problem.

danielniccoli commented 10 months ago

That is besides the point of this issue and also not the default behavior.

If in plain HTML adding open opens the dialog non-modal, then it would be highly inconsistent if Svelte just decides to do something else. Personally, I find it much easier to interact with dialog components imperatively anyway, you can just throw them away after being closed and keep everything clean.

Manually calling showModal() really should not be a problem.

I disagree. The backdrop is an integral part. If you open the modal without the backdrop showing, you did not follow the recommendations from the specs.

Which is why the on:bind needs to do more than just modify the open property.

brunnerh commented 10 months ago

What 😅 It's not a modal, it's a dialog. It can be used in any context for whatever the given use case requires.

Just because modals may be the most common use case for dialogs, does not mean that Svelte should introduce opinionated, non-standard behavior.

sprataa commented 10 months ago

The problem with that is that it doesn't enable the backdrop

oh yes, for that you need to call showModal. this is how we do it:

<script>
let modal
$: if (open) modal.showModal()
</script>
<dialog bind:this={modal} {open} on:close={() => open = false}>
...
</dialog>  

not pretty, but it works

andreasnuesslein commented 10 months ago

@danielniccoli I think your confusion stems from the fact that <dialog> can actually do both behaviours. this blogpost made the difference clear to me: https://blog.webdevsimplified.com/2023-04/html-dialog/

specifically this:

dialog.show() // Opens a non-modal dialog
dialog.showModal() // Opens a modal

cheers

danielniccoli commented 10 months ago

I didn't know it can do both. I read in the MDN about it, and the docs were either updated, or I just remember it wrong.

hyunbinseo commented 10 months ago

Please distinguish the two types of <dialog> usage. These are from MDN.

  1. HTML-only dialog - This example demonstrates the create a non-modal dialog by using only HTML.
  2. Creating a modal dialog - This example demonstrates a modal dialog with a gradient backdrop.

For modal usage - with the backdrop and the focus-trap - reference the official Svelte example.

It already binds the showModal boolean value to showModal() and close() the modal.

<script>
  export let showModal; // boolean
  let dialog; // HTMLDialogElement
  $: if (dialog && showModal) dialog.showModal();
</script>

<dialog
  bind:this={dialog}
  on:close={() => (showModal = false)}
  on:click|self={() => dialog.close()}
>

For a wrapper library, consider my svelte-html-modal.


Allowing bind:open in <dialog> will be intuitive since it is supported in details,

<script>
  let open = false;
  $: console.log(open);
</script>

<details bind:open>
  <summary>Details</summary>
  Something small enough to escape casual notice.
</details>

but without a proper DOM event in <dialog>, it seems undesirable.

Maybe provide a better lint result to explain why bind:open does not work?

Maze-fr commented 9 hours ago

I'm using Svelte 5 RC, and the problem is still here.