Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8143596

FXCanvas does not forward touch gestures to embedded scene

    Details

    • Subcomponent:
    • CPU:
      x86
    • OS:
      other

      Description

      FULL PRODUCT VERSION :


      ADDITIONAL OS VERSION INFORMATION :
      Non OS-specific problem.

      A DESCRIPTION OF THE PROBLEM :
      As reported in JDK-8088262 already, FXCanvas does not forward any touch gesture events to the embedded scene.

      The Eclipse GEF project provides an FXCanvasEx to compensate this problem (http://git.eclipse.org/c/gef/org.eclipse.gef4.git/tree/org.eclipse.gef4.fx.swt/src/org/eclipse/gef4/fx/swt/canvas/FXCanvasEx.java).

      It internally uses a modified version of the helper class (http://git.eclipse.org/c/gef/org.eclipse.gef4.git/tree/org.eclipse.gef4.fx.swt/src/org/eclipse/gef4/fx/swt/gestures/SwtToFXGestureConverter.java) mentioned by Jan Köhnlein in JDK-8088262.

      However, this helper class accesses JDK internal API, so that the workaround will probably not work with Jigsaw:

      bin -> javafx.graphics
         org.eclipse.gef4.fx.swt.gestures.SwtToFXGestureConverter (bin)
            -> com.sun.javafx.tk.TKSceneListener JDK internal API (javafx.graphics)
         org.eclipse.gef4.fx.swt.gestures.SwtToFXGestureConverter$3$1 (bin)
            -> com.sun.javafx.tk.TKSceneListener JDK internal API (javafx.graphics)

      EXPECTED VERSUS ACTUAL BEHAVIOR :
      EXPECTED -
      Have full support for forwarding touch gesture events to the embedded scene within FXCanvas.
      ACTUAL -
      No touch gesture events are forwarded by FXCanvas.

      REPRODUCIBILITY :
      This bug can be reproduced always.

      1. JDK-8143596.09_09_16.patch
        40 kB
        Alexander Nyssen
      2. JDK-8143596.17_08_16.patch
        36 kB
        Kevin Rushforth
      3. JDK-8143596.18_08_16.patch
        36 kB
        Kevin Rushforth

        Issue Links

          Activity

          Hide
          kcr Kevin Rushforth added a comment -
          Attached patch from Alexander Nyssen.
          Show
          kcr Kevin Rushforth added a comment - Attached patch from Alexander Nyssen.
          Hide
          kcr Kevin Rushforth added a comment -
          Here are the notes from the patch itself (since they cannot be part of the commit message, but belong here).

          ---------------------------
          - Ensured SWT magnify, rotate, pan, and swipe events are properly forwarded by FXCanvas to its embedded scene, including proper handling of inertia events.
          - Ensured SWT mouse wheel events (synthesized from pan gesture events) are suppressed while a gesture is active.
          - Ensured scroll events are more fine grained for gesture eventes (multiplier: 5.0) than for mouse wheel scroll (multiplier: 40.0).
          - Switched to SWT 4.6 (3.105.0), as rotate gesture events were broken on MacOSX 64-bit in SWT 3.7.2 (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=349812) and testing can thus not be performed on Mac.
          Show
          kcr Kevin Rushforth added a comment - Here are the notes from the patch itself (since they cannot be part of the commit message, but belong here). --------------------------- - Ensured SWT magnify, rotate, pan, and swipe events are properly forwarded by FXCanvas to its embedded scene, including proper handling of inertia events. - Ensured SWT mouse wheel events (synthesized from pan gesture events) are suppressed while a gesture is active. - Ensured scroll events are more fine grained for gesture eventes (multiplier: 5.0) than for mouse wheel scroll (multiplier: 40.0). - Switched to SWT 4.6 (3.105.0), as rotate gesture events were broken on MacOSX 64-bit in SWT 3.7.2 (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=349812) and testing can thus not be performed on Mac.
          Hide
          kcr Kevin Rushforth added a comment -
          This one needs more thought and careful coordination if the upgrade to SWT 4.6 is required.

          Switching to a newer version of any external dependency requires a separate, explicit step; we need to carefully control all external dependencies both from a third-party licensing point of view as well as from a technical point of view.

          On the technical side, we cache all imports on an internal server for our production builds, and need to coordinate a request to update the cached bundles; otherwise, this change will break our production builds. Additionally, the use of SWT 4.6 means that we won't be able to run on Linux until JDK-8156491 is fixed, unless we explicitly set the system property "jdk.gtk.version=3".

          So I see two choices:

          1) Don't upgrade the dependent SWT library. Any tests that fail on Mac as a result will need to be disabled/ignored on Mac.

          2) File a new bug to update the dependent SWT library to 4.6, and link that bug as "blocks" this bug. Updating to SWT 4.6 will also require either waiting until JDK-8156491 is fixed or it will require updating our build and test scripts to set "jdk.gtk.version=3" on Linux.

          I have a feeling that #1 will be easiest, unless there are other reasons it won't work.
          Show
          kcr Kevin Rushforth added a comment - This one needs more thought and careful coordination if the upgrade to SWT 4.6 is required. Switching to a newer version of any external dependency requires a separate, explicit step; we need to carefully control all external dependencies both from a third-party licensing point of view as well as from a technical point of view. On the technical side, we cache all imports on an internal server for our production builds, and need to coordinate a request to update the cached bundles; otherwise, this change will break our production builds. Additionally, the use of SWT 4.6 means that we won't be able to run on Linux until JDK-8156491 is fixed, unless we explicitly set the system property "jdk.gtk.version=3". So I see two choices: 1) Don't upgrade the dependent SWT library. Any tests that fail on Mac as a result will need to be disabled/ignored on Mac. 2) File a new bug to update the dependent SWT library to 4.6, and link that bug as "blocks" this bug. Updating to SWT 4.6 will also require either waiting until JDK-8156491 is fixed or it will require updating our build and test scripts to set "jdk.gtk.version=3" on Linux. I have a feeling that #1 will be easiest, unless there are other reasons it won't work.
          Hide
          kcr Kevin Rushforth added a comment -
          I note that even if we choose option #1 above, developers could still do manual testing on Mac with a newer swt library.
          Show
          kcr Kevin Rushforth added a comment - I note that even if we choose option #1 above, developers could still do manual testing on Mac with a newer swt library.
          Hide
          kcr Kevin Rushforth added a comment -
          Raising the priority to P3 since the current workaround for JDK 8 will no longer work in JDK 9 due to its reliance on internal packages.
          Show
          kcr Kevin Rushforth added a comment - Raising the priority to P3 since the current workaround for JDK 8 will no longer work in JDK 9 due to its reliance on internal packages.
          Hide
          kcr Kevin Rushforth added a comment -
          Attached file from Alexander Nyssen (sorry I missed it when you sent it earlier).
          Show
          kcr Kevin Rushforth added a comment - Attached file from Alexander Nyssen (sorry I missed it when you sent it earlier).
          Hide
          kcr Kevin Rushforth added a comment -
          The newer version removes the change to the SWT library against which we build / test (option #1 in the earlier comment).

          Separately, I filed a related JBS issue to track upgrading to a newer SWT which we really should do at some point: JDK-8165562
          Show
          kcr Kevin Rushforth added a comment - The newer version removes the change to the SWT library against which we build / test (option #1 in the earlier comment). Separately, I filed a related JBS issue to track upgrading to a newer SWT which we really should do at some point: JDK-8165562
          Hide
          anyssen Alexander Nyssen added a comment - - edited
          Having performed some additional testing, I observed that my patch(es) would introduce a regression. That is, JavaFX scroll events that originate from native SWT mouse wheel events would now invalidly be forwarded as 'inertia' events on Mac.

          The reason is that SWT does neither flag emulated nor inertia events as such and both, mouse wheel as well as pan gesture events, are always sent by SWT when using either the mouse wheel or the touch pad (i.e. either native mouse wheel and emulated pan gesture events or native pan gesture and emulated mouse wheel events). Currently, when both occur as a pair (having the same timestamp), the pan gesture event is being used to infer the JavaFX scroll event (discarding the mouse wheel event), even when it originated from a mouse wheel interaction and is thus emulated. As all SWT pan gesture events that occur outside of pan gestures are currently recognized as inertia events this results in the above described misbehavior.

          I will rework the patch to handle this properly. The strategy will be to identify native pan gesture events that occur outside of a gesture (i.e. native inertia events) and to give precedence to native mouse wheel events (i.e. forward them as JavaFX scroll events) in case they coincide with such emulated pan gesture events.
          Show
          anyssen Alexander Nyssen added a comment - - edited Having performed some additional testing, I observed that my patch(es) would introduce a regression. That is, JavaFX scroll events that originate from native SWT mouse wheel events would now invalidly be forwarded as 'inertia' events on Mac. The reason is that SWT does neither flag emulated nor inertia events as such and both, mouse wheel as well as pan gesture events, are always sent by SWT when using either the mouse wheel or the touch pad (i.e. either native mouse wheel and emulated pan gesture events or native pan gesture and emulated mouse wheel events). Currently, when both occur as a pair (having the same timestamp), the pan gesture event is being used to infer the JavaFX scroll event (discarding the mouse wheel event), even when it originated from a mouse wheel interaction and is thus emulated. As all SWT pan gesture events that occur outside of pan gestures are currently recognized as inertia events this results in the above described misbehavior. I will rework the patch to handle this properly. The strategy will be to identify native pan gesture events that occur outside of a gesture (i.e. native inertia events) and to give precedence to native mouse wheel events (i.e. forward them as JavaFX scroll events) in case they coincide with such emulated pan gesture events.
          Hide
          anyssen Alexander Nyssen added a comment - - edited
          This revised patch adds proper handling of inertia events as laid out before (i.e. inertia pan gesture events are now identified via their temporal relation to preceding pan gesture events).

          In addition, this patch fixes that horizontal mouse wheel events did not provide a valid xDirection on Windows (the divisor computation within SWTEvents.getWheelRotation() was lacking the case), and that scroll events on Mac were not properly scaled (again, the divisor computation within SWTEvents.getWheelRotation() had to be adjusted).
          Show
          anyssen Alexander Nyssen added a comment - - edited This revised patch adds proper handling of inertia events as laid out before (i.e. inertia pan gesture events are now identified via their temporal relation to preceding pan gesture events). In addition, this patch fixes that horizontal mouse wheel events did not provide a valid xDirection on Windows (the divisor computation within SWTEvents.getWheelRotation() was lacking the case), and that scroll events on Mac were not properly scaled (again, the divisor computation within SWTEvents.getWheelRotation() had to be adjusted).
          Hide
          anyssen Alexander Nyssen added a comment - - edited
          I adopted my latest patch to the current tip and created a webrev for it (which obsoletes the patch files uploaded here):

          http://cr.openjdk.java.net/~anyssen/8143596/webrev.00/
          Show
          anyssen Alexander Nyssen added a comment - - edited I adopted my latest patch to the current tip and created a webrev for it (which obsoletes the patch files uploaded here): http://cr.openjdk.java.net/~anyssen/8143596/webrev.00/
          Hide
          azvegint Alexander Zvegintsev added a comment -
          The fix looks good to me.
          I have only one minor comment: I think that we shouldn't throw IllegalStateException in sendGestureEventToFX in FXCanvas. It might break this function's behavior, if some new gesture(unlikely, but who knows) will be added.
          Show
          azvegint Alexander Zvegintsev added a comment - The fix looks good to me. I have only one minor comment: I think that we shouldn't throw IllegalStateException in sendGestureEventToFX in FXCanvas. It might break this function's behavior, if some new gesture(unlikely, but who knows) will be added.
          Hide
          anyssen Alexander Nyssen added a comment -
          I have uploaded an updated webrev to http://cr.openjdk.java.net/~anyssen/8143596/webrev.01/. The IllegalStateException is no longer thrown, unknown gesture events are now simply ignored. I have also fixed some whitespace issues.
          Show
          anyssen Alexander Nyssen added a comment - I have uploaded an updated webrev to http://cr.openjdk.java.net/~anyssen/8143596/webrev.01/ . The IllegalStateException is no longer thrown, unknown gesture events are now simply ignored. I have also fixed some whitespace issues.
          Hide
          azvegint Alexander Zvegintsev added a comment -
          looks good to me.
          Show
          azvegint Alexander Zvegintsev added a comment - looks good to me.
          Hide
          kcr Kevin Rushforth added a comment -
          I verified that it compiles and doesn't break the build and that the existing unit tests still run. I was on Linux, so couldn't test the actual gesture code.

          [~azvegint] If you are happy with it, then go ahead and push it.
          Show
          kcr Kevin Rushforth added a comment - I verified that it compiles and doesn't break the build and that the existing unit tests still run. I was on Linux, so couldn't test the actual gesture code. [~azvegint] If you are happy with it, then go ahead and push it.
          Show
          azvegint Alexander Zvegintsev added a comment - http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/d1e9b7548bd0

            People

            • Assignee:
              anyssen Alexander Nyssen
              Reporter:
              webbuggrp Webbug Group
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: