Skip to content

ClosureFrames: one shared trigger again; per-pair triggers lost real clicks - #514

Merged
Frotty merged 1 commit into
masterfrom
fix/frame-click-shared-trigger
Oct 21, 2026
Merged

Frotty merged 1 commit into
masterfrom
fix/frame-click-shared-trigger

Conversation

@Frotty

@Frotty Frotty commented Oct 21, 2026

Copy link
Copy Markdown
Member

Problem

Since #513, every (frame, event) pair got its own trigger, created on demand, and onEvent only dispatched when the firing trigger matched the stored one. With that, real mouse clicks on a button created inside another button's click handler never reached their listeners. In Castle Fight this broke the first button of each newly opened screen: the Game Start dialog's Campaign button (opened from the source notice's OK) and the draft's Ready button (opened from Ladder Modes).

BlzFrameClick on the same buttons still dispatched correctly, so only a real click shows the loss. An in-engine probe with BlzFrameClick (button A creating B1/B2 in its click handler) passed on #513.

Fix

Back to one shared FRAMEHANDLE_TRIGGER for all frame events, registered once per (frame, event) pair (tracked in FRAME_EVENT_REGISTERED). Kept from #513:

  • no double registration, so a handler never runs twice per click
  • the per-dispatch cursor, so a listener may destroy itself or a sibling during dispatch

Removed: per-pair triggers, the trigger-identity check in onEvent, and trigger retirement. WC3 has no per-event unregister, so a pair stays registered after its last listener goes away. On Jass, a new frame that reuses a destroyed frame's handle id is therefore not registered again, as before #513. On Lua handle ids are never reused.

Validation

  • In game (Castle Fight dev map): with this ClosureFrames, the Campaign and Ready buttons work again; with Fix stdlib bugs found by audit, with regression tests #513's they do not. Checked by real mouse click.
  • grill test --quiet: 747/747.
  • Tests: aPairWhoseListenersWentAwayStaysRegistered replaces the fresh-trigger test, and the two retired-trigger tests are gone. New aListenerLinkedDuringADispatchGetsItsOwnFrameEvents covers the dialog-opens-dialog shape.

Gap

No automated test reproduces the loss: the unit hooks call the dispatcher directly, and BlzFrameClick worked even on #513. The new test passes on #513 too; it documents the scenario but cannot catch the engine-level failure.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-21T14:30:22.054635Z aeb5100 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aeb510032e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +135 to +136
if LoadBoolean(FRAME_EVENT_REGISTERED, id, eventId)
return false

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Re-register reused Jass frame handles

On the Jass target, frame handle IDs are reused after BlzDestroyFrame, but this entry is never removed when the old frame's last listener is destroyed. If a later frame receives the same ID and event type, this branch returns false, addListenerForFrame skips registerFrameEvent, and the new frame's exported onClick/other event listeners never receive real events. The previous fresh-trigger behavior explicitly handled this case, so the shared-trigger solution must distinguish a new frame lifetime rather than permanently treating the numeric pair as registered.

AGENTS.md reference: AGENTS.md:L12-L12

Useful? React with 👍 / 👎.

@Frotty
Frotty merged commit f0dcafc into master Oct 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant