jfx icon indicating copy to clipboard operation
jfx copied to clipboard

9073465: dispatch properly touch events

Open jonatansalemes opened this issue 2 years ago • 6 comments

Hello, Currently our team is developing an touch screen software based in javafx platform for windows. We are using by now ThinkPad x1 Fold and Lenovo Yogabook as target device. Using JavaFx16 version application works very well in both touch devices and receive correct touch events from listeners.
After upgrade to 17+ touch events is not dispatch anymore.

This fix was based on this thread https://stackoverflow.com/questions/69203599/javafx-no-touchevents as ref

We have modified this PR files and build from source jfx project and with that changes both devices getting work properly again and receive touch events.

Bug was opened as id 9073465. Maybe status is for aproval yet.


Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [ ] Commit message must refer to an issue

Integration blocker

 ⚠️ Failed to retrieve information on issue 9073465. Please make sure it exists and is accessible.

Issue

  • ⚠️ Failed to retrieve information on issue 9073465.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/802/head:pull/802
$ git checkout pull/802

Update a local copy of the PR:
$ git checkout pull/802
$ git pull https://git.openjdk.org/jfx pull/802/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 802

View PR using the GUI difftool:
$ git pr show -t 802

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/802.diff

jonatansalemes avatar Jun 20 '22 23:06 jonatansalemes

Hi @jslsolucoes, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jslsolucoes" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

bridgekeeper[bot] avatar Jun 20 '22 23:06 bridgekeeper[bot]

At first glance, this doesn't seem to be a good approach to solve the problem, since you're now treating all WM_TOUCH events as direct touches, independent of what they actually are. Have you figured out why IsTouchEvent() returns false on your machine?

I see that IsTouchEvent() uses GetMessageExtraInfo(), which seems to be a legacy API. Maybe a better option would be to inspect the TOUCHINPUT.dwFlags field, which seems to include bits to identify the origin of the touch point (TOUCHEVENTF_PEN and TOUCHEVENTF_PALM).

mstr2 avatar Jun 22 '22 21:06 mstr2

@mstr2 agree, but this actual code (just two states true|false) reflects in javafx.scene.Scene

       @Override
        public void touchEventBegin(
                long time, int touchCount, boolean isDirect,
                boolean _shiftDown, boolean _controlDown,
                boolean _altDown, boolean _metaDown) {

            if (!isDirect) {
                nextTouchEvent = null;
                return;
            }
            nextTouchEvent = new TouchEvent(
                    TouchEvent.ANY, null, null, 0,
                    _shiftDown, _controlDown, _altDown, _metaDown);
            if (touchPoints == null || touchPoints.length != touchCount) {
                touchPoints = new TouchPoint[touchCount];
            }
            touchPointIndex = 0;
        }

if isDirect was false for any reason touch event is not handled. I really dont understand why this if block is required since an touch event (WM_TOUCH) was triggered. Maybe removing this if, extending TouchEvent with source (PEN,HAND) can be better approach? I`m missing something ?

jonatansalemes avatar Jun 24 '22 05:06 jonatansalemes

Just to be clear about this: Are you missing events when you interact with your screen using fingers, or are you only missing events when you interact with a pen?

In the first case, this would be a bug that needs to be fixed. However, the second case might be more nuanced.

It seems that TouchEvent is only specified to occur for finger touches (pen touches are not mentioned in the documentation). Instead of folding in pen touches into TouchEvent, we might need new API to fully support pen input.

As a work-around, we could inspect the input type when we receive a WM_TOUCH message. For finger touches, we just call HandleViewTouchEvent and return 0 (indicating that we consumed the message). However, for non-finger touches, we return with DefWindowProc to indicate that we don't want to process the message. My guess is that Windows will then convert the pen input to mouse input, so the event shows up as a regular mouse interaction.

mstr2 avatar Jun 24 '22 14:06 mstr2

node.setOnTouchPressed(touchEvent -> {
           //called in javafx16
           //never called in javafx17
        });

we always test application using fingers on touch screen. never by pen.

jonatansalemes avatar Jun 25 '22 00:06 jonatansalemes

I can't find any documentation for GetMessageExtraInfo() that says that we can actually use it to distinguish pen from touch input. This article only says that it can be used to distinguish pen from mouse input by calling it immediately after receiving a mouse-related message.

Maybe we've been relying on unspecified behavior, and you've run into an edge case where that doesn't work. I suggest removing the IsTouchEvent() function, and inspecting the TOUCHINPUT structure directly.

Like I wrote earlier, when we receive non-finger touch events, we should not consume the WM_TOUCH message, but instead return with DefWindowProc.

mstr2 avatar Jun 25 '22 16:06 mstr2

@jslsolucoes This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Mar 31 '23 23:03 bridgekeeper[bot]

@jslsolucoes This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bridgekeeper[bot] avatar Apr 29 '23 02:04 bridgekeeper[bot]