Skip to content

Dialog requestClose causes several issues with regard to interplay of CloseWatcher #11230

Open
@keithamus

Description

@keithamus

What is the issue with the HTML Standard?

Right now the spec has some fairly wide holes when it comes to requestClose on HTMLDialogElement. As it stands the spec for requestClose steps asserts that the dialog's CloseWatcher is not null, but it can be - fairly trivially so - as the only times CloseWatcher is established are during show() and showModal(). This overlaps quite closely with #10982 and #10953.

Consequently @lukewarlow raised #10954 which adds HTML Element insertion steps and Attribute change steps to Dialog elements which, if the open attribute is present, will also set up the CloseWatcher. However in some cases; for example a disconnected tree or a document without a browsing context, the CloseWatcher may or may not be established, or other asserts may fail. We cans see this in web-platform-tests/wpt#52020 which tests both of these conditions, and currently causes crashes on Chrome.

Taking a step back slightly, when we look at the interaction requestClose has with CloseWatcher, it's not entirely apparent why requestClose needs CloseWatcher. The "get enabled state" machinery in CloseWatcher was added so that requestClose would work if Dialog's computed closed-by state was None - allowing Dialog requestClose to still work even if the CloseWatcher was disabled. Additionally CloseWatcher's "request to close" algorithm takes a requireHistoryActionActivation boolean, which for light dismiss and requestClose circumvents the CloseWatcher protections around ensuring the cancel event cannot be default prevented. In other words for light dismiss and requestClose can always be default prevented, which also means they never consume a history-user activation.

So to summarise we have a few problems: requestClose & light dismiss have added additional machinery that sometimes disables relevant parts of CloseWatcher but hasn't yet gone far enough to cover edge cases that CloseWatcher is active for the dialog element.

I think we can look at solving this problem in a few ways, but each has some downsides:

  1. Remove asserts that Dialog should have an active close watcher, instead creating one if it doesn't exist.
    • This has issues in that construction of a close watcher may create a new group, so there can be observable differences if you call requestClose on a dialog that was open before it was connected rather than after.
  2. Remove the CloseWatcher mechanics entirely from requestClose, and have it dispatch cancel/close events by itself, without expecting or interacting with the CloseWatcher.
    • This is potentially problematic because requestClose should closely match what CloseWatcher.requestClose does - so the two need to be kept in sync, or we need to abstract both algorithms out into a third, which will likely inherit the complexity they already have. This isn't just spec purity but also implementation complexity.
  3. "Double down" on new mechanics to solve these edge cases, such as incorporating the changes in Fix dialog closedby and requestClose() #10954 and well beyond - including ensuring that CloseWatchers are only sometimes created during insertion/attribute changes.
    • This has some of the same problems as 1 though they're even more subtle, but also some of the same problems as 2 in terms of spec/implementation complexity.

/cc @mfreed7 @lukewarlow @domenic @smaug---- @zcorpan

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions