-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP dialog closedby attribute spec #10157
Conversation
source
Outdated
data-x="event-cancel">cancel</code> at <span>this</span>, with the <code | ||
data-x="dom-Event-cancelable">cancelable</code> attribute initialized to true.</p></li> | ||
|
||
<li><p><i data-x="create-close-watcher-closeAction">closeAction</i> being to <span>close the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps rather than conditionally applying the close watcher we could just adjust the close and cancel actions dynamically based on the closedby
attribute value? This way they'd keep their user activation flags and order in stack would be what's expected?
This would require changing close watchers to treat null actions as effectively destroyed for the purposes of consuming? COuld also get confusing if the user deletes down the stack past where this active but empty watcher is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to do something like this. #9373 (comment) was my idea but using null cancel/close actions instead of a separate boolean might work as well.
source
Outdated
<li><p>If <var>oldValue</var> and <var>value</var> are in the same | ||
<span data-x="attr-dialog-closedby">state</span>, then return.</p></li> | ||
|
||
<li><p>If <var>value</var> is <span data-x="attr-closedby-none">none</span> then:</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add more steps dictating exactly what to do with the various oldValue vs value combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess one option is like popover to just close a dialog if you dynamically update this attribute while it's open?
dfbe28a
to
a5df507
Compare
</ol> | ||
|
||
<p class="XXX"><span>Light dismiss open dialogs</span> will be called by the <a | ||
href="https://github.com/w3c/pointerevents">Pointer Events spec</a> when the user clicks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to make a PR for this spec like the popover one to call this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be best to have a single hook from pointer events into HTML, I think. So we should either combine this with light dismiss open popovers or create a wrapper algorithm around both of them.
Not sure why but the diff link is outdated |
|
||
<li><p>If <var>clickedTopDialog</var>, then return.</p></li> | ||
|
||
<li><p>Perform <span>close the dialog</span> given <var>topDialog</var>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to close the dialog here or do we actually want to do cancel and close (aka requestClose())?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to fire cancel and then fire close as a follow up if not cancelled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openui/open-ui#950 (reply in thread) - Based on this comment, I need to change this to be cancel + close not just close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started with editorial review, but then realized that wasn't as helpful at this stage...
</ol> | ||
|
||
<p class="XXX"><span>Light dismiss open dialogs</span> will be called by the <a | ||
href="https://github.com/w3c/pointerevents">Pointer Events spec</a> when the user clicks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be best to have a single hook from pointer events into HTML, I think. So we should either combine this with light dismiss open popovers or create a wrapper algorithm around both of them.
source
Outdated
<p>The following <span data-x="concept-element-attributes-change-ext">attribute change | ||
steps</span>, given <var>element</var>, <var>localName</var>, <var>oldValue</var>, | ||
<var>value</var>, and <var>namespace</var>, are used for <span data-x="HTMLDialogElement">HTML | ||
dialog elements</span>:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach seems busted as the close watcher will be in the wrong place in the stack. The dialog will visually stay below any others, but behaviorally will move to the top of the close watcher stack. The user will be very confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was my gut feeling too. Thought I'd go ahead and write a rough spec though to have some of these weird edge cases in our minds when we discuss in whatnot.
source
Outdated
@@ -61264,6 +61328,48 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> { | |||
<li><p>Set <var>topDocument</var>'s <span>autofocus processed flag</span> to true.</p></li> | |||
</ol> | |||
|
|||
<p>To get the <dfn id="any-dialog-list">showing any dialog list</dfn> for a | |||
<code>Document</code> <var>document</var>:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these light dismiss-only helpers should move into the dialog light dismiss section.
source
Outdated
data-x="event-cancel">cancel</code> at <span>this</span>, with the <code | ||
data-x="dom-Event-cancelable">cancelable</code> attribute initialized to true.</p></li> | ||
|
||
<li><p><i data-x="create-close-watcher-closeAction">closeAction</i> being to <span>close the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to do something like this. #9373 (comment) was my idea but using null cancel/close actions instead of a separate boolean might work as well.
a5df507
to
0d4ebaa
Compare
Sorry for the delays on this, I wanted to wait till close watcher spec was a bit more settled. Now that the basic machinery is pretty stable I'm going to pick this back up and try and get a draft of this where the attribute updates dynamically and does the right thing. |
… handle closedby dynamically
701f445
to
96a4138
Compare
@@ -82286,7 +82500,8 @@ body { display:none } | |||
in reverse order:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The groups not empty check should probably be a check that there's a group with an enabled close watcher in?
And <var>group</var>
needs to be the last group with an enabled close watcher in
I'm going to go ahead and close this for now. I don't think I'll be able to give it much attention at the minute and don't want to be a blocker. cc @mfreed7 incase he wants to take a look at proposing a spec for this concept. |
First commit is pulled from whatwg#10157
First commit is pulled from whatwg#10157
FIxes #9373
This is a very rough WIP draft at this. Needs linting etc.
(See WHATWG Working Mode: Changes for more details.)
💥 Error: 429 Too Many Requests 💥
PR Preview failed to build. (Last tried on May 8, 2024, 8:18 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 HTML Diff Service - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request.
🔗 [Related URL]([object Object])
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.