Add processing of mouse movements in Edge browser

[amartya4256]

This contribution captures the hover over edge browser and sends a mouse move event to its listener.

contributes to #1540

How to test

• Make sure your IDE is not using Edge as the default browser (at least until other issues have been solved)
• Start the Runtime Workspace application with the start parameter -Dorg.eclipse.swt.browser.DefaultType=Edge
• Hover over some element (class name, method, etc) so that a JavaDoc pops up
• Move the cursor inside the JavaDoc pop-up --> The scrollbars and the navigation buttons should appear in the JavaDoc pop-up:

[github-actions]

Test Results

   483 files  ±0     483 suites  ±0   7m 37s ⏱️ - 2m 18s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 77a5a81. ± Comparison against base commit 435fe7e.

♻️ This comment has been updated with latest results.

[merks]

I would expect some type of checking the location of the pointer over some time interval to see it hasn't changed, i.e., that the user is actually hovering and not just moving the pointer across the control and you happen to catch that after the .5 second delay. I'm not sure what a "normal" hover time is. And shouldn't it produce a org.eclipse.swt.SWT.MouseHover event?

[HeikoKlare]

I would expect some type of checking the location of the pointer over some time interval to see it hasn't changed, i.e., that the user is actually hovering and not just moving the pointer across the control and you happen to catch that after the .5 second delay. I'm not sure what a "normal" hover time is. And shouldn't it produce a org.eclipse.swt.SWT.MouseHover event?

I agree to both:

• From my understanding, the proposal is actually not about "hovering" (i.e., not moving the mouse for some time), but it's just about position tracking (identifying mouse movements and producing according mouse move, enter, and exit events). So there should be some position tracking, such that an event is only fired if there is an actual change in the position. I would also expect this to happen more often than 2 times a second, as you can have quite some mouse movement within 500ms, but I am also not sure what a reasonable value could be (maybe 100ms)?
• Other events like MouseEnter and MouseExit should be created as well, in particular because I guess for the issue to be addressed (missing mouse enter event for the hover information control), those are necessary events.

You should also have the code properly represent this. Currently the method states that it handles browser hover, but isn't it about something like "registring mouse movement tracking"?

[amartya4256]

• derstanding, the proposal is actually not about "hovering" (i.e., not moving the mouse for some time), but it's just about position tracking (identifying mouse movements and producing according mouse move, enter, and exit events). So there should be some position tracking, such tha

After looking at how IE handles this, IE only sends out MouseEnter, Move and Exit events and not hover. If we have to replicate the behavior of IE, I think we should not add MouseHover: also AbstractHoverInformationControl doesn't have any case for MouseHover in its listener. But I would implement MouseEnter and MouseExit. I'll have to manage the state somehow so that I know what event to emit.

Moreover, I can try setting the check every 100 ms. But I also noticed that if i send out events too often, the replacement doesn't occur, i.e. at rate < 10 ms. I'll try with 100 ms; maybe because the schedule never gets an opportunity to execute? Update: At 100 ms, it seems to not be able to trigger the replacement, but at 200 ms, it seems to work.

Also I looked into the code of IE. It doesn't have any implementation for checking if the user stays longer on the hover but it rather creates the Mouse event and sends it directly on the callback. The handling of the delay and replacement is completely handled by the AbstractHoverInformationControlManager (it delays the process by 1 second if the flag is set), but it is completely independent of Browser implementation. The browser is only responsible for sending the event on MouseMove.

eg: If we are just passing by the tooltip and the mouse exits the tooltip in less than one second then there wont be any replacement since the tooltip would close before the replacement which happens after a delay of 1 second. So, I dont think we need to implement that part. But rather just implementing sending out of the right event: MouseEnter, MouseExit and MouseMove should suffice.

[fedejeanne]

@amartya4256 I took a brief look into this PR, I have some minor comments regarding the coding style and I also had an exception while testing.

Stack trace

org.eclipse.swt.SWTException: Failed to execute runnable (org.eclipse.swt.SWTError: No more handles [0x8007139f])
	at org.eclipse.swt.SWT.error(SWT.java:4922)
	at org.eclipse.swt.SWT.error(SWT.java:4837)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4141)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3757)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1042)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:639)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:178)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1454)
