fix(components/core): animationend and transitionend handlers detect suppressed animations via getComputedStyle#4310
Merged
Blackbaud-SteveBrush merged 8 commits intomainfrom Mar 18, 2026
Conversation
…ssed animations via `getComputedStyle`
Blackbaud-SteveBrush
added
the
risk level (author): 2
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces global noop-animation detection with element-level watchers: removes mimicCssMotionEvent and _skyAnimationsDisabled, adds watchForDisabledCssAnimations/watchForDisabledCssTransitions, and updates animation/transition handlers and tests to emit via microtask when CSS motion is disabled. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Animation/Transition Handler
participant Watcher as watchForDisabledCss* Utility
participant Effect as Angular Effect
participant DOM as DOM Element
participant Emit as Output Emitter
Handler->>Watcher: init(elementRef, destroyRef, trigger, emitter, propertyToTrack?)
Watcher->>Effect: create effect observing trigger
loop on trigger change
Effect->>DOM: getComputedStyle(element)
DOM-->>Effect: animation/transition properties
Effect->>Effect: evaluate isMotionDisabled (name=none OR duration≤0 OR property-specific)
alt motion disabled
Effect->>+Emit: queueMicrotask(emit)
Emit-->>-Effect: emission scheduled
else motion enabled
Effect->>Effect: no microtask emission
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
View your CI Pipeline Execution ↗ for commit a916e69
☁️ Nx Cloud last updated this comment at |
Blackbaud-SteveBrush
changed the title
getComputedStylegetComputedStyle
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts (1)
72-74: Consider removing stale cleanup code.Same as in
animation-handler.spec.ts, theafterEachblock removes thesky-animations-disabledCSS class, but no test in this file adds it. This cleanup appears to be leftover from the previous implementation.Proposed fix
- afterEach(() => { - document.body.classList.remove('sky-animations-disabled'); - }); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts` around lines 72 - 74, The afterEach cleanup that calls document.body.classList.remove('sky-animations-disabled') in transition-handler.spec.ts is stale because no test in this file adds that class; remove the entire afterEach block (the anonymous function passed to afterEach) to avoid unnecessary teardown, ensuring tests rely only on their own setup/teardown and mirroring the change made in animation-handler.spec.ts.libs/components/core/src/lib/modules/animations/shared/utils.ts (1)
9-21: Consider typingelementRefmore strictly.The interface accepts
ElementRefwithout a type parameter, which defaults toanyfornativeElement. SincegetComputedStylerequires anElement, consider usingElementRef<Element>to make the type contract explicit.Proposed fix
interface EmitWhenMotionDisabledArgs { /** The directive's `DestroyRef`, used to suppress emissions after teardown. */ destroyRef: DestroyRef; /** A reference to the host element whose computed styles are inspected. */ - elementRef: ElementRef; + elementRef: ElementRef<Element>; /** The output emitter to call when CSS motion is disabled. */ emitter: OutputEmitterRef<void>; /** A signal that drives motion tracking. Emissions are evaluated whenever this value changes. */ trigger: Signal<unknown>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/core/src/lib/modules/animations/shared/utils.ts` around lines 9 - 21, The interface EmitWhenMotionDisabledArgs currently types elementRef as ElementRef (which makes nativeElement any); tighten the contract by changing elementRef to ElementRef<Element> so callers provide an Element-compatible ref for use with getComputedStyle, and update any call sites that construct EmitWhenMotionDisabledArgs to pass Element-typed ElementRef instances accordingly; ensure usages that call getComputedStyle(elementRef.nativeElement) compile after the change.libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts (1)
55-57: Consider removing stale cleanup code.The
afterEachblock removes thesky-animations-disabledCSS class, but sinceprovideNoopSkyAnimationsis no longer used in these tests and no test adds this class, this cleanup appears to be dead code. Consider removing it to avoid confusion.Proposed fix
- afterEach(() => { - document.body.classList.remove('sky-animations-disabled'); - }); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts` around lines 55 - 57, Remove the stale cleanup in the test file: delete the afterEach block that calls document.body.classList.remove('sky-animations-disabled') since provideNoopSkyAnimations is no longer used and no tests add that class; search for and remove the specific afterEach that references 'sky-animations-disabled' to avoid leaving dead test teardown code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts`:
- Around line 55-57: Remove the stale cleanup in the test file: delete the
afterEach block that calls
document.body.classList.remove('sky-animations-disabled') since
provideNoopSkyAnimations is no longer used and no tests add that class; search
for and remove the specific afterEach that references 'sky-animations-disabled'
to avoid leaving dead test teardown code.
In
`@libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts`:
- Around line 72-74: The afterEach cleanup that calls
document.body.classList.remove('sky-animations-disabled') in
transition-handler.spec.ts is stale because no test in this file adds that
class; remove the entire afterEach block (the anonymous function passed to
afterEach) to avoid unnecessary teardown, ensuring tests rely only on their own
setup/teardown and mirroring the change made in animation-handler.spec.ts.
In `@libs/components/core/src/lib/modules/animations/shared/utils.ts`:
- Around line 9-21: The interface EmitWhenMotionDisabledArgs currently types
elementRef as ElementRef (which makes nativeElement any); tighten the contract
by changing elementRef to ElementRef<Element> so callers provide an
Element-compatible ref for use with getComputedStyle, and update any call sites
that construct EmitWhenMotionDisabledArgs to pass Element-typed ElementRef
instances accordingly; ensure usages that call
getComputedStyle(elementRef.nativeElement) compile after the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd01bdd9-f2cd-4892-bb94-2374d788ec1d
📒 Files selected for processing (6)
libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.tslibs/components/core/src/lib/modules/animations/shared/animation-handler.tslibs/components/core/src/lib/modules/animations/shared/mimic-css-motion-event.tslibs/components/core/src/lib/modules/animations/shared/transition-handler.spec.tslibs/components/core/src/lib/modules/animations/shared/transition-handler.tslibs/components/core/src/lib/modules/animations/shared/utils.ts
💤 Files with no reviewable changes (1)
- libs/components/core/src/lib/modules/animations/shared/mimic-css-motion-event.ts
Collaborator
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates SKY UX’s internal animationend/transitionend handler directives to detect suppressed CSS motion by inspecting element computed styles (rather than relying on the sky-animations-disabled mechanism), and to fall back to microtask emissions when native motion events won’t fire (e.g., transition-property: none !important in Storybook).
Changes:
- Added shared utilities that watch computed CSS animation/transition properties and microtask-emit when motion is disabled.
- Refactored the animation/transition handler directives to use the new computed-style detection (removing the prior global “animations disabled” check).
- Updated unit tests to validate microtask fallback behavior under suppressed animation/transition CSS states.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/components/core/src/lib/modules/animations/shared/utils.ts | New shared helpers that inspect computed styles and microtask-emit when CSS motion is suppressed. |
| libs/components/core/src/lib/modules/animations/shared/transition-handler.ts | Refactors transitionend handling to rely on computed-style suppression detection utilities. |
| libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts | Updates tests to validate suppressed-transition detection and microtask fallback emissions. |
| libs/components/core/src/lib/modules/animations/shared/mimic-css-motion-event.ts | Removes the old global “motion disabled” microtask mimic helper. |
| libs/components/core/src/lib/modules/animations/shared/animation-handler.ts | Refactors animationend handling to rely on computed-style suppression detection utilities. |
| libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts | Updates tests to validate suppressed-animation detection and microtask fallback emissions. |
libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts
Show resolved
Hide resolved
libs/components/core/src/lib/modules/animations/shared/utils.ts
Outdated
Show resolved
Hide resolved
libs/components/core/src/lib/modules/animations/shared/utils.ts
Outdated
Show resolved
Hide resolved
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the core CSS transitionend/animationend handler directives to detect suppressed CSS motion by inspecting computed styles on the host element, ensuring consumers still receive completion events when transitions/animations are globally disabled (e.g., Storybook’s transition-property: none !important).
Changes:
- Added shared utilities to detect disabled CSS transitions/animations via
getComputedStyle()and emit a microtask fallback when suppressed. - Updated transition/animation handler directives to use the new computed-style based detection (removing reliance on
_skyAnimationsDisabled()and the old mimic helper). - Updated unit tests to cover disabled/enabled motion scenarios based on CSS properties.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/components/core/src/lib/modules/animations/shared/utils.ts | New shared computed-style based detection + microtask fallback helpers. |
| libs/components/core/src/lib/modules/animations/shared/transition-handler.ts | Uses the new transition suppression watcher and tracks the property via a computed signal. |
| libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts | Reworks tests to validate behavior under suppressed transitions via CSS properties. |
| libs/components/core/src/lib/modules/animations/shared/animation-handler.ts | Uses the new animation suppression watcher for microtask fallback emission. |
| libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts | Reworks tests to validate behavior under suppressed animations via CSS properties. |
| libs/components/core/src/lib/modules/animations/shared/mimic-css-motion-event.ts | Removes the old “animations disabled” microtask mimic helper. |
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts (1)
45-69:⚠️ Potential issue | 🟡 MinorThis helper makes the new initial-render transition test a false positive.
Because
setupTest()runs the firstdetectChanges()before callers can settransition-property: none, the case at Lines 206-223 is not exercising disabled motion on initial render. It also asserts before a queued microtask would run. Please allow pre-render style setup for that scenario andawait fixture.whenStable()before asserting no emission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts` around lines 45 - 69, The test helper setupTest currently always calls fixture.detectChanges() which forces initial render before callers can set pre-render styles (e.g., transition-property: none) and it asserts before queued microtasks run; change setupTest to accept an option like skipInitialDetect?: boolean (or reuse skipTrackProperty name) so callers can set pre-render styles before the first render, and make setupTest async: only call fixture.detectChanges() when not skipping and after calling detectChanges await fixture.whenStable() before returning; also update tests that check "no emission on initial render" to call setupTest({ skipInitialDetect: true }), set the transition-property style, then call fixture.detectChanges() and await fixture.whenStable() before asserting no emission. Reference functions/fields: setupTest, fixture.detectChanges, fixture.whenStable, handler.setPropertyToTrack, componentRef.setInput('trigger', signal(false)).libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts (1)
40-53:⚠️ Potential issue | 🟡 Minor
setupTest()can’t validate first-render suppression cases.This helper performs the first
detectChanges()before the spec can applyanimation-name: none/animation-duration: 0s, so the case at Lines 108-123 never actually runs with disabled motion on initial render. It also asserts before any queued microtask would flush. Please let that test set initial styles before the first render andawait fixture.whenStable()before asserting no emission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts` around lines 40 - 53, The helper setupTest() currently calls fixture.detectChanges() immediately which prevents tests from applying "animation-name: none"/"animation-duration: 0s" before first render and also skips waiting for microtasks; modify setupTest() (and any callers) to accept an option (e.g., { skipInitialDetect?: true }) or a flag to avoid the immediate detectChanges(), so tests can set initial styles on the TestComponent host (via fixture.nativeElement.style or fixture.componentRef.setInput) before the first render; also update the specs that assert no emission to call await fixture.whenStable() after the first render (or after detectChanges when used) before asserting to ensure queued microtasks have flushed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/components/core/src/lib/modules/animations/shared/utils.ts`:
- Around line 108-117: The effect created around trigger() is inadvertently
depending on propertyToTrack() because isMotionDisabled(style) reads that
signal; to fix, wrap the call to isMotionDisabled(style) inside untracked(() =>
...) so the effect only tracks trigger changes; import untracked from
`@angular/core` and replace the direct isMotionDisabled(style) check with an
untracked-wrapped call (preserving the initialized and isRendered guards) to
prevent unintended effect reruns.
---
Outside diff comments:
In
`@libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.ts`:
- Around line 40-53: The helper setupTest() currently calls
fixture.detectChanges() immediately which prevents tests from applying
"animation-name: none"/"animation-duration: 0s" before first render and also
skips waiting for microtasks; modify setupTest() (and any callers) to accept an
option (e.g., { skipInitialDetect?: true }) or a flag to avoid the immediate
detectChanges(), so tests can set initial styles on the TestComponent host (via
fixture.nativeElement.style or fixture.componentRef.setInput) before the first
render; also update the specs that assert no emission to call await
fixture.whenStable() after the first render (or after detectChanges when used)
before asserting to ensure queued microtasks have flushed.
In
`@libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts`:
- Around line 45-69: The test helper setupTest currently always calls
fixture.detectChanges() which forces initial render before callers can set
pre-render styles (e.g., transition-property: none) and it asserts before queued
microtasks run; change setupTest to accept an option like skipInitialDetect?:
boolean (or reuse skipTrackProperty name) so callers can set pre-render styles
before the first render, and make setupTest async: only call
fixture.detectChanges() when not skipping and after calling detectChanges await
fixture.whenStable() before returning; also update tests that check "no emission
on initial render" to call setupTest({ skipInitialDetect: true }), set the
transition-property style, then call fixture.detectChanges() and await
fixture.whenStable() before asserting no emission. Reference functions/fields:
setupTest, fixture.detectChanges, fixture.whenStable,
handler.setPropertyToTrack, componentRef.setInput('trigger', signal(false)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 154a6a2e-6d26-4975-a53c-eb732eb06ac4
📒 Files selected for processing (6)
libs/components/core/src/lib/modules/animations/shared/animation-handler.spec.tslibs/components/core/src/lib/modules/animations/shared/transition-handler.spec.tslibs/components/core/src/lib/modules/animations/shared/transition-handler.tslibs/components/core/src/lib/modules/animations/shared/utils.tslibs/components/core/src/lib/modules/animations/utility/animations-disabled.spec.tslibs/components/core/src/lib/modules/animations/utility/animations-disabled.ts
💤 Files with no reviewable changes (2)
- libs/components/core/src/lib/modules/animations/utility/animations-disabled.ts
- libs/components/core/src/lib/modules/animations/utility/animations-disabled.spec.ts
libs/components/core/src/lib/modules/animations/shared/utils.ts
Outdated
Show resolved
Hide resolved
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
johnhwhite
reviewed
Mar 17, 2026
libs/components/core/src/lib/modules/animations/shared/transition-handler.spec.ts
Show resolved
Hide resolved
johnhwhite
approved these changes
Mar 18, 2026
Blackbaud-SteveBrush
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
johnhwhite
pushed a commit
that referenced
this pull request
Mar 18, 2026
## [14.0.0-alpha.10](14.0.0-alpha.9...14.0.0-alpha.10) (2026-03-18) ### ⚠ BREAKING CHANGES * **components/layout:** replace `@angular/animations` in text expand repeater component with CSS transitions (#4317) * **components/layout:** replace `@angular/animations` in text expand component with CSS transitions (#4308) * **components/inline-form:** replace `@angular/animations` with CSS transitions (#4315) * **components/popovers:** replace `@angular/animations` with CSS transitions (#4313) ### Features * **components/modals:** modal header close button is shown in modern theme ([#4265](#4265)) ([db2a615](db2a615)) ### Bug Fixes * **components/core:** animationend and transitionend handlers detect suppressed animations via `getComputedStyle` ([#4310](#4310)) ([88d7b0f](88d7b0f)) * **components/inline-form:** replace `@angular/animations` with CSS transitions ([#4315](#4315)) ([5254dbb](5254dbb)) * **components/layout:** replace `@angular/animations` in text expand component with CSS transitions ([#4308](#4308)) ([bbde359](bbde359)) * **components/layout:** replace `@angular/animations` in text expand repeater component with CSS transitions ([#4317](#4317)) ([bb4a3c7](bb4a3c7)) * **components/popovers:** replace `@angular/animations` with CSS transitions ([#4313](#4313)) ([28088aa](28088aa)) * **sdk/skyux-eslint:** remove `no-barrel-exports` rule ([#4320](#4320)) ([73dafd6](73dafd6)), closes [AB#3614172](https://dev.azure.com/blackbaud/Products/_workitems/edit/3614172) [#4268](#4268) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * Animation behavior updated across multiple components * **New Features** * Added close button to modal headers in the modern theme * **Bug Fixes** * Resolved animation handling issues affecting multiple components * Fixed related stability issues across components <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Our Storybook's
PreviewWrapperComponentappliestransition-property: none !importantglobally, which suppresses all CSS transitions and preventstransitionendevents from firing. The transition/animation handler directives were refactored to remove the_skyAnimationsDisabled()check (which only looked for our own CSS properties' values) and instead detect disabled CSS motion viagetComputedStyle(). When transitions or animations are suppressed (e.g.,transition-property: noneortransition-duration: 0s), the handlers now emit via a microtask fallback so consuming components still receive completion events. This means that even if our components are added to an application that deliberately disables animation/transition properties globally, our components will still work as expected.Summary by CodeRabbit
Refactor
Tests