Repository navigation
fix(ui-components): cancel SSMode's deferred listener timer on destroy - #1035
Merged
Merged
Conversation
`toggleSSMode` defers attaching the document click/touchstart listeners by 1ms, working around the activating click firing them immediately. The timer id was never stored, so `ngOnDestroy` had no way to cancel it. Destroying the component inside that window runs the removals first, while nothing is attached yet, and the timer then attaches both listeners to a component that no longer exists. Neither removal path can reach them afterwards - `ngOnDestroy` has already run and the toggle-off branch is unreachable - so they stay on `document` for the life of the page and every later click calls `exitFullscreen()`. MakePictureComponent carries the identical workaround and was hardened for exactly this in ef594ae, from review feedback on HSF#867. That fix covered one of the two copies; this is the other. Store the timer id and clear it in `ngOnDestroy`, and also when the user toggles screenshot mode off while it is still pending. Mirrors make-picture so the two stay in step. Added tests covering that no listener is attached after destroy and that a normal toggle still attaches both. The first fails before this change. Fixes HSF#1034
|
🚀 Preview deployed: http://phoenix-pr-1035.surge.sh Built from 454ab44. |
This branch was successfully deployed
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.
Fixes #1034
Problem
toggleSSModedefers attaching the documentclick/touchstartlisteners by 1ms, working around the activating click firing them immediately. The timer id was never stored, songOnDestroyhad no way to cancel it.Destroy the component inside that window and the order is:
ngOnDestroycallsremoveEventListener— no-ops, nothing is attached yet.addEventListenerfor both events.The listeners are now owned by a destroyed component, and neither removal path can reach them:
ngOnDestroyhas already run, andtoggleSSMode's else-branch is unreachable once the component is gone. They stay ondocumentfor the life of the page, and since the handler is() => document.exitFullscreen?.(), every later click anywhere in the app drops the user out of fullscreen.This is a half-applied fix
MakePictureComponenthas the identical workaround and was hardened for exactly this inef594ae— "store setTimeout id and clear it in ngOnDestroy to prevent re-adding listeners after destroy" — from review feedback on #867.SSModeComponentwas never updated, so that fix covered one of the two copies. This PR is the other one, and deliberately mirrors make-picture's shape so the two stay in step.Fix
ssTimeoutId, cleared inside the callback when it fires normally.ngOnDestroy, before theremoveEventListenercalls.The shared
clearSSTimeout()helper keeps the null-check in one place.Tests
A note on how these are written, since it cost me a wrong turn:
onDocumentClickis an arrow-function class field, so it is a stable per-instance reference andremoveEventListenergenuinely matches it. A test that merely counts add-vs-remove calls therefore nets to zero and passes even against the bug. The assertion has to be about ordering — that an attach happens afterngOnDestroy— which is what these tests check.I also dropped an assertion on
jest.getTimerCount(); zone.js keeps its own timers pending in a TestBed fixture (it returned 6), so it is not a meaningful signal here.Verification
phoenix-ngsuite: 62 suites / 223 tests passed, 0 failures.eslintclean on thess-modedirectory.