Missing attribute changed steps for dialog can cause assertions to be hit

[lukewarlow]

What is the issue with the HTML Standard?

Currently showModal() and show() on a dialog, assert that the dialog isn't in document's open dialogs list.

However, it turns out this isn't always true.

For example if you do:
dialog.showModal(); // Adds the dialog to the open dialogs list
dialog.removeAttribute('open'); // Dialog is now in a strange state
dialog.showModal(); // Assertion fails because the dialog is already in the open dialogs list

The spec needs attribute changed steps for dialog open attribute being removed and needs to remove the dialog from the list.

[lukewarlow]

Related to #5802 but a more scoped and required fix for the spec.

[domenic]

I don't understand why the correct fix here is to implement some subset of #5802 instead of removing the broken assert.

[lukewarlow]

For the close watcher aspect it would actually be broken behaviour right now I think. The original close watcher would never be removed from the list so I'm assuming events would fire twice or it would lead to null reference or something.

This also is what chrome implements so I thought it's generally best for the spec to match implementations.

[lukewarlow]

Also removing the assertions doesn't prevent bugs occurring. open dialogs list is a list not an ordered set, which means it can have duplicate entries. I don't know if doing remove(this) only removes the last one or all matching entries but I can potentially see that causing issues too.

[lukewarlow]

cc @mfreed7 (and @keithamus because I know you're implementing in Firefox)

[keithamus]

Yeah it seems the change is observable. We could change the set the dialog close watcher steps to tear down the existing close watcher, if it exists, but that seems a little awkward. I wonder if this allows some level of circumvention of the close watcher anti abuse mechanisms, also?

[domenic]

Alright, thanks for explaining. It sounds like we have a few options:

• Revert Add dialog light dismiss behavior #10737 entirely. Sad, not fun.
• Patch up the spec by removing the broken assert and fixing any other issues that are caused by the new setup. Complicated, unclear.
• The minimal change in Add attribute changed steps to dialog element #10954.
• The more maximal fix in Close dialog elements when the open attribute is removed #10124, and/or some other more maximal fix for Dialog should be better-behaved on misuse, probably #5802.

Assuming other implementers are OK with it, I'm happy with trying to move forward and applying one of the fixes. I'd appreciate a summary of the delta between #10954 and #10124 / the proposals in #5802 though, to know what the future evolution might look like.

[lukewarlow]

I'll try my best to summarise the differences not wishing to speak for @josepharhar (feel free to correct me if I'm wrong).

#10954 is a minimal set of changes needed to satisfy the spec assertions as written (with the addition of an extra one for clarity), and to match the actual implementation within Chromium (again @mfreed7 correct me if I'm wrong on that).

In summary it adds a new attribute changed step for the dialog element such that if the open attribute is removed, the dialog is removed from the document's list of open dialogs, preventing assertions in showModal/show being hit. It also destroys and nullifies dialog's close watcher, asserting this within 'setup the dialog close watcher'. While definitely a normative change it's designed to spell out the necessary changes for the spec to be in a good state.

#10124 is a more maximal approach to the wider problem of the open attribute, it does new normative changes which solve wider UX (but not spec) issues such as modal dialogs still blocking the document. It does this by invoking the full close algorithm of the dialog, which ensures full cleanup of state. So it's a superset of my proposed change. This requires more thought on changes related to focusing which is I believe the hold up on it.

Side Note: I'm fully supportive of the direction of #10124 and I think it should be the eventual end state of the spec.

I don't think reverting the PR is the best approach, despite contentions around process I believe there is overall support of at least two implementors for the overall gist of the addition and it's largely just the exact specifics that need cleaning up. I also don't believe we would have necessarily found all these issues before merging even if it had stayed unmerged for longer.

I also don't think patching up the spec by removing assertions etc is the correct approach either. I believe there could be observable differences between cleaning up upon calling show/showModal and cleaning up upon removal of the open attribute.

Specs should generally match implementations and that is what my PR proposes.

[keithamus]

to know what the future evolution might look like.

#10954 is/can effectively be a subset; that is to say I believe it is entirely plausible for us to merge #10954 and follow up with #10124. I believe #10954 (or at least something like it) is necessary, and #10124 just adds extra steps that aren't in conflict.

[mfreed7]

Thanks for highlighting this issue! And I roughly/completely agree with the above comments. #10124 is the best eventual state, #10954 is a great (and perhaps less risky?) intermediate step, and we should attempt to land both. Both PRs make the open attribute behavior incrementally better. I suppose we could try to just jump directly to #10124, but that'd require some compat work or an attempt to ship it in a browser. I'm willing to try to do that in Chromium, if it helps the effort, and folks are onboard with that plan.

open dialogs list is a list not an ordered set, which means it can have duplicate entries.

This is a very interesting point. In Chromium, it's actually an ordered set, but also with the same assertions that the spec has, to make sure we're not using it as a set. Kind of belt and suspenders, but I wonder if the spec should make it an ordered set? Given the assertions, it shouldn't be observable.

[domenic]

Thanks all; that was very helpful. With my editor hat on, I'm convinced that #10954 first is the way to go. I hope someone can follow up with the rest of the #10124 stuff in due time.

We still need to formally confirm multi-implementer interest beyond Chromium. If @lukewarlow can speak for WebKit and/or @keithamus can speak for Gecko in that regard, that's excellent, but I'll defer that judgement to you two.

[lukewarlow]

We still need to formally confirm multi-implementer interest beyond Chromium. If @lukewarlow can speak for WebKit and/or @keithamus can speak for Gecko in that regard, that's excellent, but I'll defer that judgement to you two.

I'm unfamiliar with whatwg procedure on that point but I don't believe I can formally speak for WebKit. Cc @annevk

Having said that they've recently indicated that they will mark the wider feature as positive.

[mfreed7]

I've been looking into #10954 a bit more this morning, and it (now) scares me a bit. Here are the things that #10124 does that #10954 does not do:

• remove the dialog from the top layer (scares me the most)
• set "is modal" to false
• set returnValue
• do focus fixup stuff (this one is ok by me - I think it should be skipped)
• fire the (sync) beforetoggle event (should probably get fired? We do fire this event when the popover attribute changes state.)
• fire the (async) toggle event (this should probably get fired?)
• fire the (async) close event (this should probably get fired?)

Perhaps it might make more sense to try to jump to #10124 directly, with Joey's commented suggestions implemented? I.e. take #10124 and make it not do focus fixup? I disagree with that comment about toggle/beforetoggle - I think we should stay parallel to popover and fire those.

Any changes around this have some compat risk, but somehow given the above list of things, I'm a bit more comfortable jumping much closer to running the full close() behavior when the open attribute is removed.

[mfreed7]

• do focus fixup stuff (this one is ok by me - I think it should be skipped)

@aleventhal @scottaohara can you chime in here? The TL;DR (and there's a lot to read, so hopefully this saves you time) is that we're trying to figure out what to do in the very specific case that someone has a dialog (modal or modeless) and they manually remove the open attribute, rather than calling dialog.close(). There's a concern about synchronously doing the "return to previously focused element" behavior in that case, so there are two options:

1. Do focus fixup asynchronously.
2. Don't do focus fixup in this case.

The pro of #1 are that it still tries to do the focus fixup. The con is that the behavior might be funny. E.g. if Javascript first removes the open attribute and then moves focus itself. The focus will then be moved twice.

The pro of #2 is that there are no async shenanigans like #1. The con is that focus might not be fixed up in some cases. If we went this route, at least in Chromium, we'd definitely keep our existing console error that says "please don't touch the open attribute - use .close()". So hopefully this is a corner case only. (My preference, because it feels lower risk, is #2.)

Thoughts?

[lukewarlow]

Also worth pointing out that the status quo is that the document is almost completely broken right now, and there's definitely no focus fixups happening so 1 and 2 will both be an improvement.

[aleventhal]

Also worth pointing out that the status quo is that the document is almost completely broken right now, and there's definitely no focus fixups happening so 1 and 2 will both be an improvement.

This is a good point - it's quite broken now, so either option is an improvement.

1. Do focus fixup asynchronously.
2. Don't do focus fixup in this case.

The pro of #1 are that it still tries to do the focus fixup. The con is that the behavior might be funny. E.g. if Javascript first removes the open attribute and then moves focus itself. The focus will then be moved twice.

The pro of #2 is that there are no async shenanigans like #1. The con is that focus might not be fixed up in some cases. If we went this route, at least in Chromium, we'd definitely keep our existing console error that says "please don't touch the open attribute - use .close()". So hopefully this is a corner case only. (My preference, because it feels lower risk, is #2.)

Neither option is wonderful. But as long as browsers warn developers that changing the open attribute directly is a "bad thing to do", so that hopefully most people are calling .close(), option #2 seems perhaps better to me. Changing focus asynchronously might lead to other weird bugs, if for example the developer changes focus after removing the open attribute or something. So I'm ok with option #2 here.

[keithamus]

if for example the developer changes focus after removing the open attribute or something

Potentially more complex but we could focus the previously focused element only if the focus hasn't changed. With open removed the next focus would be the container or document right? So if it's neither of those by the time we're ready to run focus steps, then skip running focus steps.

[mfreed7]

if for example the developer changes focus after removing the open attribute or something

Potentially more complex but we could focus the previously focused element only if the focus hasn't changed. With open removed the next focus would be the container or document right? So if it's neither of those by the time we're ready to run focus steps, then skip running focus steps.

Yes, that's possible, but it gets kind of tricky. That relies on focus fixup running, and we'd have to keep track of whether focus has moved (via JS or user click?) before or after that point, right? That option just feels like it's much more ripe for additional bugs. Remember that this whole discussion is about a corner-case (monkeying with the open attribute) that we missed before. I'd hate to add more of those.

For Chromium at least, we have (and will continue to have) a large console error that shows up in any of these scenarios, telling developers that they really shouldn't be changing open, or they'll get unexpected results. I'd like to rely on that, as much as we can. Perhaps there's a future where we deprecate the open attribute even.