[PM-28079] Add attributes to filter for the mutationObserver

[jholland-livefront]

🎟️ Tracking

PM-28079

📔 Objective

As part of @jprusik's performance research he determined we can specify the attributes we want to track in the MutationObserver. I have added 23 attributes we want to track with others being excluded.

I implemented a simple MutationObserver counter locally to get a rough idea of the performance difference. For the Simple Login on webtests.dev the MutationObserver count dropped from 9 to 7 with the attributeFilter added.

I am looking for suggestions any additions or deletions to this attribute list.

⏰ 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

[claude]

Claude finished @jholland-livefront's task —— View job

---

Code Review Complete ✓

I've thoroughly analyzed this PR and validated the existing reviewer feedback. Here's my comprehensive assessment:

---

🎯 Overall Assessment: Changes Needed

The PR successfully adds an attributeFilter to the MutationObserver for performance optimization and fixes a pre-existing bug. However, there are a few issues with attribute completeness that need to be addressed.

---

✅ What's Working Well

1. Bug Fix: The PR correctly fixes the bug on line 1346 where title mutations were incorrectly updating tabindex instead of title
2. Performance Optimization: Adding attributeFilter is the right approach to reduce unnecessary mutation callbacks
3. Good Coverage: Most critical attributes are included (23 attributes)
4. Comprehensive List: Includes important aria attributes and popover attributes for modern web interactions

---

⚠️ Issues Identified

I've validated theMickster's feedback and can confirm the following issues:

1. VERIFIED - Missing placeholder Attribute ❌

Location: libs/common/src/autofill/constants/index.ts:35-63

Issue: The placeholder attribute is collected during field scanning (line 407) but:

• ❌ Not in AUTOFILL_ATTRIBUTES constant
• ❌ No mutation handler in updateAutofillFieldElementData() (lines 1332-1375)

Evidence:

// Line 407 - Collected but not tracked
placeholder: this.getPropertyOrAttribute(element, "placeholder"),

Impact: If a page dynamically changes placeholder text (common in modern SPAs), autofill won't detect it and may use stale label data.

Recommendation:

• Add PLACEHOLDER: "placeholder" to the constant
• Add a mutation handler: placeholder: () => updateAttribute("placeholder")

---

2. VERIFIED - Form class Attribute Inconsistency 💭

Location: libs/common/src/autofill/constants/index.ts:55 and collect-autofill-content.service.ts:237, 1308-1313

Issue: The class attribute is:

• ✅ In the filter
• ✅ Collected for forms (line 237)
• ✅ Has field mutation handler (line 1344)
• ❌ Missing form mutation handler (lines 1308-1313 only handle action, name, id, method)

Evidence:

// Line 237 - Collected for forms
htmlClass: this.getPropertyOrAttribute(formElement, "class"),

// Lines 1308-1313 - No class handler for forms
const updateActions: Record<string, CallableFunction> = {
  action: () => (dataTarget.htmlAction = this.getFormActionAttribute(element)),
  name: () => updateAttribute("htmlName"),
  id: () => updateAttribute("htmlID"),
  method: () => updateAttribute("htmlMethod"),
  // Missing: class handler
};

Impact: Form class mutations will trigger the observer but won't be processed.

Recommendation: Either:

• Option A (preferred): Remove class from the filter since form class changes likely don't affect autofill logic
• Option B: Add a form class mutation handler: class: () => updateAttribute("htmlClass")

---

3. PARTIALLY VERIFIED - Legacy Autocomplete Attributes ⚠️

Location: libs/common/src/autofill/constants/index.ts:45-46

Status: ✅ Already included in the PR!

I can confirm that both legacy attributes are present:

AUTOCOMPLETE_TYPE: "autocompletetype",      // Line 45
X_AUTOCOMPLETE_TYPE: "x-autocompletetype", // Line 46

However, there's a handler inconsistency:

• Line 1354 only has a handler for autocomplete (calls getAutoCompleteAttribute() which checks all three)
• No separate handlers for autocompletetype or x-autocompletetype

Current behavior: If autocompletetype or x-autocompletetype changes, mutations will be detected, but the handler at line 1354 will only trigger for autocomplete changes. The mutation will be silently ignored for the other two.

Recommendation: Add mutation handlers:

"autocompletetype": () => (dataTarget.autoCompleteType = this.getAutoCompleteAttribute(element)),
"x-autocompletetype": () => (dataTarget.autoCompleteType = this.getAutoCompleteAttribute(element)),

---

📊 Attribute Completeness Analysis

I cross-referenced all attributes used in the codebase:

Attribute	Collected	In Filter	Has Handler	Status
action	✓	✓	✓	✅ Complete
method	✓	✓	✓	✅ Complete
maxlength	✓	✓	✓	✅ Complete
type	✓	✓	✓	✅ Complete
value	✓	✓	✓	✅ Complete
checked	✓	✓	✓	✅ Complete
disabled	✓	✓	✓	✅ Complete
readonly	✓	✓	✓	✅ Complete
autocomplete	✓	✓	✓	✅ Complete
autocompletetype	✓	✓	⚠️	⚠️ Filter works, handler missing
x-autocompletetype	✓	✓	⚠️	⚠️ Filter works, handler missing
tabindex	✓	✓	✓	✅ Complete
title	✓	✓	✓	✅ Complete (bug fixed!)
rel	✓	✓	✓	✅ Complete
tagname	✓	✓	✓	✅ Complete
data-label	✓	✓	✓	✅ Complete
data-stripe	✓	✓	✓	✅ Complete
id	✓	✓	✓	✅ Complete
name	✓	✓	✓	✅ Complete
class	✓	✓	⚠️	⚠️ Field handler exists, form handler missing
aria-label	✓	✓	✓	✅ Complete
aria-hidden	✓	✓	✓	✅ Complete
aria-disabled	✓	✓	✓	✅ Complete
aria-haspopup	✓	✓	✓	✅ Complete
popover	N/A	✓	N/A	✅ Complete (top-layer handling)
popovertarget	N/A	✓	N/A	✅ Complete (top-layer handling)
popovertargetaction	N/A	✓	N/A	✅ Complete (top-layer handling)
placeholder	✓	❌	❌	❌ Missing

---

🔍 Summary of Required Changes

1. Add placeholder to the filter and handler (if dynamic placeholder changes should be tracked)
2. Fix form class attribute handling (either remove from filter or add handler)
3. Add mutation handlers for legacy autocomplete attributes (for consistency)

---

💡 Additional Observations

• Excellent: The popover attributes are forward-thinking and handle modern web patterns
• Good: All ARIA attributes are properly included for accessibility tracking
• Performance: The filter should provide the claimed performance improvement by reducing mutation callbacks

---

✅ Testing Notes

• Tests passing ✓
• Coverage maintained ✓
• BIT tests passing ✓

The changes are functionally sound, just need the attribute completeness issues addressed.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 706f59a2-0276-4962-b2d4-d92adf0d2d7a

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 41.67%. Comparing base (17ebae1) to head (41e5660).
⚠️ Report is 29 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17832      +/-   ##
==========================================
+ Coverage   41.52%   41.67%   +0.14%     
==========================================
  Files        3556     3568      +12     
  Lines      102951   103442     +491     
  Branches    15476    15572      +96     
==========================================
+ Hits        42754    43110     +356     
- Misses      58380    58500     +120     
- Partials     1817     1832      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[theMickster]

❌ CRITICAL: Missing data-stripe attribute breaks Stripe integration tracking

Details and fix

The data-stripe attribute is collected at line 427 in collect-autofill-content.service.ts:

"data-stripe": this.getPropertyOrAttribute(element, "data-stripe"),

And has a mutation handler at line 1360:

"data-stripe": () => updateAttribute("data-stripe"),

But it's missing from AUTOFILL_ATTRIBUTES. This means if a Stripe integration dynamically updates the data-stripe attribute on a form field, the MutationObserver won't detect it and autofill data will become stale.

Fix: Add to the constant:

DATA_STRIPE: "data-stripe",

Reference: Stripe Elements integration tracking

[theMickster]

⚠️ IMPORTANT: Missing legacy autocomplete attributes may break mutation detection

