fix: UAP buttons misplaced when drawer opens via keyboard#4238
fix: UAP buttons misplaced when drawer opens via keyboard#4238
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4238 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 886 886
Lines 26008 26030 +22
Branches 9421 9435 +14
=======================================
+ Hits 25278 25300 +22
+ Misses 724 683 -41
- Partials 6 47 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return; | ||
| } | ||
|
|
||
| let cleanedUp = false; |
There was a problem hiding this comment.
Does this code ever run? It's in the same function scope, so once the useEffect cleans up, this variable is already out of scope
There was a problem hiding this comment.
My understanding is that since checkPosition is called with requestAnimationFrame, there is a non-zero chance that the component unmounted in-between —that is what cleanedUp tracks.
There was a problem hiding this comment.
The cleanedUp variable is captured by closure in both the checkPosition function and the cleanup function. When cleanup runs and sets cleanedUp = true, the pending rAF callback still has access to it via closure and will see the updated value, causing it to exit.
| requestAnimationFrame(checkPosition); | ||
|
|
||
| return () => { | ||
| cleanedUp = true; |
There was a problem hiding this comment.
why do we introduce cleanedUp flag instead of cancelling the requested animation frames?
There was a problem hiding this comment.
Valid point, I will update it with cancelAnimationFrame. It is cleaner
src/internal/components/drag-handle-wrapper/__tests__/drag-handle-wrapper.test.tsx
Show resolved
Hide resolved
pan-kot
left a comment
There was a problem hiding this comment.
I see that a mock for requestAnimationFrame is added - but where is the test to ensure that the position is recomputed on every animation frame?
| }); | ||
|
|
||
| // Store RAF callbacks globally so flush() can be used in tests | ||
| let rafCallbacks: Set<FrameRequestCallback>; |
There was a problem hiding this comment.
Refactored it to const.
| rafCallbacks = new Set<FrameRequestCallback>(); | ||
| flushAnimationFrames = () => { | ||
| act(() => { | ||
| const callbacks = [...rafCallbacks]; // Snapshot before clearing |
There was a problem hiding this comment.
Why assign to a snapshot instead of just calling the callbacks first and clearing rafCallbacks afterwards?
There was a problem hiding this comment.
Because callbacks re-register themselves (animation loop). If we clear after executing, the newly registered callback is lost.
I see that a unit test for this has now been added, I'm OK with it but think that visual regression test is better here —testing the actual result and not depending so much on implementation details. |
Description
UAP buttons appeared in wrong position when opening drawer via keyboard navigation. Position was calculated during CSS transition before it settled.
Changed to continuous position monitoring while buttons are visible, matching the existing
PortalOverlaypattern.Related links, issue #, if available:
AWSUI-61721How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.