-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix focusing on first context menu item #20923
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
Fix focusing on first context menu item #20923
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the context menu opening flow to use promises and proper async handling instead of setTimeout. The changes improve timing control and prevent race conditions when menus are opened in rapid succession.
Key Changes
- Replaced
setTimeoutwith promise-based async flow usingrequestAnimationFramefor better DOM/style synchronization - Added sequence tracking (
openSeq) to prevent race conditions when multiple menus are opened rapidly - Made
reposition()return aPromise<void>for proper async chaining
frontend/src/app/shared/components/op-context-menu/op-context-menu.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/shared/components/op-context-menu/op-context-menu.service.ts
Outdated
Show resolved
Hide resolved
f02fe71 to
f181589
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
frontend/src/app/shared/components/op-context-menu/op-context-menu.service.ts:156
- The
reposition()method setsvisibility: 'visible'on line 156, but theshow()method also setsvisibility: 'visible'on line 107. This creates redundant visibility toggling. Sinceshow()now manages visibility explicitly, remove the visibility style fromreposition()to avoid conflicts and maintain single responsibility.
visibility: 'visible'
frontend/src/app/shared/components/op-context-menu/op-context-menu.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/shared/components/op-context-menu/op-context-menu.service.ts
Outdated
Show resolved
Hide resolved
Ensure menu positioning is complete. Make focus take place after next repaint.
f181589 to
f50eb5a
Compare
Ensure menu positioning is complete. Make focus take place after next repaint.
Ticket
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist