-
Notifications
You must be signed in to change notification settings - Fork 30
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 focus and hovering on context menu show/hide #161
Conversation
* Restore focus of diagram after context menu is closed * Cancel hover timer when diagram looses focus * Clear hover state when diagram looses focus Note that there is a separate hover issue with edges and handles, which is fixed in Sprotty directly. See eclipse-sprotty/sprotty#264 Fixes eclipse-glsp/glsp#496 Fixes eclipse-glsp/glsp#497 Contributed on behalf of STMicroelectronics.
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.
Hello Philip, thank you very much for your contribution! For me, the hiding of the popup works as expected. However, the restoring of the editor focus/active state does not show the expected behavior for me:
All the methods you implemented are triggered but the tab/editor does not appear to be 'active', i.e., the font is not white as it is for text editors. You can also see this if you have a selection with purple resize handles but after opening and closing the context menu the resize handles are gray.
I assume we need some kind of handling in the Theia integration. We already do it in one direction, i.e., in the glsp-diagram-widget we listen to the focus state change of the shell and send a FocusStateChangeAction to our widget if we become inactive. However, for FocusStateChangeActions we never set our widget as active in the shell. But that is just a guess from looking at the code, maybe something else might be necessary as well.
What do you think?
@martin-fleck-at Thanks for the review! Can you please double-check if you can't reproduce it either with those changes? Thanks! |
Theia should automatically activate a widget, if one of its child DOM elements receive focus. So it shouldn't be necessary to actively invoke some additional behavior in Theia. |
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.
@planger Thank you for the explanation and the fix! With it everything works now as expected. However, you added an unnecessary injection which I think we should remove.
...ages/client/src/features/context-menu/selection-service-aware-context-menu-mouse-listener.ts
Outdated
Show resolved
Hide resolved
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.
Looking good now, LGTM!
Thanks @martin-fleck-at for the thorough and fast review! |
Note that there is a separate hover issue with edges and handles, which is fixed in Sprotty directly. See eclipse-sprotty/sprotty#264
Fixes eclipse-glsp/glsp#496
Fixes eclipse-glsp/glsp#497
Contributed on behalf of STMicroelectronics.