Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STCOM-1387 ExportCSV download link not working in Modals #2400

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Conversation

JohnC-80
Copy link
Contributor

@JohnC-80 JohnC-80 commented Dec 2, 2024

STCOM-1387

The focus-trapping logic of Modal (via focus-trap) causes the click() event not to trigger on the download link rendered by ExportCSV because the link is appended outside of the focus-trapped element (in the body.)

Approach:
Rendering the link within the div#OverlayContainer places it in within the focus-trapping boundary. Since this element is always present in the stripes UI, it's okay to render here. For tests where the OverlayContainer may not exist, the logic falls back to the body.

Additionally, the exportCSV logic would simply click the link and then remove it. This may not cause the focus to move, but it's a safe accessibility measure to return focus to the element it was originally on prior to clicking the download trigger.

A similar fix is required in stripes-util since we're not sure how many ui modules use THAT version of ExportCSV... That PR: folio-org/stripes-util#91

@JohnC-80 JohnC-80 requested review from zburke and a team December 2, 2024 21:45
Copy link

github-actions bot commented Dec 2, 2024

Bigtest Unit Test Results

    1 files  ±0      1 suites  ±0   15s ⏱️ ±0s
1 533 tests ±0  1 525 ✅ ±0  8 💤 ±0  0 ❌ ±0 
1 535 runs  ±0  1 527 ✅ ±0  8 💤 ±0  0 ❌ ±0 

Results for commit 7d0e795. ± Comparison against base commit b0868af.

♻️ This comment has been updated with latest results.

Copy link

sonarqubecloud bot commented Dec 2, 2024

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Thank you, thank you for the clear explanation of the problem, its solution, and the attendant a11y concerns and mitigations. It's so helpful to have that extra context in the comments.

@JohnC-80 JohnC-80 merged commit 45756bb into master Dec 3, 2024
15 checks passed
@JohnC-80 JohnC-80 deleted the STCOM-1387 branch December 3, 2024 14:17
github-actions bot pushed a commit that referenced this pull request Dec 3, 2024
## [STCOM-1387](https://folio-org.atlassian.net/browse/STCOM-1387)

The focus-trapping logic of Modal (via `focus-trap`) causes the
`click()` event not to trigger on the download link rendered by
`ExportCSV` because the link is appended outside of the focus-trapped
element (in the body.)

Approach: 
Rendering the link within the `div#OverlayContainer` places it in within
the `focus-trapping` boundary. Since this element is always present in
the stripes UI, it's okay to render here. For tests where the
`OverlayContainer` may not exist, the logic falls back to the body.

Additionally, the `exportCSV` logic would simply click the link and then
remove it. This may not cause the focus to move, but it's a safe
accessibility measure to return focus to the element it was originally
on prior to clicking the download trigger.

A similar fix is required in `stripes-util` since we're not sure how
many ui modules use THAT version of `ExportCSV`... That PR:
folio-org/stripes-util#91
zburke pushed a commit that referenced this pull request Dec 17, 2024
The focus-trapping logic of Modal (via `focus-trap`) causes the
`click()` event not to trigger on the download link rendered by
`ExportCSV` because the link is appended outside of the focus-trapped
element (in the body.)

Approach:
Rendering the link within the `div#OverlayContainer` places it in within
the `focus-trapping` boundary. Since this element is always present in
the stripes UI, it's okay to render here. For tests where the
`OverlayContainer` may not exist, the logic falls back to the body.

Additionally, the `exportCSV` logic would simply click the link and then
remove it. This may not cause the focus to move, but it's a safe
accessibility measure to return focus to the element it was originally
on prior to clicking the download trigger.

A similar fix is required in `stripes-util` since we're not sure how
many ui modules use THAT version of `ExportCSV`... That PR:
folio-org/stripes-util#91

(cherry picked from commit 45756bb)
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.

4 participants