Repository navigation
UI: pin the keyboard unsubscribe, document what a shortcut does not do - #1733
Merged
Merged
Conversation
Follow-up to #1732. Thirteen mutations against its suite; it caught eight. The one worth fixing is that deleting all three `off()` calls in `onDeactivateEvent` leaves every test green. `keyboardActive` already gates both handlers, so dropping the unsubscribe changes no BEHAVIOUR at all, which is exactly why the behavioural tests cannot see it: what it changes is that each add and remove cycle leaves two listeners on the global bus, and every later key press walks all of them. Measured at twenty leaked KEYDOWN listeners over twenty cycles. The new test counts net subscriptions, which is the only thing that notices. The four other survivors are guards no current path reaches rather than defects, so they stay: the repeat suppression is already covered by the sibling `!this.released`, a default `bindKey` of -1 cannot equal a real keycode, an empty string cannot equal an action name, and nothing activates a button twice without deactivating it. The docs gained the two things a reader would otherwise learn at runtime. A shortcut calls `onClick()` with NO pointer, so a handler written against a pointer, reading `event.gameX`, throws the first time someone presses the key: that is the upgrade hazard for buttons that predate this, and it is now the first thing the skill says. And losing focus cancels a held key WITHOUT calling `onRelease()`, so code using the pair as press and depress has to stop on `event.BLUR` as well or it keeps firing while the window is away. Also corrects "respect isClickable" on the property doc: it gates the press, not the release, which is what stops a button disabled mid-hold from sticking down. Their own test pins that, the doc said otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t
The entry from #1732 describes the feature but not the two things a reader has to act on, and both only bite buttons that already exist. A shortcut calls the handlers with NO pointer, so an `onClick(event)` written back when only a pointer could press it throws the first time someone uses the key. And losing focus cancels a held key without calling `onRelease()`, so a button that fires while held keeps firing unless it also stops on `event.BLUR`. Extended rather than added as a second entry: 20.9 is unreleased, it is the same change, and @snowyukitty's credit stays on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1732. Thirteen mutations against its suite; it caught eight, which is a good result. One survivor is worth fixing and the rest are guards rather than defects.
The surviving mutation: the unsubscribe is untested
Deleting all three
off()calls inonDeactivateEventleaves all 18 tests green.The reason is worth stating, because it is why no behavioural test could ever catch it:
onDeactivateEventalso clearskeyboardActive, and both handlers return early on that. So removing the unsubscribe changes no behaviour at all. What it changes is that every add and remove cycle leaves two listeners on the global event bus, and every later key press walks all of them.Measured: 20 leaked KEYDOWN listeners over 20 add/remove cycles. For a game that opens and closes menus, that grows without bound.
The new test counts net
addListener/removeListenercalls, which is the only thing that notices. It passes as-is and fails with theoff()calls removed.The four survivors left alone
Guards that no current path can reach, kept as cheap depth rather than removed:
keyboardKey !== undefinedinkeyDown!this.releasedalready blocks repeatsbindKey >= 0-1cannot equal a real keycodebindKey.length > 0keyboardActiveearly return inonActivateEventDocumentation
Two things a reader would otherwise find out at runtime:
A shortcut calls
onClick()with noPointer. This is the upgrade hazard for every button written before #1732: a handler that readsevent.gameXworked for years because only a pointer could press it, and now throws the first time someone presses the key. Verified that it does throw, not merely readundefined. It is the first thing the skill section says.Losing focus cancels a held key without calling
onRelease(). Deliberate and pinned by a test in #1732, but undocumented. Code using the pair as press and depress, firing while held, has to stop onevent.BLURtoo or it keeps firing while the window is away.Also corrects the
bindKeyproperty doc, which claimed shortcuts "respect isClickable". They respect it on the press, not the release: a button disabled while its key is held still releases, which is what stops it sticking down. #1732's own test pins that behaviour; the doc said otherwise.Verification
pnpm lint0 errors (175 warnings, unchanged baseline),tsc --noEmitclean,check-doc-readmegreen🤖 Generated with Claude Code
https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t