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

avm2: Access Event.target in dispatchEvent #19236

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

Lord-McSweeney
Copy link
Collaborator

Like #19127, but clones the Event in EventDispatcher.dispatchEvent instead of events::dispatch_event.

Copy link
Collaborator

@adrian17 adrian17 left a comment

Choose a reason for hiding this comment

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

To be sure, this means the clone logic will never trigger if we (as in, native code) are the ones doing dispatching. I'm assuming we're considering it safe?

Can you please also copy the test from my PR? It test a more real-world case and a getter being hit.

@Lord-McSweeney Lord-McSweeney added A-avm2 Area: AVM2 (ActionScript 3) T-fix Type: Bug fix (in something that's supposed to work already) labels Jan 16, 2025
@Lord-McSweeney
Copy link
Collaborator Author

To be sure, this means the clone logic will never trigger if we (as in, native code) are the ones doing dispatching. I'm assuming we're considering it safe?

I tested it- none of the "automatically" dispatched events (addedToStage, enterFrame, etc) are mutable at all, so event handlers can't modify them anyway.

Can you please also copy the test from my PR? It test a more real-world case and a getter being hit.

I was going to ask you :P

@Lord-McSweeney Lord-McSweeney force-pushed the avm2-event-clone-target branch from 1c23364 to e4d5f7e Compare January 16, 2025 01:24
@Lord-McSweeney Lord-McSweeney enabled auto-merge (rebase) January 16, 2025 01:24
auto-merge was automatically disabled January 16, 2025 01:37

Rebase failed

@torokati44 torokati44 force-pushed the avm2-event-clone-target branch from e4d5f7e to fc217ab Compare January 16, 2025 02:00
@torokati44 torokati44 enabled auto-merge (rebase) January 16, 2025 02:01
@torokati44
Copy link
Member

GitHub had a brainfart and thought that a no-op rebase would fail, so I force-pushed the last commit with the same changes, hopefully it will work this time.

auto-merge was automatically disabled January 16, 2025 02:12

Rebase failed

@torokati44
Copy link
Member

Narrator: It did not.
What the heck?

…ine whether to call clone

Test written by Adrian17
@torokati44 torokati44 force-pushed the avm2-event-clone-target branch from fc217ab to 765576b Compare January 16, 2025 10:27
@torokati44 torokati44 enabled auto-merge (rebase) January 16, 2025 10:28
@torokati44 torokati44 merged commit 7bcec28 into ruffle-rs:master Jan 16, 2025
21 of 22 checks passed
@torokati44
Copy link
Member

Third time's the charm! 🎉
BTW the issue was https://www.githubstatus.com/incidents/78wybrzyv0wf, referencing https://github.com/orgs/community/discussions/149282.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-avm2 Area: AVM2 (ActionScript 3) T-fix Type: Bug fix (in something that's supposed to work already)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants