[CL-879] use tooltip on icon button

[BryanCunningham]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/CL-879

📔 Objective

Use the required label input from the icon button component to attach a tooltip with the same content as the label

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – 67877346-b457-4918-b2ef-f64a2a098772

Great job! No new security vulnerabilities introduced in this pull request

[claude]

Claude finished @BryanCunningham's task —— View job

---

Todo List:

• Read changed files to understand current implementation
• Review git diff to analyze changes since origin/main
• Compare current implementation against previous review feedback
• Check if Angular anti-patterns were resolved
• Identify any new issues or concerns
• Verify adherence to Bitwarden coding standards
• Provide comprehensive review feedback

---

Code Review: Tooltip Integration with Icon Button

Summary of Changes

This PR integrates tooltip functionality into the BitIconButtonComponent by automatically applying tooltips using the component's label input. The implementation uses Angular's host directives pattern to compose the TooltipDirective with the icon button.

Key Changes:

• Modified TooltipDirective to use model input for tooltipContent (bitTooltip)
• Added addTooltipToDescribedby input to control aria-describedby attribute behavior
• Removed one pointer-events line from ButtonComponent to enable tooltips on disabled buttons
• Icon buttons now automatically set tooltip content from either existing title attributes or the label input
• Updated tests to reflect accessibility changes
• Added new stories demonstrating tooltip usage patterns

Status of Previously Identified Issues

✅ RESOLVED: DOM Query Anti-Pattern

Previous Issue (from @willmartian): Reading DOM attributes in constructor (this.elementRef.nativeElement.getAttribute())

Current Status: ⚠️ PARTIALLY RESOLVED - The code still reads DOM attributes in the constructor at tooltip.directive.ts:61-62:

private currentDescribedByIds =
  this.elementRef.nativeElement.getAttribute("aria-describedby") || null;

This remains an Angular anti-pattern. The declarative host binding at line 30 is correct, but the initial value is still being read from the DOM.

Recommendation: Consider using an input to capture existing aria-describedby values if needed, or accept that the initial value will be null/undefined and let Angular manage it declaratively from the start.

✅ RESOLVED: Model Input Approach

Previous Issue: Unnecessary signal complexity

Current Status: ✅ FIXED - The implementation now uses model() input directly (tooltip.directive.ts:38):

readonly tooltipContent = model("", { alias: "bitTooltip" });

This provides writability without additional complexity. The icon button correctly sets content via this.tooltip?.tooltipContent.set(tooltipContent) at icon-button.component.ts:213.

✅ IMPROVED: Signal Architecture

Previous Issue: Multiple unnecessary computed signals

Current Status: ✅ IMPROVED - The signal architecture is much cleaner. The only computed signal remaining is resolvedDescribedByIds which serves a legitimate purpose.

Critical Issues

⚠️ Issue 1: Remaining DOM Query Anti-Pattern - tooltip.directive.ts:61-62

Severity: Medium

The directive still reads aria-describedby from the DOM in the constructor:

private currentDescribedByIds =
  this.elementRef.nativeElement.getAttribute("aria-describedby") || null;

Impact:

• Violates Angular's declarative data flow principle (TS → HTML, not TS → HTML → TS)
• Could cause change detection timing issues
• Makes the component harder to test and reason about

Suggested Fix:

// Option 1: Accept as input if needed
readonly existingDescribedby = input<string>();

// Option 2: Remove entirely if not needed
// Let Angular manage aria-describedby completely declaratively

⚠️ Issue 2: Static Counter Mutation in Constructor - tooltip.directive.ts:60

Severity: Low

private tooltipId = `bit-tooltip-${TooltipDirective.nextId++}`;

While using a static counter is correct, mutating it in a property initializer can be problematic in edge cases (e.g., during testing or if the class is instantiated but not fully initialized).

Suggested Fix:

private tooltipId: string;

constructor() {
  this.tooltipId = `bit-tooltip-${TooltipDirective.nextId++}`;
  // ... rest of initialization
}

📝 Issue 3: Missing Input Validation - icon-button.component.ts:210-214

Severity: Low

const tooltipContent: string = originalTitle || this.label();

if (tooltipContent) {
  this.tooltip?.tooltipContent.set(tooltipContent);
}

The check doesn't validate for empty/whitespace-only strings. If this.label() returns " ", it will still set the tooltip content.

Suggested Fix:

const tooltipContent: string = originalTitle || this.label();

if (tooltipContent?.trim()) {
  this.tooltip?.tooltipContent.set(tooltipContent);
}

Good Practices Observed

• ✅ Clean host directive integration pattern (icon-button.component.ts:104-107)
• ✅ Proper use of Angular Signals following ADR-0027
• ✅ Good accessibility with aria-describedby implementation
• ✅ Efficient overlay lifecycle (create on demand, destroy on hide)
• ✅ Comprehensive Storybook examples including edge cases
• ✅ Updated tests to reflect accessibility improvements
• ✅ Clear documentation in MDX file

Suggestions for Improvement

💡 Suggestion 1: Clarify Documentation - tooltip.mdx:22-27

The note about addTooltipToDescribedby could be clearer:

Current:

"NOTE: This behavior is not always necessary and could be redundant if the host element's aria attributes already convey the same message as the tooltip."

Suggested:

"NOTE: For icon buttons, this is typically unnecessary since the label input already provides the aria-label. Use addTooltipToDescribedby="true" only when the tooltip provides additional context beyond what's in the aria-label."

💡 Suggestion 2: Story Attribute Syntax - tooltip.stories.ts:173

addTooltipToDescribedby="true"  // String "true"

Should be:

[addTooltipToDescribedby]="true"  // Boolean true

Or use the attribute binding syntax properly for boolean attributes.

💡 Suggestion 3: Test Coverage - tooltip.spec.ts

The tests only check visibility state. Consider adding tests for:

• Tooltip content updates via model input
• aria-describedby attribute behavior with addTooltipToDescribedby
• Tooltip behavior on disabled elements

Security & Performance

Security: ✅ No security concerns identified

• No XSS vulnerabilities
• Proper sanitization through Angular's built-in mechanisms
• No credential exposure risks

Performance: ✅ Good

• Efficient overlay lifecycle management
• Proper cleanup via destroyTooltip() method
• No memory leaks identified

Bitwarden Standards Compliance

• ✅ ADR-0027: Appropriate use of Signals in components
• ✅ ADR-0025: No new TypeScript enums introduced
• ✅ ADR-0003: No Observable pattern violations (appropriate use of Observables where needed)
• ✅ Code Organization: Proper separation of concerns
• ⚠️ Angular Best Practices: One remaining anti-pattern (DOM query)

Action Items for Author

Must Fix:

1. Address the DOM query anti-pattern (tooltip.directive.ts:61-62) - Either remove the DOM query entirely or accept the existing aria-describedby as an input if preservation is required

Should Fix:
2. Add whitespace validation for tooltip content (icon-button.component.ts:212)
3. Fix story attribute syntax (tooltip.stories.ts:173) - use boolean binding instead of string

Could Improve:
4. Move static counter increment to constructor for better initialization guarantees
5. Enhance test coverage for aria-describedby behavior and model input updates
6. Clarify documentation around when to use addTooltipToDescribedby

Overall Assessment

Recommendation: ⚠️ Approve with Minor Changes

The implementation has significantly improved since the last review and addresses most of the Angular anti-patterns previously identified. The remaining DOM query anti-pattern is the primary concern, but it's a relatively contained issue that could be addressed in a follow-up if needed for expediency.

Strengths:

• Clean architecture using host directives
• Good accessibility implementation
• Proper signal usage with model inputs
• Comprehensive documentation and examples
• Efficient resource management

Remaining Concerns:

• One DOM query anti-pattern remains (medium severity)
• Minor input validation gap
• Small test coverage gaps

The core functionality is solid and the integration provides good developer experience. Once the DOM query pattern is addressed, this will be an excellent enhancement to the component library.

---

[claude]

Claude finished @BryanCunningham's task —— View job

---

Code Review: Tooltip on Icon Button Integration

I've analyzed the changes and overall this is a solid improvement that integrates tooltip functionality directly into icon buttons. Here's my detailed feedback:

✅ Positive Aspects

1. Clean Integration: The tooltip is properly integrated as a host directive in icon-button.component.ts:104, which is the Angular-recommended approach for reusable behavior.

2. Accessibility Improvements:

   • Proper aria-describedby handling in tooltip.directive.ts:29
   • Tooltip IDs are correctly generated and referenced
   • Test updates reflect the accessibility changes (checking aria-label instead of title)

3. Flexible Content Management: The new setContent() method allows dynamic tooltip content updates.

🎨 Suggestions for Improvement

tooltip.component.ts:37 - ID Generation Concern

private nextId = Date.now().toString(36);

Issue: Using Date.now() for ID generation could potentially create collisions if multiple tooltips are created in the same millisecond.

Suggestion: Consider using a counter-based approach or crypto.randomUUID():

private static nextId = 0;
private nextId = `${++TooltipComponent.nextId}`;
// or
private nextId = crypto.randomUUID().substring(0, 8);

tooltip.directive.ts:43 - Input Signal Initialization

readonly isDescribedbyText = input<boolean>(true);
private _isDescribedbyText = signal(this.isDescribedbyText());

Issue: The private signal is initialized in the constructor context, but this.isDescribedbyText() might not be available yet.

Suggestion: Initialize the private signal in the constructor or use effect() to sync them:

constructor() {
  effect(() => {
    this._isDescribedbyText.set(this.isDescribedbyText());
  });
}

⚠️ Potential Issues

tooltip.directive.ts:97-118 - Memory Management

The tooltip creation logic doesn't check if tooltip content exists before creating an overlay:

private showTooltip = () => {
  if (!this.overlayRef) {
    // Should check if there's content to show
    if (!this.resolvedTooltipText()) return;
    
    this.overlayRef = this.overlay.create({
      ...this.defaultPopoverConfig,
      positionStrategy: this.positionStrategy,
    });
    // ... rest of logic
  }
}

icon-button.component.ts:208 - Null Safety

this.tooltip?.setContent(originalTitle || this.label());

Issue: If both originalTitle and this.label() are falsy, an empty string is passed.

Suggestion: Only set content when there's actually content:

const content = originalTitle || this.label();
if (content) {
  this.tooltip?.setContent(content);
}

📝 Minor Observations

1. tooltip.component.ts:22 - Commented code should be removed:

   // let nextId = 0; // Remove this

2. Consistency: The Stories examples properly switched from bitTooltip to label attributes, maintaining consistency with the new integration pattern.

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ Proper cleanup with destroyTooltip() method
• ✅ Efficient overlay management (creates only when needed)
• ✅ Proper use of Angular Signals for reactivity

🧪 Testing

The test updates look good - properly mocking the overlay system and testing visibility state changes. The switch from testing title to aria-label attributes reflects the accessibility improvements.

Overall Assessment

This is a well-implemented feature that enhances the icon button component with proper tooltip functionality while maintaining good accessibility practices. The main concerns are around ID generation and edge case handling, but these are relatively minor issues in an otherwise solid implementation.

Rating: 👍 Approve with minor suggestions

---

[claude]

Claude finished @BryanCunningham's task —— View job

---

Todo List:

• Read and analyze changed files to understand tooltip/icon-button implementation
• Review code quality and adherence to Bitwarden coding standards
• Check for potential bugs, security issues, and performance considerations
• Provide comprehensive review feedback

---

Code Review: Tooltip Integration with Icon Button

This PR successfully integrates tooltip functionality into the icon button component, enhancing accessibility and user experience. Overall, this is a well-implemented feature that follows Angular best practices.

✅ Positive Aspects

Excellent Architecture Decisions:

• Clean Host Directive Integration: Using hostDirectives in icon-button.component.ts:60-64 is the recommended Angular approach for reusable behavior composition
• Proper Signal Usage: Following ADR-0027 with appropriate use of Angular Signals for reactive state management
• Accessibility Focus: Good implementation of aria-describedby handling for screen reader support

Smart Integration Logic:

• Automatic Content Detection: icon-button.component.ts:160-166 intelligently uses either existing title attribute or the label input for tooltip content
• Proper Cleanup: tooltip.directive.ts:144-161 correctly manages overlay lifecycle and DOM attributes
• Flexible API: Added setContent() and setIsDescribedbyText() methods for programmatic control

🎨 Areas for Improvement

ID Generation Concern - tooltip.component.ts:12

private nextId = Date.now().toString(36);

Issue: Using Date.now() for ID generation could create collisions if multiple tooltips are created in rapid succession.

Suggestion: Use a counter-based approach for guaranteed uniqueness:

private static nextId = 0;
private nextId = `${++TooltipComponent.nextId}`;

Signal Synchronization - tooltip.directive.ts:41-42,54-64

private _isDescribedbyText = signal(this.isDescribedbyText());

Issue: The private signal initialization in the constructor might not properly sync with input signal changes.

Suggestion: Use effect() to maintain synchronization:

constructor() {
  effect(() => {
    this._isDescribedbyText.set(this.isDescribedbyText());
    this._tooltipText.set(this.bitTooltipText());
  });
}

Optional Input Validation - tooltip.directive.ts:33

The bitTooltip input is now optional, which is good for flexibility, but consider adding validation in showTooltip() to ensure content exists before creating the overlay.

📝 Code Quality Observations

Good Practices:

• ✅ Proper use of computed() for derived state (tooltip.directive.ts:49-51)
• ✅ Clean timeout management with proper cancellation
• ✅ Appropriate test updates reflecting the architectural changes
• ✅ Stories updated to showcase the new integration

Minor Cleanup Needed:

• Commented Code: Remove // let nextId = 0; in tooltip.component.ts:11
• Type Safety: The overlay mocking in tests is well done and properly typed

🔒 Security & Performance Review

No Security Concerns: ✅

• No user input is directly rendered without sanitization
• Proper DOM manipulation practices
• No XSS vulnerabilities identified

Performance Optimizations: ✅

• Efficient overlay creation/destruction pattern
• Proper use of computed values for reactive updates
• Good memory management with cleanup methods

🧪 Testing Updates

The test modifications are appropriate:

• Accessibility Tests: Properly updated to check aria-label instead of title attributes
• Mock Strategy: Good overlay service mocking that reflects the new lifecycle
• Coverage: Maintains good test coverage for tooltip behavior

🦮 Repository Standards Compliance

• ✅ ADR-0027: Appropriate use of Signals in components
• ✅ ADR-0003: No Observable pattern violations
• ✅ ADR-0025: No new TypeScript enums introduced
• ✅ Code Organization: Proper separation of concerns
• ✅ Accessibility: Good ARIA implementation

🏆 Summary

This is a solid implementation that enhances the icon button component with integrated tooltip functionality. The architecture is clean, follows Angular best practices, and maintains good accessibility standards. The main suggestions are around ID generation and signal synchronization, but these are relatively minor issues.

Recommendation: ✅ Approve with minor suggestions - The code is production-ready and the suggestions are non-blocking improvements.

The integration successfully provides a better developer experience by automatically handling tooltip content from existing attributes while maintaining flexibility for custom usage patterns.

---

[claude]

Claude finished @BryanCunningham's task —— View job

---

Todo List:

• Read and analyze changed files to understand tooltip/icon-button implementation
• Review code quality and adherence to Bitwarden coding standards
• Check for potential bugs, security issues, and performance considerations
• Provide comprehensive review feedback

---

Code Review: Tooltip Integration with Icon Button ✅

This PR successfully integrates tooltip functionality into the icon button component, providing a clean and accessible user experience. The implementation follows Angular best practices and adheres to Bitwarden's coding standards.

✅ Positive Aspects

Excellent Architecture & Angular Patterns:

• Smart Host Directive Usage: icon-button.component.ts:102-105 properly uses hostDirectives with TooltipDirective, following Angular's composition patterns
• Proper Signal Implementation: Follows ADR-0027 with appropriate use of Angular Signals for reactive state management
• Clean Accessibility: Good aria-describedby handling in tooltip.directive.ts:30,113-115 for screen reader support

Well-Designed Integration Logic:

• Automatic Content Detection: icon-button.component.ts:208-212 intelligently uses either existing title attribute or the label input for tooltip content
• Flexible API: tooltip.directive.ts:140-146 provides setContent() and setIsDescribedbyText() methods for programmatic control
• Improved Overlay Lifecycle: tooltip.directive.ts:98-125 creates/destroys overlays on-demand instead of keeping them persistent

🎨 Areas for Enhancement

ID Generation Strategy - tooltip.component.ts:22,38

let nextId = 0;
protected id = `bit-tooltip-${nextId++}`;

Good: Simple counter-based approach ensures unique IDs without collision risk.
Suggestion: Consider making nextId static to ensure uniqueness across component instances:

private static nextId = 0;
protected id = `bit-tooltip-${++TooltipComponent.nextId}`;

Signal Synchronization - tooltip.directive.ts:47,149-151

The private _isDescribedbyText signal initialization and effect synchronization is well-handled, ensuring proper reactivity.

Optional Input Validation - tooltip.directive.ts:37

Making bitTooltip optional enhances flexibility. The computed resolvedTooltipText properly handles both programmatic and declarative content.

🔍 Technical Quality Assessment

Security & Performance: ✅

• No security vulnerabilities identified
• Efficient overlay management with proper cleanup
• Good memory management through destroyTooltip() method
• No XSS risks in content handling

Code Standards Compliance: ✅

• ADR-0027: Appropriate use of Signals in components ✅
• ADR-0003: No Observable pattern violations ✅
• ADR-0025: No new TypeScript enums introduced ✅
• Bitwarden Patterns: Follows established component architecture ✅

Accessibility: ✅

• Proper ARIA implementation with aria-describedby
• Unique tooltip IDs for screen reader association
• Maintains existing accessibility attributes when adding tooltip functionality

📝 Minor Observations

1. Test Coverage: Test updates are appropriate - properly mocking overlay lifecycle and checking accessibility attributes (aria-label instead of title)

2. Stories Integration: Storybook examples correctly updated to use label instead of bitTooltip attributes, showcasing the new integration pattern

3. Vault Component Updates: Test updates in vault components reflect the accessibility improvements, checking aria-label attributes as expected

🚀 Implementation Highlights

Smart Integration Pattern:

• Icon buttons now automatically get tooltip content from either existing title attributes or label inputs
• Maintains backward compatibility while enhancing functionality
• Clean separation between tooltip positioning (tooltipPosition) and content management

Proper Lifecycle Management:

• Tooltip overlays are created only when needed (on show) and properly disposed (on hide)
• Maintains original aria-describedby attributes when adding tooltip references
• Efficient change detection with manual detectChanges() call for ID retrieval

📊 Summary

This is a well-architected feature that enhances the icon button component with integrated tooltip functionality. The implementation:

• ✅ Follows Angular and Bitwarden coding standards
• ✅ Maintains excellent accessibility practices
• ✅ Provides clean API with both declarative and programmatic usage
• ✅ Includes comprehensive test coverage updates
• ✅ Is backward-compatible and enhances existing functionality

Recommendation: ✅ Approve - This implementation is production-ready and represents a thoughtful enhancement to the component library.

The integration provides a better developer experience by automatically handling tooltip content from existing attributes while maintaining the flexibility for custom usage patterns.

Reviewed synchronously and addressed all comments.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jaasen-livefront]

vault changes look good. nice work!