Skip to content
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

Add attribute changed steps to dialog element #10954

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented Jan 27, 2025

Add attribute changed steps to dialog element

When the dialogs open attribute is removed:

  1. Remove dialog from the document's open dialogs list.
  2. Destroy and nullify dialog's close watcher

This also adds an assertion to the start of
'set the dialog close watcher' that dialog's close watcher is null.

Fixes #10953

(See WHATWG Working Mode: Changes for more details.)


/interactive-elements.html ( diff )

@lukewarlow lukewarlow marked this pull request as draft January 27, 2025 20:18
When the dialogs open attribute is removed:

1. Remove dialog from the document's open dialogs list.
2. Destroy and nullify dialog's close watcher

This also adds an assertion to the start of
'set the dialog close watcher' that dialog's close watcher is null.
@lukewarlow lukewarlow force-pushed the remove-dialog-open-clear-list branch from b33a63f to 21886e7 Compare January 27, 2025 20:28
@lukewarlow lukewarlow changed the title Remove dialog from open dialogs list when open attribute removed Add attribute changed steps to dialog element Jan 27, 2025
@lukewarlow lukewarlow marked this pull request as ready for review January 27, 2025 20:31
@domenic
Copy link
Member

domenic commented Jan 28, 2025

This test would also pass if we removed the assert, i.e. fixed the regression introduced recently.

Marking "do not merge yet" until we get more clarity in #10953 as to what the right path forward is.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing "do not merge yet" as I'm convinced by the arguments in #10953, but of course we still need to check all the boxes to merge.

For the tests box, could you point to more tests that enforce this behavior that aren't crash tests? Since assertions are actually comments; they don't cause crashes.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic domenic added topic: dialog The <dialog> element and removed do not merge yet Pull request must not be merged per rationale in comment labels Jan 30, 2025
@lukewarlow
Copy link
Member Author

I've opened web-platform-tests/wpt#50393 which fails in a browser which follows the current spec:

  • I tested by locally modifiying chrome to use an ordered list for the open dialogs list, and removed the attribute changed set. In that build both the escape case and the click outside case result in the dialog being closed when it shouldn't be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: dialog The <dialog> element
Development

Successfully merging this pull request may close these issues.

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