[PM-27798] Prevent inline menu from opening on the page outside of the viewport

[jprusik]

🎟️ Tracking

PM-27798

📔 Objective

Add checks to verify the inline menu is fully within the viewport. Dismiss it otherwise.

📸 Screenshots

Before

show-inline-menu-if-outside-viewport.mp4

After

hide-inline-menu-if-outside-viewport.mp4

🦮 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

[jprusik]

unrelated cleanup

[jprusik]

verified in testing that these values can explicitly be null

[jprusik]

While this can be used for any element, it is currently only employed for checking the (newly initialized) inline menu iframe

[jprusik]

unrelated change (to match the same pattern used in apps/browser/src/autofill/services/autofill-overlay-content.service.ts)

[github-actions]

Checkmarx One – Scan Summary & Details – c9d448c1-7ade-4868-b3c0-e9f7638dc618

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	Client_Use_Of_Iframe_Without_Sandbox	/apps/browser/src/autofill/overlay/inline-menu/iframe-content/autofill-inline-menu-iframe.service.ts: 90

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 41.54%. Comparing base (854f2ab) to head (d734fa8).
⚠️ Report is 63 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #17664   +/-   ##
=======================================
  Coverage   41.53%   41.54%           
=======================================
  Files        3551     3551           
  Lines      102239   102255   +16     
  Branches    15340    15348    +8     
=======================================
+ Hits        42465    42481   +16     
  Misses      57985    57985           
  Partials     1789     1789           

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

[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! 🎉

[claude]

Claude finished @jprusik's task —— View job

---

Code Review Complete

I've reviewed the PR and the implementation looks solid. The viewport checking logic is well-implemented with comprehensive test coverage. Here are my findings:

Finding 1: Early return prevents fade-in animation execution

💭 In updateIframePosition (line 285), when the viewport check fails, the method returns early which prevents the fade-in animation logic below from executing. While this is intentional behavior for out-of-viewport elements, consider if the ordering could be clearer with a comment explaining that viewport validation must occur before animation.

Context

private updateIframePosition(position: Partial<CSSStyleDeclaration>) {
  if (!globalThis.document.hasFocus()) {
    return;
  }

  const styles = this.fadeInTimeout ? Object.assign(position, { opacity: "0" }) : position;
  this.updateElementStyles(this.iframe, styles);

  const elementHeightCompletelyInViewport = this.isElementCompletelyWithinViewport(
    this.iframe.getBoundingClientRect(),
  );

  if (!elementHeightCompletelyInViewport) {
    this.forceCloseInlineMenu();
    return; // Prevents fade-in below
  }

  if (this.fadeInTimeout) {
    this.handleFadeInInlineMenuIframe();
  }
  // ...
}

The early return is correct, but a comment like "// Close menu before animation if outside viewport" might help future maintainers.

Finding 2: Visual Viewport API preference is well-justified

👍 The use of visualViewport API when available (line 330) is excellent - it correctly handles mobile browsers and zoom scenarios. The fallback to window.innerHeight/Width is appropriate. The comment explaining this is helpful.

Finding 3: Test coverage is comprehensive

👍 The test suite added 253 lines covering all edge cases:

• All four viewport boundaries (top, bottom, left, right)
• Zero-dimension elements
• VisualViewport API usage
• Integration with existing fade-in logic

This is thorough testing that gives confidence in the implementation.

Finding 4: Type safety improvement for InlineMenuPosition

👍 Changing button?: InlineMenuElementPosition to button?: InlineMenuElementPosition | null in the type definition correctly reflects that these values can be explicitly null (as noted in your self-review comment).

Finding 5: Unrelated cleanup changes are appropriate

👍 The TOTP comment fixes in overlay.background.ts (fixing "postion" → "position" and "totp" → "TOTP") and the threshold comment pattern match in collect-autofill-content.service.ts are good housekeeping changes that improve code quality without affecting functionality.

---

Summary

This PR successfully implements viewport boundary checking for the inline menu with no significant issues found. The implementation is clean, well-tested, and handles edge cases appropriately. The code follows established patterns in the codebase and the test coverage is excellent.

No blocking issues identified. The PR is ready to merge from a code quality perspective.

[dan-livefront]

One minor nit, but I'll leave it to your discretion whether you want to address it. Great work here!

[dan-livefront]

can this be collapsed to a single return expression and avoid the consts for reduced lines?

[jprusik]

yeah, I had also considered early returns for each check as well, but it felt a little overkill to me and harder to quickly scan/understand for marginal perf gain.

The verbosity is okay in my mind to help signal intent, but we could revisit it in future work if you don't consider this a blocking concern.