Details and fix

The code checks three autocomplete attribute variants (line 457-462 in collect-autofill-content.service.ts):

private getAutoCompleteAttribute(element: ElementWithOpId<FormFieldElement>): string {
  return (
    this.getPropertyOrAttribute(element, "x-autocompletetype") ||
    this.getPropertyOrAttribute(element, "autocompletetype") ||
    this.getPropertyOrAttribute(element, "autocomplete")
  );
}

But only autocomplete is in the filter. The mutation handler at line 1351 calls getAutoCompleteAttribute() which checks all three, but mutations won't be detected for x-autocompletetype or autocompletetype changes since they're not in the filter.

Impact: Legacy sites using x-autocompletetype or autocompletetype (older IE/Safari patterns) won't have their autocomplete changes detected.

Fix: Add these legacy attributes:

X_AUTOCOMPLETETYPE: "x-autocompletetype",
AUTOCOMPLETETYPE: "autocompletetype",

While these are legacy attributes, they're explicitly supported by the codebase and should be tracked if a page uses them dynamically.

[theMickster]

💭 QUESTION: Should placeholder attribute be tracked for mutations?

Details and context

The placeholder attribute is collected during initial field scanning (line 405 in collect-autofill-content.service.ts):

placeholder: this.getPropertyOrAttribute(element, "placeholder"),

However:

1. There's no mutation handler for it in updateAutofillFieldElementData() (lines 1329-1372)
2. It's not in this attribute filter

Two possible interpretations:

Option A - Intentionally excluded: Placeholder is only used for initial label detection and doesn't need dynamic tracking. If a page changes placeholders dynamically, we don't care because we've already captured the label.

Option B - Missing handler: We should track placeholder changes and add a mutation handler for it.

Question for team: Is placeholder intentionally excluded from mutation tracking, or should we add both the attribute filter entry and a mutation handler?

Context: Placeholder text is used as a fallback label in the autofill system and some modern SPAs (especially progressive web apps) do change placeholders dynamically based on user state.

[theMickster]

💭 QUESTION: Form class attribute tracked but no mutation handler exists

Details and context

The class attribute is in the filter and is collected for forms (line 235 in collect-autofill-content.service.ts):

htmlClass: this.getPropertyOrAttribute(formElement, "class"),

It also has a mutation handler for fields (line 1341):

class: () => updateAttribute("htmlClass"),

However, there's no handler for form class changes in updateAutofillFormElementData() (lines 1305-1310) - only action, name, id, and method are handled.

Question: Is this intentional, or should we add a class handler to form updates?

// In updateAutofillFormElementData:
const updateActions: Record<string, CallableFunction> = {
  action: () => (dataTarget.htmlAction = this.getFormActionAttribute(element)),
  name: () => updateAttribute("htmlName"),
  id: () => updateAttribute("htmlID"),
  method: () => updateAttribute("htmlMethod"),
  // Missing: class: () => updateAttribute("htmlClass"),
};

If form class changes aren't needed for autofill logic, we could remove it from the filter to reduce mutation observer overhead (the goal of this PR).

[theMickster]

Overall Assessment: REQUEST CHANGES

Critical Issues:

• libs/common/src/autofill/constants/index.ts:59 - Missing data-stripe attribute (has mutation handler)
• apps/browser/src/autofill/services/collect-autofill-content.service.ts:1343 - Pre-existing bug: title mutation handler incorrectly updates tabindex

Important Issues:

• libs/common/src/autofill/constants/index.ts:46 - Missing legacy autocomplete attributes (x-autocompletetype, autocompletetype)

See inline comments for details and recommended fixes.

---

Note on pre-existing bug (line 1343): While this bug exists in unchanged code, it becomes relevant because this PR enables title mutation tracking. The bug causes title attribute changes to incorrectly overwrite tabindex data. Consider fixing it as part of this PR or in a follow-up.

[bw-ghapp]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and all feature flags disabled.

✅ Fortunately, these BIT tests have passed! 🎉

[bw-ghapp]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and the feature flag configuration used by vault.bitwarden.com.

✅ Fortunately, these BIT tests have passed! 🎉