Add request-close command for dialogs #11045
Conversation
domenic
added
addition/proposal
This new command maps to the dialog's requestClose function.
- Rename enable close watcher for requestClose() - Add note to enable close watcher for request close
lukewarlow
force-pushed
the
request-close-command
branch
from
155ca45 to
0b2bccb
Compare
|
I believe this is just waiting on implementor interest cc @smaug---- @annevk |
|
@domenic can I take your comments during WhatNOT to be an official signal that Chrome is interested? |
https://bugs.webkit.org/show_bug.cgi?id=288476 Reviewed by NOBODY (OOPS!). Spec PR: whatwg/html#11045 This patch implements the request-close command for dialog, this maps to the requestClose() method. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/the-button-element/command-and-commandfor/on-dialog-behavior-request-close.tentative-expected.txt: Added. * Source/WebCore/dom/Element.h: * Source/WebCore/html/HTMLButtonElement.cpp: (WebCore::HTMLButtonElement::commandType const): * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::isValidCommandType): (WebCore::HTMLDialogElement::handleCommandInternal):
|
Thanks for checking! We should get @mfreed7 or @josepharhar to represent Chromium in that regard instead, as their team would be the one implementing it. |
There was a problem hiding this comment.
LGTM with nit.
Regarding Chromium implementer interest, I see @mfreed7 already approved https://chromium-review.googlesource.com/c/chromium/src/+/6213975, so I think we can say they are interested.
@smaug---- was vaguely positive during WHATNOT but indeed it would be good to give him a chance to affirmatively comment on this specific PR text.
|
|
||
| <li><p>Set <var>dialog</var>'s <span>enable close watcher for <code | ||
| data-x="dom-dialog-requestClose">requestClose()</code></span> to true.</p></li> | ||
|
|
||
| <li><p>If <var>returnValue</var> is not given, then set it to null.</p></li> |
There was a problem hiding this comment.
Why do we set it to null and not the empty string? With button elements it can never become null but that doesn't seem to matter as null and the empty string are always handled identically in close the dialog? So maybe we should remove that distinction? Probably okay as a follow-up, though might be nice to do here as now we're adding additional callers.
There was a problem hiding this comment.
null and the empty string aren't handled identically. If it's an empty string it replaces returnValue, if it's null returnValue isn't set to the new value (so keeps it's old value). Step 10 https://html.spec.whatwg.org/multipage/interactive-elements.html#close-the-dialog
There was a problem hiding this comment.
Was there no desire for this new feature to be able to emulate that then? Should this not be called out somehow?
There was a problem hiding this comment.
I think you've found a spec issue here, because Chrome does distinguish between button with a value attribute and button without. I'm just looking into it now.
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#concept-form-submit - the existing place does "if submitter has a value", is that correct spec language that I can copy? I don't know if has a value is really defined?
There was a problem hiding this comment.
A button element is defined to always have a value. See its concept-fe-value definition.
There was a problem hiding this comment.
Turns out there's an interop issue here, WebKit and Chrome behave different to Firefox (which follows the spec)
There was a problem hiding this comment.
I've updated the spec for close and request-close commands to check for the attributes existence before using the value. I'm checking WPTs for command invokers now to see if this case is covered if not I'll add them to the linked test PR. I'll raise a separate issue for the dialog form case: #11092
There was a problem hiding this comment.
Have added tests to that PR.
There was a problem hiding this comment.
then set*
But if we have to do this for all consumers of concept-fe-value maybe it should be defined differently. Hmm.
There was a problem hiding this comment.
Yeah perhaps as part of #11092 we'll need to revise how this is written.
|
Is this ready to go, or given discussions in Slack is WebKit neutral rather than supportive on this? |
|
You can consider WebKit as interested, but I think we should figure out #11092 as part of this if we think it'll impact the wording. Changing the HTML standard is very much a measure-twice-cut-once type of situation. Or to put it another way: it's better that we figure this out than require everyone reading the standard to figure it out. |
Add request-close command for dialogs
This new command maps to the dialog's requestClose function.
(See WHATWG Working Mode: Changes for more details.)
/form-elements.html ( diff )
/indices.html ( diff )
/interactive-elements.html ( diff )