Caused by: org.eclipse.swt.SWTError: No more handles [0x8007139f]
	at org.eclipse.swt.SWT.error(SWT.java:4948)
	at org.eclipse.swt.browser.Edge.error(Edge.java:221)
	at org.eclipse.swt.browser.Edge.setupBrowser(Edge.java:561)
	at org.eclipse.swt.browser.Edge.lambda$10(Edge.java:543)
	at org.eclipse.swt.browser.Edge.lambda$5(Edge.java:228)
	at org.eclipse.swt.internal.win32.OS.DispatchMessage(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3752)
	at org.eclipse.jface.internal.text.html.BrowserInformationControl.setVisible(BrowserInformationControl.java:357)
	at org.eclipse.jface.text.AbstractInformationControlManager.showInformationControl(AbstractInformationControlManager.java:1240)
	at org.eclipse.jface.internal.text.StickyHoverManager.showInformationControl(StickyHoverManager.java:262)
	at org.eclipse.jface.internal.text.InformationControlReplacer.showInformationControl(InformationControlReplacer.java:150)
	at org.eclipse.jface.text.AbstractInformationControlManager.internalShowInformationControl(AbstractInformationControlManager.java:1134)
	at org.eclipse.jface.text.AbstractInformationControlManager.presentInformation(AbstractInformationControlManager.java:1120)
	at org.eclipse.jface.text.AbstractInformationControlManager.setInformation(AbstractInformationControlManager.java:431)
	at org.eclipse.jface.internal.text.InformationControlReplacer.computeInformation(InformationControlReplacer.java:116)
	at org.eclipse.jface.text.AbstractInformationControlManager.doShowInformation(AbstractInformationControlManager.java:1101)
	at org.eclipse.jface.text.AbstractInformationControlManager.showInformation(AbstractInformationControlManager.java:1091)
	at org.eclipse.jface.internal.text.InformationControlReplacer.replaceInformationControl(InformationControlReplacer.java:103)
	at org.eclipse.jface.text.AbstractInformationControlManager.replaceInformationControl(AbstractInformationControlManager.java:1268)
	at org.eclipse.jface.text.AbstractHoverInformationControlManager.replaceInformationControl(AbstractHoverInformationControlManager.java:794)
	at org.eclipse.jface.text.TextViewerHoverManager.replaceInformationControl(TextViewerHoverManager.java:297)
	at org.eclipse.jface.text.AbstractHoverInformationControlManager$1.lambda$0(AbstractHoverInformationControlManager.java:856)
	at org.eclipse.ui.internal.PendingSyncExec.run(PendingSyncExec.java:68)
	at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:166)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
	... 22 more

How I tested

• Added start parameter -Dorg.eclipse.swt.browser.DefaultType=Edge
• Created class and added JavaDoc
• Hovered over the name of the class
  • JavaDoc appeared but it was empty ❌
  • Hovering turned the popup into a browser (buttons showed up) ✔️
  • The text was still blank ❌
  • Resizing the popup didn't redraw properly ❌

Video (WARNING!!! Flickering)

[HeikoKlare]

Moreover, I can try setting the check every 100 ms. But I also noticed that if i send out events too often, the replacement doesn't occur, i.e. at rate < 10 ms. I'll try with 100 ms; maybe because the schedule never gets an opportunity to execute? Update: At 100 ms, it seems to not be able to trigger the replacement, but at 200 ms, it seems to work.

This sounds quite strange. Why should the success of the event processing depend on the rate in which it is checked? Sounds to me as if we need to understand the root cause for this. Just trying out what value may work in some specific environment and using this value will not be sustainable. I would expect this to even work even if you schedule this repeatedly, without any delay, as I do not see a conceptual reason why it should not work.

Also I looked into the code of IE. It doesn't have any implementation for checking if the user stays longer on the hover but it rather creates the Mouse event and sends it directly on the callback. The handling of the delay and replacement is completely handled by the AbstractHoverInformationControlManager (it delays the process by 1 second if the flag is set), but it is completely independent of Browser implementation. The browser is only responsible for sending the event on MouseMove.

eg: If we are just passing by the tooltip and the mouse exits the tooltip in less than one second then there wont be any replacement since the tooltip would close before the replacement which happens after a delay of 1 second. So, I dont think we need to implement that part. But rather just implementing sending out of the right event: MouseEnter, MouseExit and MouseMove should suffice.

That's all fine. Of course, we do not and even should not implement anything related to use-case specific delays (like when the hover information control is replaced) in SWT. It is really about separation of concerns:

• This implementation in SWT should be responsible for generating appropriate MouseMove/MoveEnter/MouseExit events. The time to wait until such an event may be generated must not depend on a specific use case for which it might be relevant (like the replacement of the hover information control).
• The hover information control processes the MoveMove/MoveEnter events to identify if there is a hovering for more than 1 second. In that case, it replaces the widget. For this, it requires the according, timely events fired by the underlying browser widget.

[amartya4256]

@HeikoKlare @merks Cleaned up the code a bit. Now, we have all the types of events being sent out as per the state of the cursor w.r.t. browser. Since this also limits the number of events being sent out, now it works with 100 ms as well. But you are right, there are 1000s of events generated in a second, it should be able to process all of them. I need to check what happens to the message at the handler, in case a lot of them are sent. Would keep you posted.

[amartya4256]

@amartya4256 I took a brief look into this PR, I have some minor comments regarding the coding style and I also had an exception while testing.

How I tested

• Added start parameter -Dorg.eclipse.swt.browser.DefaultType=Edge

• Created class and added JavaDoc

• Hovered over the name of the class

  • JavaDoc appeared but it was empty ❌
  • Hovering turned the popup into a browser (buttons showed up) ✔️
  • The text was still blank ❌
  • Resizing the popup didn't redraw properly ❌

You have this issue because you are using Edge browser on your IDE and launching another sdk product with Edge browser there as well. This issue is related to this: #1013 and here are the PRs for that, currently in progress: eclipse-platform/eclipse.platform.ui#2434 & #1548

If you set your default browser to IE in your working IDE and then try to launch the sdk product, you won't have this issue.

[fedejeanne]

You have this issue because you are using Edge browser on your IDE and launching another sdk product with Edge browser there as well.

I switched to IE in my IDE and it works better (the JavaDoc is there and it reacts to hover events) but there's still the flickering (see block below):

WARNING!!! Flickering

The errors are gone.

[fedejeanne]

BTW @amartya4256 I updated the description of this PR and added a "How to test". Feel free to improve it :-)

[HeikoKlare]

@sratz as you have also been working on this issue (via #1545): do you any opinions on the proposed solution?

[fedejeanne]

I'd make the record definition private since it's not needed anywhere else.

I can understand the code, I don't notice any performance change and it works as expected ✔️

The only thing missing is fixing the flickering (see #1551 (comment)). I assume this is the exact same issue as #1122 so I'd say we leave it "unsolved" (in this PR).

Other than that, this PR LGTM (approved!) 👍

[HeikoKlare]

The only thing missing is fixing the flickering (see #1551 (comment)). I assume this is the exact same issue as #1122 so I'd say we leave it "unsolved" (in this PR).

Indeed, that issue remains, but affects every composite embedding an Edge browser. So it should be addressed separately, according to its documentation in #1122.

[sratz]

@sratz as you have also been working on this issue (via #1545): do you any opinions on the proposed solution?

I think this is a good first solution and better than the javascript approach (which does not work universally). 👍

I was wondering: Currently we schedule a 100ms runnable for each Browser instance and even if the cursor is outside the Browser.

I see two possibile optimizations:

• Can we somehow detect that we enter or leave the browser and only do repeated scheduling if we are within its boundaries?
• Can we use a static runnable that we start once and add / remove Browser instances to which deals with all of them in one go?

[HeikoKlare]

I was wondering: Currently we schedule a 100ms runnable for each Browser instance and even if the cursor is outside the Browser.

I see two possibile optimizations:

• Can we somehow detect that we enter or leave the browser and only do repeated scheduling if we are within its boundaries?
• Can we use a static runnable that we start once and add / remove Browser instances to which deals with all of them in one go?

Good point, I also thought about whether we can avoid that this runnable is executed all the time. But since once the mouse enters the browser widgets no mouse move events are fires any more, for which this is actually the workaround, we do not see any possibility of improving the detection of the mouse entering/leaving the browser. Otherwise that mechanism would completely address the current issue that we want to solve, as it's only about the entering event (and not the move event) at all.

Having just one static runnable to avoid this effort to increase by the number of open browser sounds reasonable. What about merging this PR as is to give it a try as soon as possible and I will improve the implementation to use a static runnable as a follow-up PR?

[HeikoKlare]

Okay, let's give this a try. I will prepare the promised follow-up PR as soon as possible.

Failing tests are unrelated and documented: #1523

[HeikoKlare]

• Can we use a static runnable that we start once and add / remove Browser instances to which deals with all of them in one go?

I have just started working on this but found that all the calculations need to be performed in a browser-instance-specific way (as each requires its own coordinate transformation and check for the mouse being inside the browser). Only the scheduling itself could be done in a "static" way. So I am not sure whether it is (1) worth the effort and additional complexity to have a central, WebViewEnvironment-specific scheduling of the movement tracking and whether it is (2) not even better to have this distributed across the browser instances so that the according events are also distributed over time instead of always having a batch of multiple move events at the same point in time. @sratz what do you think?

[merks]

Avoid making it more complicated. I doubt performance is a concern.

[sratz]

Agreed, let's test this out in the current state first.