Close dialog elements when the open attribute is removed #10124
Conversation
Fixes whatwg#5802 This patch adds attribute change steps to run the dialog closing steps whenever the open attribute is removed in order to prevent a bad state where the dialog is hidden but modal which renders the rest of the page inert.
source
Outdated
| return.</p></li> | ||
|
|
||
| <li><p>If <var>value</var> is null, then <span>close the dialog</span> given | ||
| <var>element</var>.</p></li> |
There was a problem hiding this comment.
This won't work, because it will return in step 1, because the open attribute has already been removed.
I guess we need to factor out steps 3-9 into a separate algorithm. Maybe "process open attribute removal"?
There was a problem hiding this comment.
Whoops I forgot that, I added a parameter to close in chromium. I just added it here. What do you think?
There was a problem hiding this comment.
I think it's slightly harder to understand than factoring out the steps would be, but I don't feel too strongly. (Thanks for updating the algorithm declaration style while you were there!)
There was a problem hiding this comment.
Ok, I factored out the steps into a separate algorithm
I will ship this behind a flag, and I will let yall know if any issues come up. I don't have a usecounter for it but I don't see why this change would be a problem. |
source
Outdated
| return.</p></li> | ||
|
|
||
| <li><p>If <var>value</var> is null, then <span>close the dialog</span> given | ||
| <var>element</var>.</p></li> |
There was a problem hiding this comment.
I think it's slightly harder to understand than factoring out the steps would be, but I don't feel too strongly. (Thanks for updating the algorithm declaration style while you were there!)
|
Seems reasonable modulo web compatibility. @lukewarlow might also be interested in this. |
There was a problem hiding this comment.
As a web developer (currently using <dialog> quite a bit), I'd be interested in seeing this change. (I've raised https://phabricator.services.mozilla.com/D201076 for Firefox).
source
Outdated
| <p>To <dfn>close the dialog</dfn> given a <code>dialog</code> element <var>subject</var> and a | ||
| string or null <var>result</var>:</p> | ||
|
|
||
| <ol> | ||
| <li><p>If the <span>is modal</span> flag of <var>subject</var> is true, then <span>request an | ||
| element to be removed from the top layer</span> given <var>subject</var>.</p></li> | ||
|
|
There was a problem hiding this comment.
(github doesn't seem to let me add comment in the right place outside the PR changes)
Don't "focusing steps" possibly end up firing an event at problematic time.
There was a problem hiding this comment.
I think the best thing would either be to not focus the previously focused element in this case, or to post a task/microtask to focus that element. Thoughts? @domenic @keithamus
There was a problem hiding this comment.
@dbaron too if you have thoughts
There was a problem hiding this comment.
Related for popovers: #10129
There was a problem hiding this comment.
I'd prefer to schedule a task. We should avoid losing focus on the document if we can help it.
There was a problem hiding this comment.
Scheduling a task sounds good to me too
There was a problem hiding this comment.
@smaug---- what do you think of scheduling a task?
There was a problem hiding this comment.
I made it schedule a task to focus for this case
https: //github.com/whatwg/html/pull/10124#discussion_r1483640232 Change-Id: Iab20699b345d96a8cffc83f7e7e44f4a03feb833
|
This feels like a big win for developers and having looked at the changes I don't think it's likely to cause any issues. Especially because it's unlikely developers will be relying on this behaviour (seems like its always undesirable) |
|
Any updates on this PR? It's mildly related to #10737 inasmuch as the close watcher logic is a little weirder (implementation-wise) when the open attribute can just be yanked off without calling close(). @josepharhar did you happen to try shipping the new behavior to see about web compat? (I'm happy to try, if there's consensus that this PR's behavior is desirable.) |
No, I haven't shipped this yet. I'm still waiting for feedback from @smaug---- in this thread: #10124 (comment) |
Fixes #5802
This patch adds attribute change steps to run the dialog closing steps whenever the open attribute is removed in order to prevent a bad state where the dialog is hidden but modal which renders the rest of the page inert.
(See WHATWG Working Mode: Changes for more details.)
/form-control-infrastructure.html ( diff )
/interactive-elements.html ( diff )