-
Notifications
You must be signed in to change notification settings - Fork 295
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
Update to use new "report an exception" algorithm in HTML #1303
Conversation
It's necessary to specify which global these are reported to. This seems to be the event listener callback's realm's global in the event listener case, and the custom element constructor's realm, based on a combination of consistency with the related sites in HTML and browser behavior -- though browser behavior for custom elements isn't consistent in the (unusual) case of custom elements across realms. Part of whatwg/html#10516.
@domenic kind of on the margin of editorial but consistent with the changes made to HTML in whatwg/html#10404; wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we shouldn't have too high a bar for this change, as it moves us toward consistency and the previous version was just not well-specified at all.
Web platform tests and browser bugs would be nice if you have the time, but I don't want to require them.
I'll not merge immediately since it seems @annevk is back from vacation and he's the editor.
I'm going to push a minor fixup and I don't think we should merge this with the "Editorial:" prefix, but otherwise I'm okay with going ahead with this. |
Also, it's really great to see this cleaned up! Thanks Jeremy. |
One thing to flag: just found the old discussion at WICG/webcomponents#635 which is somewhat at odds with this, but is more consistent with how callbacks of other sorts, like the event listeners in this same PR, work. |
To be clear, you mean that your current PR is more consistent in that it applies the same rules to all callbacks? Or, do you mean the ideas suggested in that old discussion are more consistent? I think it's the former, in which case, I think our current conclusions are good and I'm happy to stick with them. But if I've misunderstood then let me know. |
Yes, the former. I agree, I'm happy with the current conclusions. (Having gone home to my Mac, I can now also say that this seems to match Safari's behavior -- though Safari's muting logic is so aggressive that I get |
Thanks again! |
This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17
This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635}
This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635}
…exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Jeremy Roman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342638}
This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635}
…exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Jeremy Roman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342638}
…m element upgrade, a=testonly Automatic update from web-platform-tests Add WPT for error reporting during custom element upgrade This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635} -- wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c wpt-pr: 47632
…spatch when multiple globals exist, a=testonly Automatic update from web-platform-tests Add WPT for error events during event dispatch when multiple globals exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Jeremy Roman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342638} -- wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21 wpt-pr: 47636
…m element upgrade, a=testonly Automatic update from web-platform-tests Add WPT for error reporting during custom element upgrade This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635} -- wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c wpt-pr: 47632
…spatch when multiple globals exist, a=testonly Automatic update from web-platform-tests Add WPT for error events during event dispatch when multiple globals exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Jeremy Roman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342638} -- wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21 wpt-pr: 47636
This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635}
…exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Jeremy Roman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342638}
…m element upgrade, a=testonly Automatic update from web-platform-tests Add WPT for error reporting during custom element upgrade This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342635} -- wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c wpt-pr: 47632
…spatch when multiple globals exist, a=testonly Automatic update from web-platform-tests Add WPT for error events during event dispatch when multiple globals exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Jeremy Roman <[email protected]> Cr-Commit-Position: refs/heads/main@{#1342638} -- wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21 wpt-pr: 47636
…m element upgrade, a=testonly Automatic update from web-platform-tests Add WPT for error reporting during custom element upgrade This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <jbromanchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1342635} -- wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c wpt-pr: 47632 UltraBlame original commit: 3346b6f0e9a9650f56a331cd6257ae9b73411acc
…spatch when multiple globals exist, a=testonly Automatic update from web-platform-tests Add WPT for error events during event dispatch when multiple globals exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <masonfchromium.org> Commit-Queue: Jeremy Roman <jbromanchromium.org> Cr-Commit-Position: refs/heads/main{#1342638} -- wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21 wpt-pr: 47636 UltraBlame original commit: 53550a41ef5e8c49b5d50c34de67aebc5aca4fb2
…m element upgrade, a=testonly Automatic update from web-platform-tests Add WPT for error reporting during custom element upgrade This test currently fails in Chromium. 3/4 subtests fail with: error event should target definition's global but instead targets test harness global" See also: whatwg/dom#1303 Bug: 359909516 Change-Id: I0cc589735d9f8c8ec2c4b55c89aabdb4f7500e17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5789341 Commit-Queue: Jeremy Roman <jbromanchromium.org> Reviewed-by: Mason Freed <masonfchromium.org> Cr-Commit-Position: refs/heads/main{#1342635} -- wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c wpt-pr: 47632 UltraBlame original commit: 3346b6f0e9a9650f56a331cd6257ae9b73411acc
…spatch when multiple globals exist, a=testonly Automatic update from web-platform-tests Add WPT for error events during event dispatch when multiple globals exist See also whatwg/dom#1303 Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046 Reviewed-by: Mason Freed <masonfchromium.org> Commit-Queue: Jeremy Roman <jbromanchromium.org> Cr-Commit-Position: refs/heads/main{#1342638} -- wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21 wpt-pr: 47636 UltraBlame original commit: 53550a41ef5e8c49b5d50c34de67aebc5aca4fb2
It's necessary to specify which global these are reported to. This seems to be the event listener callback's realm's global in the event listener case, and the custom element constructor's realm, based on a combination of consistency with the related sites in HTML and browser behavior -- though browser behavior for custom elements isn't consistent in the (unusual) case of custom elements across realms.
Part of whatwg/html#10516.
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff