fix: handle 0s transition duration in close animations and improve st… #64
Conversation
…yle cleanup - Update `AdvantageWrapper.animateClose` to detect if a transition will actually occur. - Immediately execute cleanup callbacks if `transition-duration` is 0s or `transition-property` is 'none', preventing the wrapper from getting stuck waiting for a `transitionend` event that never fires. - Fix `WelcomePage` format close logic to also handle 0s transition durations correctly. - Update `TopScroll` format to remove CSS properties (height, animation duration) only *after* the closing animation completes (via callback), ensuring smooth transitions. - Add `if (this.currentFormat)` check in `animateClose` to prevent race conditions if a new format is requested while the previous one is closing. - Allow `WelcomePage` transitionfix: handle 0s transition duration in close animations and improve style cleanup - Update `AdvantageWrapper.animateClose` to detect if a transition will actually occur. - Immediately execute cleanup callbacks if `transition-duration` is 0s or `transition-property` is 'none', preventing the wrapper from getting stuck waiting for a `transitionend` event that never fires. - Fix `WelcomePage` format close logic to also handle 0s transition durations correctly. - Update `TopScroll` format to remove CSS properties (height, animation duration) only *after* the closing animation completes (via callback), ensuring smooth transitions. - Add `if (this.currentFormat)` check in `animateClose` to prevent race conditions if a new format is requested while the previous one is closing. - Allow `WelcomePage` transition
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical issue where ad formats could become stuck in a "closing" state when transition animations are disabled (duration = 0s) or when users have reduced motion preferences. The fix detects zero-duration transitions and executes cleanup immediately rather than waiting for a transitionend event that never fires.
Key Changes:
- Enhanced
animateClosein wrapper.ts to detect transition duration and skip event listeners when duration is 0 - Applied similar zero-duration detection to WelcomePage format's close method
- Made WelcomePage's transition duration configurable via CSS variable
- Added test page to verify zero-duration close behavior
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/advantage/wrapper.ts | Added transition detection logic to animateClose method to handle zero-duration animations with immediate cleanup |
| src/advantage/formats/welcomepage.ts | Implemented zero-duration transition detection in close method, added support for configurable animation duration via CSS variable, and added CSS variable cleanup in reset |
| src/advantage/formats/welcomepage.css | Updated transition property to use CSS variable for configurable duration |
| playground/local-dev/welcomepage-test.ts | Added test configuration file for WelcomePage with zero animation duration |
| playground/local-dev/welcomepage-test.html | Added test HTML page to manually verify zero-duration close behavior |
| playground/local-dev/config.ts | Added closeButtonAnimationDuration option to TopScroll format configuration for testing |
Comments suppressed due to low confidence (1)
src/advantage/wrapper.ts:501
- The transitionend event listener is added without removing it in case of errors or edge cases. While the
once: trueoption ensures it fires only once, if a transition is cancelled or interrupted (e.g., element removed from DOM, CSS changed), the callback might never fire. Consider adding a safety timeout or checking if the element is still in the DOM before setting display to none.
this.addEventListener(
"transitionend",
() => {
// If a new format has started (currentFormat is set), abort cleanup
if (this.currentFormat) {
return;
}
this.style.display = "none";
this.classList.remove("animate");
if (callback) {
callback();
}
},
{ once: true }
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (container) { | ||
| const computedStyle = window.getComputedStyle(container); | ||
| const transitionProperty = computedStyle.transitionProperty; | ||
| const transitionDuration = computedStyle.transitionDuration; | ||
|
|
||
| const hasTransition = | ||
| transitionProperty !== "none" && | ||
| transitionDuration.split(",").some((d) => parseFloat(d) > 0); | ||
|
|
||
| if (!hasTransition) { | ||
| wrapper.style.display = "none"; | ||
| wrapper.classList.remove("show"); | ||
| wrapper.style.height = "0px"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The transition detection logic is duplicated between wrapper.ts (lines 467-475) and welcomepage.ts (lines 144-150). This creates a maintenance burden where bugs or improvements must be applied in multiple places. Consider extracting this logic into a shared utility function that both can use.
| return new Promise((resolve) => { | ||
| // Inser the CSS for the top scroll format | ||
| wrapper.insertCSS(varsCSS.concat(welcomepageCSS)); | ||
| // Set the styles for the ad iframe | ||
| if (ad) { | ||
| setDimensionsUntilAdvantageAdSlot(ad); | ||
|
|
||
| if (config.closeButtonAnimationDuration !== undefined) { | ||
| wrapper.style.setProperty( | ||
| "--adv-wp-transition-duration", | ||
| `${config.closeButtonAnimationDuration}s` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The call to setDimensionsUntilAdvantageAdSlot(ad) was removed from the setup function. All other formats (topscroll.ts line 51, midscroll.ts line 15, multi-midscroll-base.ts line 69) call this function during setup, and welcomepage.ts still calls resetDimensionsUntilAdvantageAdSlot(ad) in its reset function (line 126). This removal appears unintentional and could break the ad dimensions setup for the WelcomePage format. The function call should be restored.
| if (!hasTransition) { | ||
| this.classList.remove("animate"); | ||
| this.style.height = "0px"; | ||
| this.style.display = "none"; |
There was a problem hiding this comment.
In the immediate cleanup path (no transition), the code sets style.height = "0px" before style.display = "none". This could cause a brief visual flicker if the element is still visible when height is set to 0. Consider setting display = "none" first, or remove the height setting since it's redundant when display is none. The transitioned path (line 502) sets height after setting up the listener, which is correct for animation.
| if (container) { | ||
| const computedStyle = window.getComputedStyle(container); | ||
| const transitionProperty = computedStyle.transitionProperty; | ||
| const transitionDuration = computedStyle.transitionDuration; | ||
|
|
||
| const hasTransition = | ||
| transitionProperty !== "none" && | ||
| transitionDuration.split(",").some((d) => parseFloat(d) > 0); | ||
|
|
||
| if (!hasTransition) { | ||
| wrapper.style.display = "none"; | ||
| wrapper.classList.remove("show"); | ||
| wrapper.style.height = "0px"; | ||
| return; | ||
| } | ||
|
|
||
| container.addEventListener("transitionend", handleTransitionEnd); | ||
| } | ||
|
|
There was a problem hiding this comment.
When the container is not found (null or undefined), the function silently continues to execute lines 162-163, which set wrapper styles. This could lead to unexpected behavior where the wrapper's height is set to 0px and the "show" class is removed even though no container was found. Consider returning early or handling the null case explicitly, similar to how the hasTransition check returns early on line 156.
|
What copilot said :) |
Fix: Handle 0s transition duration & improve style cleanup on close
Summary
This PR addresses a critical issue where ad formats could get stuck in a "closing" state if the close animation duration was set to 0 (or if the user prefers reduced motion). It ensures that cleanup callbacks are executed immediately when no transition occurs, rather than waiting for a
transitionendevent that never fires.Additionally, it refines the
TopScrollclose behavior to prevent visual glitches by deferring style resets until after the closing animation completes.Key Changes
Core Wrapper Logic (
src/advantage/wrapper.ts)animateClosenow checksgetComputedStylefortransition-durationandtransition-property.0sor the property isnone, the element is hidden and the callback is executed immediately, bypassing thetransitionendlistener.if (this.currentFormat)) inside the transition callback to abort cleanup if a new format has started while the previous one was animating out.animateClosenow accepts an optionalcallbackparameter to run logic after the animation (or immediate close) finishes.Format Updates
TopScroll (
src/advantage/formats/topscroll.ts)closeto use the newanimateClosecallback.resetCSS()are now removed only after the animation finishes. This prevents the ad from snapping to a different state before sliding out.WelcomePage (
src/advantage/formats/welcomepage.ts&.css)transitionend.closeButtonAnimationDurationby mapping it to a CSS variable (--adv-wp-transition-duration), allowing the animation speed to be controlled or disabled via config.Testing
playground/local-dev/welcomepage-test.html) to verify thatWelcomePagecloses immediately and correctly whencloseButtonAnimationDurationis set to0.Checklist
TopScrollformat animates out smoothly before resetting styles.TopScrollformat closes immediately if animation duration is 0.WelcomePageformat closes immediately if animation duration is 0.Summary
This PR addresses a critical issue where ad formats could get stuck in a "closing" state if the close animation duration was set to 0 (or if the user prefers reduced motion). It ensures that cleanup callbacks are executed immediately when no transition occurs, rather than waiting for a
transitionendevent that never fires.Additionally, it refines the
TopScrollclose behavior to prevent visual glitches by deferring style resets until after the closing animation completes.Key Changes
Core Wrapper Logic (
src/advantage/wrapper.ts)animateClosenow checksgetComputedStylefortransition-durationandtransition-property.0sor the property isnone, the element is hidden and the callback is executed immediately, bypassing thetransitionendlistener.if (this.currentFormat)) inside the transition callback to abort cleanup if a new format has started while the previous one was animating out.animateClosenow accepts an optionalcallbackparameter to run logic after the animation (or immediate close) finishes.Format Updates
TopScroll (
src/advantage/formats/topscroll.ts)closeto use the newanimateClosecallback.resetCSS()are now removed only after the animation finishes. This prevents the ad from snapping to a different state before sliding out.WelcomePage (
src/advantage/formats/welcomepage.ts&.css)transitionend.closeButtonAnimationDurationby mapping it to a CSS variable (--adv-wp-transition-duration), allowing the animation speed to be controlled or disabled via config.Testing
playground/local-dev/welcomepage-test.html) to verify thatWelcomePagecloses immediately and correctly whencloseButtonAnimationDurationis set to0.Checklist
TopScrollformat animates out smoothly before resetting styles.TopScrollformat closes immediately if animation duration is 0.WelcomePageformat closes immediately if animation duration is 0.