Fix mask gradient "none" producing invisible element

[mattgperry]

Summary

• mask: none means "no masking" (fully visible), but getAnimatableNone was making all colors transparent (alpha=0) — causing elements to start/end invisible when animating mask to/from "none"
• Added a mask value type (like the existing filter type) that keeps colors opaque (alpha=1) when generating the zero-equivalent template
• Registered mask and WebkitMask in the defaults map and animatable-none resolver

Test plan

• Unit tests for mask.getAnimatableNone with linear gradients (rgba, hex) and radial gradients
• All existing value type tests pass (35/35)
• All animate-prop integration tests pass (53/53)

🤖 Generated with Claude Code

[claude]

Code Review

Summary

This PR fixes a bug where animating CSS mask properties to/from "none" would cause elements to become invisible. The fix introduces a new mask value type that keeps colors opaque (alpha=1) when generating the zero-equivalent template, similar to the existing filter type implementation.

---

✅ Code Quality & Best Practices

Excellent:

• Follows the established pattern from filter.ts consistently
• Adheres to the codebase style guidelines (named exports, arrow callbacks, interfaces, small file size)
• Clean separation of concerns with a dedicated mask.ts file
• Proper registration in both defaults.ts and animatable-none.ts
• Good test coverage with multiple gradient scenarios (linear, radial, rgba, hex)

Minor suggestions:

1. Type annotation: In mask.ts:5, the makeOpaque function parameter type could be more precise:

   const makeOpaque = (v: number | Color | string) => {

   Since this is used with ComplexValues (which is Array<CSSVariableToken | AnyResolvedKeyframe | Color>), the string type seems to cover CSSVariableToken and AnyResolvedKeyframe, but it might be clearer to use the actual union type or add a comment explaining the type choice.

2. Consistency with filter.ts: The filter type defines getAnimatableNone inline, while mask defines it as a separate function. Both approaches work, but for codebase consistency, consider matching the style in filter.ts unless there's a reason for the difference.

---

✅ Correctness & Bug Prevention

The fix is sound and addresses the root cause:

• Problem identified correctly: mask: none means "no masking" (fully visible), but the default getAnimatableNone was making colors transparent (alpha=0), causing invisible elements
• Solution is correct: Setting alpha to 1 maintains visibility while zeroing out numeric values (percentages, angles)
• Proper registration: Both mask and WebkitMask are registered, ensuring cross-browser compatibility

Edge cases covered:

• Linear gradients with rgba ✓
• Linear gradients with hex ✓
• Radial gradients ✓
• Mixed color formats ✓

Potential edge case to verify (not blocking):

• What happens with mask-image values that use url() functions or other non-gradient masks? The complex type should handle these gracefully, but an integration test animating from mask: url(#svg-mask) to mask: none could verify this works as expected.

---

✅ Performance Considerations

Excellent:

• Follows the library's principle of prioritizing small file size
• Reuses existing complex.parse and complex.createTransformer
• No performance regressions expected
• The makeOpaque function is concise and efficient

---

✅ Security Concerns

No security issues identified. The code:

• Properly uses the existing parsing infrastructure
• Doesn't introduce any new user input handling
• Leverages the same sanitization as the complex type

---

⚠️ Test Coverage

Good unit test coverage, but missing integration tests:

✅ Present:

• Unit tests for getAnimatableNone with various gradient formats
• Tests verify opaque colors (alpha=1) are maintained
• Tests verify numeric values are zeroed

❌ Missing (recommended):
According to CLAUDE.md: "IMPORTANT: Always write tests for every bug fix AND every new feature. Write a failing test FIRST before implementing."

1. Cypress E2E test to verify the actual animation behavior:
   • Create a test page in dev/react/src/tests/mask-none.tsx
   • Create a spec in packages/framer-motion/cypress/integration/mask-none.ts
   • Test animating from a gradient mask to "none" and verify the element remains visible throughout
   • Verify WAAPI acceleration using element.getAnimations()

Suggested E2E test case:

// dev/react/src/tests/mask-none.tsx
export const App = () => {
    const [isNone, setIsNone] = useState(false)
    
    return (
        <motion.div
            data-testid="masked-element"
            animate={{
                mask: isNone 
                    ? "none" 
                    : "linear-gradient(0deg, rgba(0,0,0,0) 0%, rgb(0,0,0) 100%)"
            }}
            onClick={() => setIsNone(!isNone)}
        >
            Content
        </motion.div>
    )
}

// packages/framer-motion/cypress/integration/mask-none.ts
describe("mask animation to 'none'", () => {
    it("should keep element visible when animating to mask: none", () => {
        cy.visit("?test=mask-none")
            .get("[data-testid='masked-element']")
            .should("be.visible")
            .click()
            .wait(50)
            .should("be.visible") // Should remain visible during/after animation
    })
})

2. Alternative: If E2E testing is too heavy for this fix, at minimum add a test in packages/motion-dom/src/animate/__tests__/index.test.ts that calls animate() with mask values and verifies the generated keyframes.

---

📋 Additional Notes

1. Documentation: Consider if this warrants a mention in release notes or migration docs, as users may have worked around this bug and could be surprised by the fix.

2. WebkitMask: Good catch including the vendor prefix. Consider if MozMask should also be included (though Firefox has supported unprefixed mask since version 53).

3. Related properties: Are there other mask-related properties that might have similar issues? (mask-size, mask-position, etc.) These likely don't need special handling since they don't use gradients, but worth confirming.

---

🎯 Recommendation

Approve with minor suggestions

The code is well-written, follows established patterns, and correctly fixes the reported bug. The main gap is integration testing per the repository's testing guidelines. Consider adding a Cypress E2E test to verify the actual animation behavior, not just the value transformation logic.

Severity of missing E2E test: Low - The unit tests provide good coverage of the transformation logic, and the pattern is proven with the existing filter type. However, since CLAUDE.md emphasizes "ALWAYS write tests for every bug fix" and specifically recommends E2E tests for "UI behavior that involves DOM rendering", adding one would align with best practices.

Great work overall! 🚀

[claude]

Code Review - PR #3558

Summary

This PR fixes a critical bug where mask: none animations were producing invisible elements. The root cause was that getAnimatableNone was setting alpha to 0 (transparent) for all colors, but for mask gradients, none means "no masking" (fully visible), so colors should remain opaque (alpha=1).

Positive Feedback

Excellent problem identification and solution ✅

• The fix correctly identifies that mask gradients need different "zero-equivalent" behavior than other CSS properties
• Following the established pattern from filter.ts (packages/motion-dom/src/value/types/complex/filter.ts:26-32) is the right approach

Clean implementation ✅

• The makeOpaque helper is concise and handles the three value types correctly (number → 0, color object → alpha=1, string → unchanged)
• Proper use of the complex type's parse and createTransformer methods
• Registration in both defaults.ts and animatable-none.ts ensures the type is properly recognized

Good test coverage ✅

• Tests cover linear gradients with rgba and hex colors
• Tests cover radial gradients
• Tests verify that numbers are zeroed while colors stay opaque
• All three test cases pass according to the PR description

Performance optimization ✅

• Using a Set for customTypes (packages/motion-dom/src/value/types/utils/animatable-none.ts:6) instead of multiple equality checks is a smart micro-optimization that improves lookup performance

Code Quality Observations

Type safety ✅
The implementation correctly uses the existing types:

• AnyResolvedKeyframe for the input (packages/motion-dom/src/value/types/complex/mask.ts:3)
• Color type with proper alpha handling (packages/motion-dom/src/value/types/complex/mask.ts:7)
• The makeOpaque function handles all three possible parsed value types from complex.parse

Consistency ✅

• Follows the same pattern as filter.ts
• WebKit prefix support included (WebkitMask) alongside standard mask
• Spread operator usage {...complex, getAnimatableNone} maintains all base functionality

Potential Concerns & Suggestions

1. String value handling ⚠️
In the makeOpaque function (packages/motion-dom/src/value/types/complex/mask.ts:5-8), string values are returned as-is:

if (typeof v === "object" && "alpha" in v) return { ...v, alpha: 1 }
return v

This means CSS variables (e.g., var(--my-color)) will be preserved, which is likely correct. However, it would be worth verifying that mask animations with CSS variables behave as expected, though this is probably an edge case.

2. Test coverage for edge cases
The current tests are good, but consider adding tests for:

• Mixed mask properties (e.g., mask: url(image.png), linear-gradient(...))
• CSS variable usage in mask gradients
• Multi-layered masks
• mask-image vs mask shorthand property behavior

These may be beyond the scope of this bug fix, but they're worth considering for future test additions.

3. Documentation
Consider adding a brief comment in mask.ts explaining why mask needs special handling (similar to how filter.ts has comments). For example:

/**
 * Mask gradients need special handling: mask: none means "no masking" (fully visible),
 * so the zero-equivalent should keep colors opaque (alpha=1) rather than transparent.
 */

4. Type narrowing
The type guard typeof v === "object" && "alpha" in v works, but could be more precise:

const makeOpaque = (v: number | Color | string) => {
    if (typeof v === "number") return 0
    if (typeof v === "object" && "alpha" in v) {
        return { ...v, alpha: 1 } // TypeScript knows v is Color here
    }
    return v
}

This is already well-structured, but ensuring TypeScript properly narrows the type inside the conditional would provide better type safety.

Security & Performance

No security concerns ✅

• No user input is directly executed or evaluated
• The parsing is handled by the existing complex type which already sanitizes values

Performance impact: Minimal ✅

• The Set lookup for custom types is O(1)
• The makeOpaque transformation is only applied when needed (when animating mask: none)
• File size impact is minimal (~20 lines for the new file)

Browser Compatibility

Good WebKit support ✅

• Including both mask and WebkitMask ensures compatibility with older WebKit browsers
• The -webkit-mask prefix is needed for Safari < 15.4 and Chrome < 120

Best Practices Alignment

Follows CLAUDE.md conventions ✅

• Small file size prioritized (20 lines) ✅
• Uses optional chaining where appropriate ✅
• Named exports (no default export) ✅
• Arrow functions used ✅
• Tests added for the bug fix ✅

Recommendation

APPROVE with minor suggestions ✅

This is a solid bug fix that:

1. Correctly identifies and solves the root cause
2. Follows established patterns in the codebase
3. Includes appropriate test coverage
4. Has minimal performance and security impact
5. Adheres to the project's coding standards

The minor suggestions above are for potential future improvements and don't block this PR.

---

Great work on this fix! The problem was well-diagnosed and the solution is clean and maintainable.

🤖 Review generated with Claude Code

[claude]

Code Review

This is a well-crafted fix that correctly addresses the mask gradient animation issue. The implementation follows the existing pattern established by the filter type and includes comprehensive test coverage.

Strengths

✅ Correct Problem Diagnosis: The fix properly identifies that mask: none means "fully visible" (no masking), not transparent. The previous behavior of setting alpha to 0 would make elements invisible, which is the opposite of the intended behavior.

✅ Consistent Pattern: The implementation follows the exact same pattern as the existing filter type (packages/motion-dom/src/value/types/complex/filter.ts:26-32), which is the right approach for maintainability.

✅ Comprehensive Test Coverage: The unit tests cover multiple gradient formats:

• Linear gradients with rgba colors
• Linear gradients with hex colors
• Radial gradients with circle positioning
• Multiple color stops

✅ Code Style: Adheres to the repository's conventions:

• Named exports (no default exports)
• Arrow functions for callbacks
• Proper TypeScript interfaces
• Tree-shaking annotations (/*@__PURE__*/)
• Minimal file size (concise implementation)

✅ Cross-Browser Support: Registers both mask and WebkitMask in the defaults map (packages/motion-dom/src/value/types/maps/defaults.ts:28-29)

Technical Analysis

The makeOpaque helper correctly:

• Zeros out numbers (for positions, percentages, angles)
• Preserves color objects while forcing alpha to 1
• Leaves other values unchanged (e.g., CSS variables)

This ensures that animating from mask: "none" to a gradient will:

1. Start with colors at full opacity (visible)
2. Animate positions/percentages from 0
3. Smoothly transition to the target gradient

The integration with getAnimatableNone in packages/motion-dom/src/value/types/utils/animatable-none.ts:6-10 is clean - using a Set for O(1) lookup of custom types.

Minor Observations

Type Safety: The makeOpaque function signature is:

const makeOpaque = (v: number | Color | string) => ...

This correctly handles the types returned by complex.parse(), which returns ComplexValues (CSSVariableToken | AnyResolvedKeyframe | Color). The implementation handles all three cases properly.

Real-World Usage: I noticed the codebase already uses mask-image with gradients in dev/react/src/tests/drag-tabs.tsx:331-332, so this fix will benefit existing usage patterns in the library.

Security & Performance

✅ No security concerns - pure value transformation logic
✅ Performance is optimal - simple mapping operation with no loops or complex logic
✅ Tree-shaking annotation ensures minimal bundle impact

Test Plan Verification

The PR description indicates:

• ✅ Unit tests added and passing (35/35 value type tests)
• ✅ Integration tests passing (53/53 animate-prop tests)

Recommendation

Approved ✅

This is a high-quality fix that solves a real bug, follows established patterns, includes proper test coverage, and has no obvious downsides. The implementation is minimal, focused, and correct.

---

Great work on this fix! The attention to detail in matching the filter pattern and the comprehensive test coverage is exactly what this codebase needs.

[claude]

PR Review

Summary

This PR fixes a bug where animating CSS mask properties to/from "none" caused elements to become invisible. The root cause was that getAnimatableNone was setting all colors to transparent (alpha=0), but for masks, none means "no masking" (fully visible), so colors should remain opaque (alpha=1).

✅ Strengths

1. Correct Fix: The implementation properly mirrors the existing filter value type pattern, which is the right architectural approach
2. Well-tested: Comprehensive unit tests covering:
   • Linear gradients with rgba and hex colors
   • Multiple color stops
   • Radial gradients
3. Minimal footprint: Following the codebase's priority of small file size (15 LOC for the fix)
4. Proper registration: Both mask and WebkitMask are registered in the defaults map and animatable-none resolver

🔍 Code Quality

Good practices observed:

• Clean use of arrow callbacks
• Concise, byte-efficient patterns using ternary operators
• Proper use of spread operator for immutability
• Consistent with existing filter.ts implementation

Follows repository conventions:

• Named exports (no default exports)
• TypeScript interfaces (when applicable)
• Tree-shakeable with /*@__PURE__*/ comment in animatable-none.ts:6

🐛 Potential Issues

1. Type safety consideration (minor):
The check typeof v === "object" on line 11 assumes color objects will always have an alpha property. While this works with the current implementation (colors are parsed as RGBA/HSLA objects), it could be more explicit:

// Current:
typeof v === "object" ? { ...v, alpha: 1 } : v

// More explicit (optional improvement):
typeof v === "object" && "alpha" in v ? { ...v, alpha: 1 } : v

However, given that complex.parse only returns Color objects (which always have alpha), the current code is safe.

2. CSS variable handling:
CSS variables (var(--color)) are treated as strings and passed through unchanged. This is correct behavior (you can't know the value at parse time), but worth noting that if a CSS variable resolves to a color in a mask gradient, it won't get the alpha treatment. This is acceptable since it's a limitation of static analysis.

⚡ Performance

• No performance concerns
• The Set-based lookup in animatable-none.ts:6 (customTypes.has()) is O(1) and efficient
• Minimal overhead added to the animation system

🔒 Security

• No security concerns
• Pure data transformation with no user-controlled code execution

🧪 Test Coverage

Unit tests are comprehensive, but consider adding:

1. E2E test: A Cypress test demonstrating the actual bug fix (animating from/to mask: "none") would provide regression protection and validate the fix in a real browser environment. Following the pattern in CLAUDE.md:

// Suggested test in dev/react/src/tests/mask-none-animation.tsx
export function App() {
    const [masked, setMasked] = useState(false)
    return (
        <motion.div
            data-testid="masked-element"
            animate={{
                mask: masked 
                    ? "linear-gradient(0deg, rgba(0,0,0,0) 0%, rgb(0,0,0) 100%)"
                    : "none"
            }}
            onClick={() => setMasked(!masked)}
        />
    )
}

2. Edge cases: Consider testing:
   • Empty mask value
   • Invalid gradient syntax
   • Multiple mask layers (comma-separated)

📋 Recommendations

1. ✅ Approve and merge - The fix is correct and well-implemented
2. 💡 Optional follow-up: Add a Cypress E2E test to prevent regression
3. 💡 Documentation: If there's a public API docs section on value types, mention the mask type

Minor Nits

• Line 11 in mask.ts: The nested ternary is readable but could be formatted with line breaks for clarity (though current style matches the codebase's byte-efficiency priority)

---

Overall verdict: ✅ LGTM - This is a solid fix with good test coverage. The implementation follows best practices and repository conventions. Great job identifying and fixing this subtle alpha transparency issue!

🤖 Review generated with Claude Code