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

Update to use new "report an exception" algorithm in HTML #1303

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

jeremyroman
Copy link
Contributor

@jeremyroman jeremyroman commented Aug 12, 2024

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

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.
@jeremyroman
Copy link
Contributor Author

@domenic kind of on the margin of editorial but consistent with the changes made to HTML in whatwg/html#10404; wdyt?

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.

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.

@annevk
Copy link
Member

annevk commented Aug 13, 2024

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.

@annevk
Copy link
Member

annevk commented Aug 13, 2024

Also, it's really great to see this cleaned up! Thanks Jeremy.

@jeremyroman jeremyroman changed the title Editorial: Update to use new "report an exception" algorithm in HTML Update to use new "report an exception" algorithm in HTML Aug 14, 2024
@jeremyroman
Copy link
Contributor Author

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.

@domenic
Copy link
Member

domenic commented Aug 15, 2024

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.

@jeremyroman
Copy link
Contributor Author

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 error events with the error censored.)

@annevk annevk merged commit 054356d into whatwg:main Aug 15, 2024
2 checks passed
@annevk
Copy link
Member

annevk commented Aug 15, 2024

Thanks again!

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 16, 2024
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
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 16, 2024
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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 16, 2024
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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 16, 2024
…exist

See also whatwg/dom#1303

Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342638}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 16, 2024
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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 16, 2024
…exist

See also whatwg/dom#1303

Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342638}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 20, 2024
…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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}

--

wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c
wpt-pr: 47632
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 20, 2024
…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 <masonf@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342638}

--

wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21
wpt-pr: 47636
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 21, 2024
…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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}

--

wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c
wpt-pr: 47632
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 21, 2024
…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 <masonf@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342638}

--

wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21
wpt-pr: 47636
Westbrook pushed a commit to Westbrook/wpt that referenced this pull request Aug 21, 2024
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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}
Westbrook pushed a commit to Westbrook/wpt that referenced this pull request Aug 21, 2024
…exist

See also whatwg/dom#1303

Change-Id: Ie54cb8204e41d9e4ba1b972d297a8b43f45585ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5784046
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342638}
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Aug 22, 2024
…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 <jbroman@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342635}

--

wpt-commits: 88f98a55ad995dd0dfe271f34d27344b6fc8ab7c
wpt-pr: 47632
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Aug 22, 2024
…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 <masonf@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342638}

--

wpt-commits: 0a811c51619b14f78fec60ba7dd1603795ca6a21
wpt-pr: 47636
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Aug 23, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Aug 23, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Aug 23, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Aug 23, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants