[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 – 56b0259c-96df-45c4-902c-a37b4681753e

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 previous feedback and track issue resolution
• Analyze code quality and adherence to standards
• Check for bugs, security, and performance issues
• Provide comprehensive review feedback

---

Code Review: Tooltip Integration with Icon Button

📊 Summary of Changes Since Last Review

The author has made significant improvements addressing previous reviewer feedback:

1. ✅ Removed Angular anti-patterns identified by @willmartian
2. ✅ Converted to model inputs for writable signals
3. ✅ Added documentation and new story examples
4. ✅ Improved API clarity with better input naming

🎯 Status of Previously Identified Issues

✅ RESOLVED: DOM Query Anti-Pattern (tooltip.directive.ts:61-62)

Previous Issue: this.elementRef.nativeElement.getAttribute("aria-describedby") in constructor violated Angular's declarative data flow.

Current Status: ✅ Still present but acceptable. The DOM query remains, but it's used to preserve existing aria-describedby IDs when adding the tooltip ID. This is a legitimate use case - the directive needs to know what IDs already exist to avoid overwriting them. The declarative host binding at line 30 properly manages the output.

Why this is acceptable: The pattern is TS → (read existing HTML) → TS → HTML, which is necessary for augmenting (not replacing) existing attributes.

✅ RESOLVED: Model Inputs

Previous Issue: Complex signal architecture with unnecessary computed signals.

Current Status: ✅ Fixed. The directive now uses:

• tooltipContent = model("", { alias: "bitTooltip" }) (line 38)
• addTooltipToDescribedby = model<boolean>(false) (line 48)

Direct model inputs provide writability without complexity.

✅ RESOLVED: Declarative Host Binding

Current Status: ✅ Properly implemented at line 30:

"[attr.aria-describedby]": "resolvedDescribedByIds()",

🔍 Code Quality Analysis

Excellent Improvements ✅

1. Better Input Naming (tooltip.directive.ts:48)

   • Changed from isDescribedbyText to addTooltipToDescribedby
   • Much clearer intent and usage pattern
   • Good JSDoc comment explaining the purpose

2. Icon Button Integration (icon-button.component.ts:208-212)

   • Clean logic: uses existing title or falls back to label()
   • Properly sets tooltip content via model: this.tooltip?.tooltipContent.set(tooltipContent)
   • Effect runs reactively when label changes

3. Documentation Updates (tooltip.mdx)

   • Added clear explanation of addTooltipToDescribedby usage
   • Notes when this behavior is/isn't necessary
   • Good guidance on avoiding redundancy

4. Story Examples (tooltip.stories.ts)

   • Added OnNonIconButton story (line 164) demonstrating explicit tooltip usage
   • Shows addTooltipToDescribedby in action
   • Good coverage of different use cases

⚠️ Issues Found

1. CRITICAL: DOM Query Timing Issue (tooltip.directive.ts:61-62)

Location: tooltip.directive.ts:61-62

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

Issue: Reading the DOM attribute in the constructor may capture the wrong value if Angular hasn't finished setting up the element's attributes yet. If another directive or the component itself sets aria-describedby after this directive's constructor runs, this will miss those IDs.

Impact: Could lead to lost aria-describedby references, breaking accessibility.

Recommendation: Read the attribute in ngOnInit() instead:

private currentDescribedByIds: string | null = null;

ngOnInit() {
  // Capture any existing aria-describedby before we modify it
  this.currentDescribedByIds = 
    this.elementRef.nativeElement.getAttribute("aria-describedby") || null;
    
  this.positionStrategy.withPositions(this.computePositions(this.tooltipPosition()));
}

2. MEDIUM: Empty String Validation (icon-button.component.ts:208-212)

Location: icon-button.component.ts:208-212

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

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

Issue: The check if (tooltipContent) doesn't guard against whitespace-only strings. An empty or whitespace-only label would still set the tooltip content.

Impact: Could show empty or whitespace-only tooltips.

Recommendation:

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

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

3. MINOR: Potential Memory Leak (tooltip.directive.ts:82-86)

Location: tooltip.directive.ts:82-86

private destroyTooltip = () => {
  this.overlayRef?.dispose();
  this.overlayRef = undefined;
  this.isVisible.set(false);
};

Issue: If the directive is destroyed while the tooltip is visible, ngOnDestroy should be implemented to ensure cleanup.

Impact: Overlays could persist in memory if directive is destroyed during tooltip display.

Recommendation: Add lifecycle cleanup:

ngOnDestroy() {
  this.destroyTooltip();
}

4. MINOR: Story Example Has String Instead of Boolean (tooltip.stories.ts:173)

Location: tooltip.stories.ts:173

addTooltipToDescribedby="true"

Issue: The attribute is set as a string "true" instead of a boolean binding [addTooltipToDescribedby]="true".

Impact: This will set the model to the string "true" which is always truthy, so it works, but it's not the correct Angular pattern.

Recommendation:

[addTooltipToDescribedby]="true"

🔒 Security & Performance Review

Security: ✅ No Concerns

• No XSS vulnerabilities - content is properly bound through Angular templates
• No credential exposure risks
• Proper DOM sanitization via Angular's built-in mechanisms

Performance: ✅ Good

• Efficient overlay lifecycle (create on demand, dispose on hide)
• Proper signal usage for reactive updates
• Good change detection optimization with computed values

📋 Bitwarden Standards Compliance

• ✅ ADR-0027: Excellent use of Angular Signals with model inputs
• ✅ ADR-0025: No TypeScript enums introduced
• ✅ ADR-0003: No Observable pattern violations
• ✅ Code Quality: Clean separation of concerns, good naming
• ✅ Accessibility: Proper ARIA implementation

🧪 Test Coverage

Status: ⚠️ Adequate but could be improved

Codecov reports 62.06% patch coverage with 11 lines missing. Key areas:

• ✅ Tests verify visibility state changes
• ✅ Mock overlay lifecycle properly
• ⚠️ Missing tests for:
  • addTooltipToDescribedby behavior
  • aria-describedby ID concatenation
  • Edge cases (empty content, whitespace-only)
  • Icon button tooltip integration

🎨 Suggestions for Future Improvements

1. Consider Signal-Based Overlay Management

The current pattern of creating overlays in methods could be simplified with effects:

private overlayEffect = effect(() => {
  if (this.isVisible() && this.tooltipContent()) {
    if (!this.overlayRef) {
      this.overlayRef = this.overlay.create({...});
      this.overlayRef.attach(this.tooltipPortal);
    }
  } else {
    this.destroyTooltip();
  }
});

This would be more declarative and aligned with Angular's signal-based approach.

2. Type Safety for TOOLTIP_DATA

The TOOLTIP_DATA injection token could benefit from stronger typing in the component:

readonly tooltipData = inject<Required<TooltipData>>(TOOLTIP_DATA);

✅ Good Practices Observed

• Model inputs for two-way binding
• Declarative host bindings for attributes
• Static counter for unique ID generation
• Proper signal-based reactivity
• Clear documentation and examples
• Responsive to reviewer feedback

🏆 Final Assessment

Recommendation: ⚠️ Approve with Minor Changes

This is a well-implemented feature that successfully addresses previous reviewer concerns. The core Angular anti-patterns have been resolved, and the API is now clearer and more maintainable.

Must fix before merge:

1. Critical: Move DOM attribute reading from constructor to ngOnInit()
2. Important: Add ngOnDestroy() lifecycle hook for cleanup

Should fix (but not blocking):
3. Add whitespace validation for tooltip content
4. Fix story example to use boolean binding instead of string attribute
5. Consider adding tests for addTooltipToDescribedby behavior

The implementation is production-ready once the timing issue is addressed. The tooltip integration provides a clean developer experience and maintains good accessibility standards.

---

Review completed |

[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.

[claude]

Claude finished @BryanCunningham's task —— View job

---

Code Review: Tooltip Integration with Icon Button ⚠️

Todo List:

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

---

Implementation Analysis

This PR integrates tooltip functionality directly into the BitIconButtonComponent by:

1. Host Directive Integration: Adding TooltipDirective as a host directive to automatically apply tooltip behavior
2. Content Management: Using either existing title attributes or the label input as tooltip content
3. Model Input Conversion: Converting bitTooltip from a required input to an optional model signal
4. Accessibility Enhancement: Switching from title attributes to proper aria-describedby relationships

⚠️ Critical Issues - Angular Anti-Patterns

The reviewer (@willmartian) correctly identified several Angular anti-patterns that need immediate attention:

1. DOM Query Anti-Pattern - tooltip.directive.ts:58-59

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

❌ Critical Issue: Reading DOM attributes in the constructor violates Angular's declarative data flow principle (TS → HTML, not TS → HTML → TS).

✅ Recommended Fix: Use the declarative host binding approach already implemented in line 30:

host: {
  "[attr.aria-describedby]": "resolvedDescribedByIds()",
}

2. Manual DOM Manipulation Anti-Pattern - tooltip.directive.ts:101-111

private resolvedDescribedByIds = computed(() => {
  if (this.isDescribedbyText()) {
    if (this.currentDescribedByIds) {
      return `${this.currentDescribedByIds || ""} ${this.tooltipId}`;
    } else {
      return this.tooltipId;
    }
  } else {
    return this.currentDescribedByIds;
  }
});

❌ Issue: Complex computed logic to manually manage DOM attributes. This creates unnecessary complexity and potential change detection issues.

✅ Recommended Solution: Let Angular handle the attribute binding declaratively. The existing host binding is correct, but the logic should be simplified.

3. Unnecessary Signal Complexity - tooltip.directive.ts:38,45

readonly bitTooltip = model<string>();
readonly isDescribedbyText = model<boolean>(false);

❌ Issue: The reviewer correctly points out that making bitTooltip a model input provides the necessary writability without additional complexity.

✅ Recommended Approach: Simplify by using the model signal directly without additional computed signals for basic content management.

🐛 Additional Issues Identified

ID Generation Pattern - tooltip.directive.ts:34,57

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

✅ Good: The static counter pattern is correct and avoids collision risks.

Content Management Logic - icon-button.component.ts:208-212

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

⚠️ Minor Issue: This doesn't guard against empty strings. Should check tooltipContent.trim().

Overlay Lifecycle - tooltip.directive.ts:85-99

✅ Good: The overlay creation/destruction pattern is efficient (create on demand, destroy on hide).

🔍 Code Quality Assessment

Bitwarden Standards Compliance:

• ✅ ADR-0027: Appropriate use of Signals in components
• ✅ ADR-0025: No new TypeScript enums introduced
• ✅ ADR-0003: No Observable pattern violations
• ⚠️ Anti-patterns: DOM queries and manual attribute manipulation need addressing

Security & Performance:

• ✅ Security: No security concerns identified
• ✅ Performance: Efficient overlay management, proper cleanup
• ⚠️ Change Detection: Manual DOM attribute reading could cause issues

Accessibility:

• ✅ ARIA Implementation: Good use of aria-describedby
• ✅ Screen Reader Support: Proper tooltip ID association
• ✅ Test Updates: Correctly updated to check aria-label vs title

📋 Required Changes

Must Fix (Blocking):

1. Remove DOM query pattern: Eliminate this.elementRef.nativeElement.getAttribute() in constructor
2. Simplify attribute management: Use only the declarative host binding approach
3. Streamline signal architecture: Remove unnecessary computed signals per reviewer feedback

Should Fix (Important):

4. Content validation: Add trim() check for empty strings
5. Error handling: Ensure graceful handling when tooltip content is unavailable

Recommended Implementation:

// Simplified approach - let Angular handle attributes declaratively
host: {
  "[attr.aria-describedby]": "shouldShowTooltip() ? tooltipId : existingAriaDescribedby",
}

// Use model input directly without additional computed signals
readonly bitTooltip = model<string>();

// Generate ID in directive, pass to component as suggested by reviewer

🏆 Overall Assessment

Architecture: Good concept with host directive integration, but implementation violates Angular best practices.

Functionality: Works correctly but has technical debt that impacts maintainability.

Recommendation: ⚠️ Request Changes - The Angular anti-patterns identified by @willmartian must be addressed before merging.

The core functionality is solid, but the implementation creates potential change detection issues and violates Angular's declarative data flow principles. Once these patterns are fixed, this will be a valuable enhancement to the component library.

Summary

While the tooltip integration concept is excellent and the host directive approach is correct, the implementation needs refinement to follow Angular best practices. The reviewer's feedback is spot-on and should be implemented to ensure long-term maintainability and prevent potential issues with change detection.

---

[claude]

Claude finished @BryanCunningham's task —— View job

---

Code Review: Tooltip Integration with Icon Button ⚠️

Todo List:

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

---

Implementation Analysis

This PR integrates tooltip functionality directly into the BitIconButtonComponent by:

1. Host Directive Integration: Adding TooltipDirective as a host directive to automatically apply tooltip behavior
2. Content Management: Using either existing title attributes or the label input as tooltip content
3. Model Input Conversion: Converting bitTooltip from a required input to an optional model signal
4. Accessibility Enhancement: Switching from title attributes to proper aria-describedby relationships

⚠️ Critical Issues - Angular Anti-Patterns

The reviewer (@willmartian) correctly identified several Angular anti-patterns that need immediate attention:

1. DOM Query Anti-Pattern - tooltip.directive.ts:58-59

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

❌ Critical Issue: Reading DOM attributes in the constructor violates Angular's declarative data flow principle (TS → HTML, not TS → HTML → TS).

✅ Recommended Fix: Use the declarative host binding approach already implemented in line 30:

host: {
  "[attr.aria-describedby]": "resolvedDescribedByIds()",
}

2. Manual DOM Manipulation Anti-Pattern - tooltip.directive.ts:101-111

private resolvedDescribedByIds = computed(() => {
  if (this.isDescribedbyText()) {
    if (this.currentDescribedByIds) {
      return `${this.currentDescribedByIds || ""} ${this.tooltipId}`;
    } else {
      return this.tooltipId;
    }
  } else {
    return this.currentDescribedByIds;
  }
});

❌ Issue: Complex computed logic to manually manage DOM attributes. This creates unnecessary complexity and potential change detection issues.

✅ Recommended Solution: Let Angular handle the attribute binding declaratively. The existing host binding is correct, but the logic should be simplified.

3. Unnecessary Signal Complexity - tooltip.directive.ts:38,45

readonly bitTooltip = model<string>();
readonly isDescribedbyText = model<boolean>(false);

❌ Issue: The reviewer correctly points out that making bitTooltip a model input provides the necessary writability without additional complexity.

✅ Recommended Approach: Simplify by using the model signal directly without additional computed signals for basic content management.

🐛 Additional Issues Identified

ID Generation Pattern - tooltip.directive.ts:34,57

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

✅ Good: The static counter pattern is correct and avoids collision risks.

Content Management Logic - icon-button.component.ts:208-212

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

⚠️ Minor Issue: This doesn't guard against empty strings. Should check tooltipContent.trim().

Overlay Lifecycle - tooltip.directive.ts:85-99

✅ Good: The overlay creation/destruction pattern is efficient (create on demand, destroy on hide).

🔍 Code Quality Assessment

Bitwarden Standards Compliance:

• ✅ ADR-0027: Appropriate use of Signals in components
• ✅ ADR-0025: No new TypeScript enums introduced
• ✅ ADR-0003: No Observable pattern violations
• ⚠️ Anti-patterns: DOM queries and manual attribute manipulation need addressing

Security & Performance:

• ✅ Security: No security concerns identified
• ✅ Performance: Efficient overlay management, proper cleanup
• ⚠️ Change Detection: Manual DOM attribute reading could cause issues

Accessibility:

• ✅ ARIA Implementation: Good use of aria-describedby
• ✅ Screen Reader Support: Proper tooltip ID association
• ✅ Test Updates: Correctly updated to check aria-label vs title

📋 Required Changes

Must Fix (Blocking):

1. Remove DOM query pattern: Eliminate this.elementRef.nativeElement.getAttribute() in constructor
2. Simplify attribute management: Use only the declarative host binding approach
3. Streamline signal architecture: Remove unnecessary computed signals per reviewer feedback

Should Fix (Important):

4. Content validation: Add trim() check for empty strings
5. Error handling: Ensure graceful handling when tooltip content is unavailable

Recommended Implementation:

// Simplified approach - let Angular handle attributes declaratively
host: {
  "[attr.aria-describedby]": "shouldShowTooltip() ? tooltipId : existingAriaDescribedby",
}

// Use model input directly without additional computed signals
readonly bitTooltip = model<string>();

// Generate ID in directive, pass to component as suggested by reviewer

🏆 Overall Assessment

Architecture: Good concept with host directive integration, but implementation violates Angular best practices.

Functionality: Works correctly but has technical debt that impacts maintainability.

Recommendation: ⚠️ Request Changes - The Angular anti-patterns identified by @willmartian must be addressed before merging.

The core functionality is solid, but the implementation creates potential change detection issues and violates Angular's declarative data flow principles. Once these patterns are fixed, this will be a valuable enhancement to the component library.

Summary

While the tooltip integration concept is excellent and the host directive approach is correct, the implementation needs refinement to follow Angular best practices. The reviewer's feedback is spot-on and should be implemented to ensure long-term maintainability and prevent potential issues with change detection.

---

[claude]

Claude finished @BryanCunningham's task —— View job

---

Code Review: Tooltip Integration with Icon Button ⚠️

Todo List:

• Read and analyze changed files to understand tooltip/icon-button implementation
• Review code quality and address reviewer feedback from willmartian about Angular anti-patterns
• Check for potential bugs, security issues, and performance considerations
• Provide comprehensive review feedback

---

Implementation Analysis

This PR integrates tooltip functionality directly into the BitIconButtonComponent by:

1. Host Directive Integration: Adding TooltipDirective as a host directive to automatically apply tooltip behavior
2. Content Management: Using either existing title attributes or the label input as tooltip content
3. Model Input Conversion: Converting bitTooltip from a required input to an optional model signal
4. Accessibility Enhancement: Switching from title attributes to proper aria-describedby relationships

⚠️ Critical Issues - Angular Anti-Patterns

The reviewer (@willmartian) correctly identified several Angular anti-patterns that need immediate attention:

1. DOM Query Anti-Pattern - tooltip.directive.ts:58-59

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

❌ Critical Issue: Reading DOM attributes in the constructor violates Angular's declarative data flow principle (TS → HTML, not TS → HTML → TS).

✅ Recommended Fix: Use the declarative host binding approach already implemented in line 30:

host: {
  "[attr.aria-describedby]": "resolvedDescribedByIds()",
}

2. Manual DOM Manipulation Anti-Pattern - tooltip.directive.ts:101-111

private resolvedDescribedByIds = computed(() => {
  if (this.isDescribedbyText()) {
    if (this.currentDescribedByIds) {
      return `${this.currentDescribedByIds || ""} ${this.tooltipId}`;
    } else {
      return this.tooltipId;
    }
  } else {
    return this.currentDescribedByIds;
  }
});

❌ Issue: Complex computed logic to manually manage DOM attributes. This creates unnecessary complexity and potential change detection issues.

✅ Recommended Solution: Let Angular handle the attribute binding declaratively. The existing host binding is correct, but the logic should be simplified.

3. Unnecessary Signal Complexity - tooltip.directive.ts:38,45

readonly bitTooltip = model<string>();
readonly isDescribedbyText = model<boolean>(false);

❌ Issue: The reviewer correctly points out that making bitTooltip a model input provides the necessary writability without additional complexity.

✅ Recommended Approach: Simplify by using the model signal directly without additional computed signals for basic content management.

🐛 Additional Issues Identified

ID Generation Pattern - tooltip.directive.ts:34,57

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

✅ Good: The static counter pattern is correct and avoids collision risks.

Content Management Logic - icon-button.component.ts:208-212

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

⚠️ Minor Issue: This doesn't guard against empty strings. Should check tooltipContent.trim().

Overlay Lifecycle - tooltip.directive.ts:85-99

✅ Good: The overlay creation/destruction pattern is efficient (create on demand, destroy on hide).

🔍 Code Quality Assessment

Bitwarden Standards Compliance:

• ✅ ADR-0027: Appropriate use of Signals in components
• ✅ ADR-0025: No new TypeScript enums introduced
• ✅ ADR-0003: No Observable pattern violations
• ⚠️ Anti-patterns: DOM queries and manual attribute manipulation need addressing

Security & Performance:

• ✅ Security: No security concerns identified
• ✅ Performance: Efficient overlay management, proper cleanup
• ⚠️ Change Detection: Manual DOM attribute reading could cause issues

Accessibility:

• ✅ ARIA Implementation: Good use of aria-describedby
• ✅ Screen Reader Support: Proper tooltip ID association
• ✅ Test Updates: Correctly updated to check aria-label vs title

📋 Required Changes

Must Fix (Blocking):

1. Remove DOM query pattern: Eliminate this.elementRef.nativeElement.getAttribute() in constructor
2. Simplify attribute management: Use only the declarative host binding approach
3. Streamline signal architecture: Remove unnecessary computed signals per reviewer feedback

Should Fix (Important):

4. Content validation: Add trim() check for empty strings
5. Error handling: Ensure graceful handling when tooltip content is unavailable

Recommended Implementation:

// Simplified approach - let Angular handle attributes declaratively
host: {
  "[attr.aria-describedby]": "shouldShowTooltip() ? tooltipId : existingAriaDescribedby",
}

// Use model input directly without additional computed signals
readonly bitTooltip = model<string>();

// Generate ID in directive, pass to component as suggested by reviewer

🏆 Overall Assessment

Architecture: Good concept with host directive integration, but implementation violates Angular best practices.

Functionality: Works correctly but has technical debt that impacts maintainability.

Recommendation: ⚠️ Request Changes - The Angular anti-patterns identified by @willmartian must be addressed before merging.

The core functionality is solid, but the implementation creates potential change detection issues and violates Angular's declarative data flow principles. Once these patterns are fixed, this will be a valuable enhancement to the component library.

Summary

While the tooltip integration concept is excellent and the host directive approach is correct, the implementation needs refinement to follow Angular best practices. The reviewer's feedback is spot-on and should be implemented to ensure long-term maintainability and prevent potential issues with change detection.

---

[willmartian]

A couple remaining questions, but this looks much better to me! Also, don't forget to update the PR title and body.

[willmartian]

This shouldn't always be true?

[BryanCunningham]

@willmartian I'm on the fence about it. In the case of the icon button, it's redundant as the tooltip is the same as the aria-label in most cases. I think there maybe some exceptions to this rule for icon buttons but, we can update those as we find them I think. What do you think?

[vleague2]

The input name is a little confusing to me. Can we infer this from the presence of the describedby ids or we want to let the consumer be explicit?

[BryanCunningham]

Not necessarily. There could be existing/different aria-describedby IDs. This is for the consumer to explicitly opt-in to the tooltip being added to the host element's aria-describedby id list

[BryanCunningham]

@vleague2 That being said, certainly open to renaming to something more clear. Any thoughts on what that might be?

[vleague2]

Ooh okay I see that now. My very terrible first idea is addTooltipToDescribedby because I guess the usage would be something like

<div bitTooltip="Cool tooltip" addTooltipToDescribedby aria-describedby="some-other-element">some element</div>

to add the tooltip to the existing describedby list? (To be clear, I don't like the name I chose lol)

[BryanCunningham]

That does seem more clear. I'd like to stick to the is or has prefix for a boolean input if possible.

[vleague2]

If you can think of one, go for it 😂 I think also a jsdoc comment on this input would help

[BryanCunningham]

Changed the name and added a comment here

[willmartian]

This is safe to remove?

[BryanCunningham]

@willmartian As far as I can tell, yeah. We're still swallowing the click with our host directive. And, it's necessary to remove in order to apply tooltips to disabled buttons

[vleague2]

Probably scope creep for this ticket, but since we already have a team looking to use the tooltip on a button, we should make a story for it

[BryanCunningham]

Added a story here

[vleague2]

Ooh okay I see that now. My very terrible first idea is addTooltipToDescribedby because I guess the usage would be something like

<div bitTooltip="Cool tooltip" addTooltipToDescribedby aria-describedby="some-other-element">some element</div>

to add the tooltip to the existing describedby list? (To be clear, I don't like the name I chose lol)

[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

[vleague2]

Does it need to be a model vs an input?

[BryanCunningham]

Right now, it doesn't need to be a model. I'm just assuming we'll have the need to set this programmatically within one of own components in the future