Fix dialog `requestClose()` to handle edge case #10983

lukewarlow · 2025-02-02T23:35:46Z

Fix dialog requestClose() to handle edge case

Dialog requestClose() now directly calls close the dialog if dialog's close watcher is null.

This replaces a previous assert that's wrong.

At least two implementers are interested (and none opposed):
- …
- …
Tests are written and can be reviewed and commented upon at:
- Add requestClose() subtests for initially open dialog web-platform-tests/wpt#50432
Implementation bugs are filed:
- Chromium: …
- Gecko: …
- WebKit: …
Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
MDN issue is filed: N/A
The top of this comment includes a clear commit message to use.

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

domenic

I guess this is more temporary patches since we haven't fixed the asymmetry between setting open and calling show().

@mfreed7 PTAL?

lukewarlow · 2025-02-03T12:05:49Z

I guess this is more temporary patches since we haven't fixed the asymmetry between setting open and calling show().

Yeah idk what the end result we want is, perhaps we can change this to an assert in future and make sure close watcher is created when open is added but for now this seems like the minimal fix to prevent a crash.

I think it's potentially worth a discussion at whatnot about what we envisage the end state to be for the open attribute because it causes many an issue so it might be worth spending time fully fixing it up.

Dialog requestClose() now directly calls close the dialog if dialog's close watcher is null. This replaces a previous assert that's wrong.

mfreed7 · 2025-02-07T19:40:29Z

So I just put up a patch in Chromium to fix (I think all?) of these crasher/assert cases. Instead of relaxing the asserts, it adds attribute change steps for open that construct the close watcher and add the dialog to the open dialogs list when the open attribute is added to the <dialog>. WDYT about that approach, instead of the one in this PR?

lukewarlow · 2025-02-07T19:52:00Z

Provided there's no unexpected behaviour (such as the dialog being added to the list (set in chrome so might not be noticed) twice when calling show() and showModal()) I think that's preferable. Would solve the initially open case too.

For the sake of completeness I think it should match :open too. I know that's not a spec concern but it would be good to add a WPT to ensure it's synced to the attribute correctly. (Likewise when removing the open attribute)

mfreed7 · 2025-02-10T22:20:19Z

Provided there's no unexpected behaviour (such as the dialog being added to the list (set in chrome so might not be noticed) twice when calling show() and showModal()) I think that's preferable. Would solve the initially open case too.

I added (back) DCHECKs everywhere I'm modifying the dialog list, to make sure it's being managed correctly. All existing tests pass, so I think this approach is looking good. Would you be interested in tweaking this PR (or #10954) accordingly? I can too, I'm just kind of swamped so it might take me some time.

For the sake of completeness I think it should match :open too. I know that's not a spec concern but it would be good to add a WPT to ensure it's synced to the attribute correctly. (Likewise when removing the open attribute)

I'll add :open checks to the appropriate tests.

lukewarlow · 2025-02-10T22:25:25Z

I'll close this PR and update the other one to include all the proposed changes.

See this request: whatwg/html#10983 (comment) plus this comment: https://chromium-review.googlesource.com/c/chromium/src/+/6245064/comments/e932d31a_c996d18e This just adds more testing of dialog closedby and requestClose, to make sure close watchers are still set up, the `:open` pseudo class works properly, events get fired correctly, etc., when the dialog `open` attribute is manually added and removed. Bug: 376516550 Change-Id: Ibdee09c5f722db2ce97c64c38c39726c1503e1d2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6253798 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Reviewed-by: Luke <[email protected]> Cr-Commit-Position: refs/heads/main@{#1422074}

…estClose, a=testonly Automatic update from web-platform-tests Improve testing dialog closedby and requestClose See this request: whatwg/html#10983 (comment) plus this comment: https://chromium-review.googlesource.com/c/chromium/src/+/6245064/comments/e932d31a_c996d18e This just adds more testing of dialog closedby and requestClose, to make sure close watchers are still set up, the `:open` pseudo class works properly, events get fired correctly, etc., when the dialog `open` attribute is manually added and removed. Bug: 376516550 Change-Id: Ibdee09c5f722db2ce97c64c38c39726c1503e1d2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6253798 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Reviewed-by: Luke <[email protected]> Cr-Commit-Position: refs/heads/main@{#1422074} -- wpt-commits: 57d6088fe0481e9662763d2147831219e692d3bc wpt-pr: 50808

…estClose, a=testonly Automatic update from web-platform-tests Improve testing dialog closedby and requestClose See this request: whatwg/html#10983 (comment) plus this comment: https://chromium-review.googlesource.com/c/chromium/src/+/6245064/comments/e932d31a_c996d18e This just adds more testing of dialog closedby and requestClose, to make sure close watchers are still set up, the `:open` pseudo class works properly, events get fired correctly, etc., when the dialog `open` attribute is manually added and removed. Bug: 376516550 Change-Id: Ibdee09c5f722db2ce97c64c38c39726c1503e1d2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6253798 Auto-Submit: Mason Freed <masonfchromium.org> Commit-Queue: Mason Freed <masonfchromium.org> Reviewed-by: Joey Arhar <jarharchromium.org> Reviewed-by: Luke <lwarlowigalia.com> Cr-Commit-Position: refs/heads/main{#1422074} -- wpt-commits: 57d6088fe0481e9662763d2147831219e692d3bc wpt-pr: 50808 UltraBlame original commit: 9668c9902b5818811f24cb50228e71a8fa1ca08e

…estClose, a=testonly Automatic update from web-platform-tests Improve testing dialog closedby and requestClose See this request: whatwg/html#10983 (comment) plus this comment: https://chromium-review.googlesource.com/c/chromium/src/+/6245064/comments/e932d31a_c996d18e This just adds more testing of dialog closedby and requestClose, to make sure close watchers are still set up, the `:open` pseudo class works properly, events get fired correctly, etc., when the dialog `open` attribute is manually added and removed. Bug: 376516550 Change-Id: Ibdee09c5f722db2ce97c64c38c39726c1503e1d2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6253798 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Mason Freed <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Reviewed-by: Luke <[email protected]> Cr-Commit-Position: refs/heads/main@{#1422074} -- wpt-commits: 57d6088fe0481e9662763d2147831219e692d3bc wpt-pr: 50808

lukewarlow marked this pull request as ready for review February 2, 2025 23:53

lukewarlow force-pushed the fix-dialog-requestclose branch from 67f78ff to bc0f5ec Compare February 2, 2025 23:56

domenic added topic: dialog The <dialog> element topic: close watchers labels Feb 3, 2025

domenic approved these changes Feb 3, 2025

View reviewed changes

Fix dialog requestClose() to handle edge case

e2f7801

Dialog requestClose() now directly calls close the dialog if dialog's close watcher is null. This replaces a previous assert that's wrong.

lukewarlow force-pushed the fix-dialog-requestclose branch from bc0f5ec to e2f7801 Compare February 3, 2025 12:20

past mentioned this pull request Feb 7, 2025

Upcoming WHATNOT meeting on 2025-02-06 #10972

Closed

lukewarlow closed this Feb 10, 2025

chromium-wpt-export-bot mentioned this pull request Feb 19, 2025

Improve testing dialog closedby and requestClose web-platform-tests/wpt#50808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix dialog `requestClose()` to handle edge case #10983

Fix dialog `requestClose()` to handle edge case #10983

Uh oh!

lukewarlow commented Feb 2, 2025 •

edited by pr-preview bot

Loading

Uh oh!

domenic left a comment

Uh oh!

lukewarlow commented Feb 3, 2025

Uh oh!

mfreed7 commented Feb 7, 2025

Uh oh!

lukewarlow commented Feb 7, 2025 •

edited

Loading

Uh oh!

mfreed7 commented Feb 10, 2025

Uh oh!

lukewarlow commented Feb 10, 2025

Uh oh!

Uh oh!

Fix dialog requestClose() to handle edge case #10983

Fix dialog requestClose() to handle edge case #10983

Uh oh!

Conversation

lukewarlow commented Feb 2, 2025 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

lukewarlow commented Feb 3, 2025

Uh oh!

mfreed7 commented Feb 7, 2025

Uh oh!

lukewarlow commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfreed7 commented Feb 10, 2025

Uh oh!

lukewarlow commented Feb 10, 2025

Uh oh!

Uh oh!

Fix dialog `requestClose()` to handle edge case #10983

Fix dialog `requestClose()` to handle edge case #10983

lukewarlow commented Feb 2, 2025 •

edited by pr-preview bot

Loading

lukewarlow commented Feb 7, 2025 •

edited

Loading