-
Notifications
You must be signed in to change notification settings - Fork 214
Add MouseEnter & MouseMove Event in PopupCloser #2977
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
base: master
Are you sure you want to change the base?
Add MouseEnter & MouseMove Event in PopupCloser #2977
Conversation
The failing check seems unrelated and reminds me of eclipse-platform/eclipse.platform.releng.aggregator#2406. The error message this time is slightly different though:
@merks / @iloveeclipse do you have any hints? Is the error safe to be ignored or should I try some fix? Thank you in advance |
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.
The changes look fine and I tested on Windows, everything is fine. I haven't tested on Linux or Mac yet.
I would still like to postpone merging this PR to the next M1. The reason for that is that the affected class is quite central and the logic involved is not trivial. Applying this PR means that now 2 additional mouse events are handled/propagated and since there are so many subclasses of IInformationControlExtension5
(which is official API) that could be receiving these 2 new events, this rather small change could have big repercussions in consumer code.
Also, the original issue (eclipse-platform/eclipse.platform.swt#2072) does have a functioning workaround and it's very easy: simply press the Tab
key.
Last but not least, the (first) candidate build for M3 is going to be built today so there is no time to test it properly in the community. I'd hate to introduce some unforeseen bug just because we rushed this PR.
@vogella, @akurtakov I see that you were the last 2 people to do working changes to this class so I'd like to know what do you think.
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.
In general, I think this change is fine both from technical side (to support proper usage of Edge inside PopupCloser) as well as from UX perspective (to replace the information control when hovering over it).
Still, some thoughts/concerns on this:
- The PopupCloser shows the information "Press
Tab
from proposal table or click to focus", which becomes invalid/incomplete with this change: it also receives focus when just hovering over it. - Regarding focus: would it be possible to only replace the control when hovering over it without giving it focus and would that make sense? I find it a bit strange that I change the focus control just by hovering as that's quite unusual. When pressing tab or clicking that's kind of expected but not when hovering.
- When repeatedly hovering over the control, it seems to be replaced repeatedly (at least it flickers every time I click in the completion proposal popup and then hover over the additional information control). While that's fine when navigating via tab or when activating it by clicking into it, it's a bit strange that this repeatedly happens just on hovering over the already replaced additional information control, as you can see in this video:
Shall we change the description?
Implemented
|
8b8dd8d
to
68256d6
Compare
Yes, we need to do that.
You are right. I was moving the mouse too fast so it looked a bit weird. But the behavior as you describe it totally makes sense and in my opinion is nothing we should/need to fix. |
68256d6
to
26bc62b
Compare
This commit adds MouseEnter and MouseMove event in the PopupCloser, as Edge browser can only react to mouse movements and not mouse clicks. This also makes the beahviour of PopupCloser consistent with AbstractHoverInformationControl:Closer. fixes eclipse-platform/eclipse.platform.swt#2072
26bc62b
to
f640247
Compare
This commit adds MouseEnter and MouseMove event in the PopupCloser, as Edge browser can only react to mouse movements and not mouse clicks. This also makes the beahviour of PopupCloser consistent with AbstractHoverInformationControl:Closer.
See eclipse-platform/eclipse.platform.swt#1551
fixes eclipse-platform/eclipse.platform.swt#2072
Explanation: The reason why edge browser doesn't react to a mouse hover over the jvadoc tooltip is because the controller responsible for managing the javadoc tooltip in this case is AdditionalInfoController and the closer needed to replace the tooltip on the mouse event is a PopUpCloser. The PopUpCloser doesn't have any mouse movement events (MouseMove & MouseEnter) registered like AbstractHoverInformationControl:Closer (Which is responsible for javadoc tooltip on code hover), hence the javadoc tooltip is not replaced.
Fix: Simply register PopUpCloser with the MouseMove and MouseEnter events and handle these events to trigger the replacement of the control similar to AbstractHoverInformationControl:Closer.