Fix "Handling the return value" example - #44627
Conversation
- Code description was inaccurate - Falsely stated that "Esc" doesn't trigger close event - "Confirm" button having a value that is never used is confusing - Code didn't check for the initial "" returnValue, which seems like an oversight to me
github-actions
Bot
added
Content:HTML
|
Preview URLs (1 page) (comment last updated: 2026-07-12 21:09:53) |
|
Oh, maybe it should be mentioned that on pressing Esc a |
should make it clear that it's possible to separately handle the cancel event?
pepelsbey
left a comment
pepelsbey
left a comment
There was a problem hiding this comment.
Hey! Thank you for contributing.
I have a few suggestions below, but also there’s a very similar demo for the :modal pseudo-class. It might be worth updating it in the same PR.
| // "Cancel" button closes the dialog without submitting because of [formmethod="dialog"] | ||
| // or `favDialog.close()` was called, triggering a close event. | ||
| // Or "Esc" was pressed, followed by a cancel event, which is also followed by a close event. |
There was a problem hiding this comment.
It’s better not to bury prose in code comments, as it’s harder to read. I would suggest adding a shorter comment and explaining it in more detail outside the code snippet. But I think it’s already explained above.
There was a problem hiding this comment.
Yeah, it also looked a bit ugly to me, wasn't sure about it either. I added it because to me it kind of felt as if the initial comment suggested that the "Cancel" button was the only way to trigger that function. Not that it was actually saying that.
There was a problem hiding this comment.
I was also wondering if it would be appropriate to maybe delete more comments from the code that don't add much additional value
| This example demonstrates the [`returnValue`](/en-US/docs/Web/API/HTMLDialogElement/returnValue) of the `<dialog>` element and how to close a modal dialog by using a form. By default, the `returnValue` is the empty string or the value of the button that submits the form within the `<dialog>` element, if there is one. | ||
|
|
||
| This example opens a modal dialog when the "Show the dialog" button is activated. The dialog contains a form with a {{HTMLElement("select")}} and two {{HTMLElement("button")}} elements, which default to `type="submit"`. An event listener updates the value of the "Confirm" button when the select option changes. If the "Confirm" button is activated to close the dialog, the current value of the button is the return value. If the dialog is closed by pressing the "Cancel" button, the `returnValue` is `cancel`. | ||
| This example opens a modal dialog when the "Show the dialog" button is activated. The dialog contains a form with a {{HTMLElement("select")}} and two {{HTMLElement("button")}} elements, which default to `type="submit"`. If the "Confirm" button is activated to close the dialog, an event listener sets `returnValue` to the current value of the select box instead of the button's value. If the dialog is closed by pressing the "Cancel" button, the `returnValue` is `cancel`. |
There was a problem hiding this comment.
the button's value
Since this PR removes the button's value, it now reads off.
There was a problem hiding this comment.
So what you're saying is remove
instead of the button's value
?
I'm just asking because I'm not 100% sure I understand the "it now reads off".
add requestClose button handle cancel event simplify close event handling
|
I thought about the example a little bit more because it still kind of bugged me. I made another commit with a suggestion for changes/additions to it:
Didn't include any changes to the accompanying descriptions outside the code in this commit. What do you think? |
3 participants
Description
Fixes inaccuracies and oversights in the example description and code.
Maybe the code could say
<option value>Choose…</option>instead of<option value="">Choose…</option>?And maybe instead of
favDialog.returnValue === ""the truthiness could be checked instead.But probably fine as it is?
Of course it could also check for
(favDialog.returnValue === "default" || favDialog.returnValue === "")instead and leave the<option value="default">Choose…</option>untouched, but in any way it probably should check for the initial "" value in some way IMO.Motivation
Descriptions that don't match the code are confusing.
Code with additional bits that have no influence on the behavior even though they look like they might is confusing.
Code that doesn't handle edge cases makes it easy to forget that they exist, leading to potential bugs along the road.
Clearing those things up will hopefully be beneficial to the average reader.
Additional details
Related issues and pull requests