Skip to content

chore: Globally handle focus-visible management for application spanning multiple iframes - #143

Merged
avinashbot merged 2 commits into
mainfrom
focus-visible-iframe
Jul 15, 2025
Merged

chore: Globally handle focus-visible management for application spanning multiple iframes#143
avinashbot merged 2 commits into
mainfrom
focus-visible-iframe

Conversation

@avinashbot

@avinashbot avinashbot commented Jul 9, 2025

Copy link
Copy Markdown
Member

Issue #, if available: AWSUI-60845

Description of changes: Supporting the niche but real use case where components are rendered into a same-origin iframe (typically using a React portal). We keep the reference counting logic, but extend it so that each frame (including the main one) has its own count. This should release first - the matching PR for the components package will follow.

Check my dev pipeline (AwsUi-v3-dwaraa) to see what it looks like.

This is a stopgap to resolve this issue so that the CSS focus-visible migration isn't tied to this simple bugfix. The "default keyboard" approach of the browser behavior compared to the "default mouse" approach of the JS implementation means that there are some async or initial page load scenarios where the focus ring appears where it previously didn't. Some cases may need fixing, some cases we can live with, but that investigation is a separate topic.

Also, as you can tell from the tests, this only works if Cloudscape components are mounted in all the frames where detection needs to happen. Maybe there's a better approach, but at the same time, it also makes sense that I shouldn't start digging up into parent iframes to listen to events if the application isn't even rendered there.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov

codecov Bot commented Jul 9, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.39%. Comparing base (ed3ca20) to head (5189c31).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
+ Coverage   99.36%   99.39%   +0.03%     
==========================================
  Files          33       33              
  Lines         787      833      +46     
  Branches      214      234      +20     
==========================================
+ Hits          782      828      +46     
  Misses          5        5              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@avinashbot
avinashbot force-pushed the focus-visible-iframe branch from dd04585 to d383033 Compare July 10, 2025 10:37
@avinashbot
avinashbot marked this pull request as ready for review July 11, 2025 12:13
@avinashbot
avinashbot requested a review from a team as a code owner July 11, 2025 12:13
@avinashbot
avinashbot requested review from pan-kot and removed request for a team July 11, 2025 12:13
Comment thread src/internal/focus-visible/index.ts Outdated
Comment thread src/internal/focus-visible/__tests__/iframes.test.tsx
@@ -0,0 +1,135 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great tests!

@avinashbot
avinashbot requested a review from pan-kot July 15, 2025 15:11
@avinashbot
avinashbot added this pull request to the merge queue Jul 15, 2025
Merged via the queue into main with commit bd4ad12 Jul 15, 2025
36 checks passed
@avinashbot
avinashbot deleted the focus-visible-iframe branch July 15, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants