RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down

[Kevin Rushforth]

On Thu, 24 Feb 2022 18:46:21 GMT, Hima Bindu Meda <duke at openjdk.java.net> wrote:

> Basically, buttons property is a mask which represents the button/buttons clicked on the mouse.
>     It is observed that event.buttons property is set to 0 when there is mouse press or drag event.This behaviour is observed only with javafx webView.Other browsers set the buttons property to 1, when there is mouse press or drag.
>      The issue happens because the buttons property is not updated in the framework.
>      Added implementation to update and propagate the buttons property from javafx platform to native webkit.Added a robot test case for the same.
>    Performed sanity testing with the added implementation and the buttons property is compliant with the specification mentioned in https://w3c.github.io/pointerevents/#the-buttons-property.

The fix and test both look good. I verified that the new test fails without your fix and passes with the fix.

I left a few suggestions and minor formatting issues.

modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 844:

> 842:                                                 //intermediate mouse events that can change text selection.
> 843:                 && twkProcessMouseEvent(getPage(), me.getID(),
> 844:                                         me.getButton(), me.getButtons(), me.getClickCount(),

I wonder if `buttonMask` would be a better (more descriptive) name on the Java side, and consequently, in the JNI call? Within WebKit, using `buttons` makes sense, since that's what they call it. This is just a suggestion. I don't mind if you leave it as is.

modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 1089:

> 1087:         }
> 1088:         final int buttonMask = ((ev.isPrimaryButtonDown() ? WCMouseEvent.BUTTON1 : WCMouseEvent.NOBUTTON) | (ev.isMiddleButtonDown() ? WCMouseEvent.BUTTON2 : WCMouseEvent.NOBUTTON) |
> 1089:                                     (ev.isSecondaryButtonDown() ? WCMouseEvent.BUTTON3 : WCMouseEvent.NOBUTTON));

Minor: I would wrap this so each of the three bits is on its own line (so the first line isn't so long). Also, the indentation should either be 8 spaces or line up with the RHS expression on the previous line.

modules/javafx.web/src/main/native/Source/WebCore/platform/PlatformMouseEvent.h line 48:

> 46:     enum SyntheticClickType : int8_t { NoTap, OneFingerTap, TwoFingerTap };
> 47: #if PLATFORM(JAVA)
> 48:     enum MouseButtonPressed : uint8_t { NoButtonPress = 0, LeftButtonPress, RightButtonPress, MiddleButtonPress = 4 };

Do you think `MouseButtonMask`, `LeftButtonMask`, etc., would be better names than `...Pressed`?

modules/javafx.web/src/main/native/Source/WebCore/platform/PlatformMouseEvent.h line 76:

> 74:        PlatformMouseEvent(const IntPoint& position, const IntPoint& globalPosition, MouseButton button, PlatformEvent::Type type,
> 75:                            int clickCount, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, WallTime timestamp, double force,
> 76:                            SyntheticClickType syntheticClickType, PointerID pointerId = mousePointerID)

I recommend reverting this change, since this is in WebKit shared code and the only change you made is in formatting. It will help avoid future merge conflicts.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 89:

> 87: 
> 88:     private static CountDownLatch loadLatch;
> 89:     private static CountDownLatch startupLatch;

I would reverse the order here, since startup latch is triggered first. Alternatively, you can use a single latch and initialize it to 2.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 128:

> 126:     }
> 127: 
> 128:     public String mouseButtonDrag(MouseButton... button) {

I recommend changing the name of the parameter to `buttonArray` (or `buttonArr` or just `buttons` if you want it shorter), since this varargs parameter is an array of potentially more than one button.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 137:

> 135:         for (int i = 0; i < DRAG_DISTANCE; i++) {
> 136:                 final int c = i;
> 137:                 Util.runAndWait(() -> {

Minor: I think you can move the `runAndWait` outside the list, although that will change the timing slightly.

Whether or not you do this, the indentation is a little off (the first several lines are indented too much and the closing paren for the `Util.runAndWait` isn't lined up).

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 140:

> 138:                     // Move the mouse backwards so that the pointer does not stay on the popup,if any.
> 139:                     robot.mouseMove((int)(scene.getWindow().getX() + scene.getX() + DX - c),
> 140:                         (int)(scene.getWindow().getY() + scene.getY() + DY) );

Minor: you don't need the extra space between the last two ')'.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 150:

> 148:         });
> 149: 
> 150:         return buttonMask;

if you move the `Integer.parseInt` from the caller to here (and change the return type to `int`) it will simplify the callers a bit. Just a suggestion.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 156:

> 154:     public void testLeftButtonDrag() {
> 155:         int result = Integer.parseInt(mouseButtonDrag(MouseButton.PRIMARY));
> 156:         Assert.assertEquals(LEFT_BUTTON_DRAG,result);

Minor: there should be a space after the `,`. Also, you might consider using a static import for `assertEquals`.

This applies to all of the test methods.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 174:

> 172:     public void testLeftMiddleButtonDrag() {
> 173:         int result = Integer.parseInt(mouseButtonDrag(MouseButton.PRIMARY, MouseButton.MIDDLE));
> 174:         Assert.assertEquals((LEFT_BUTTON_DRAG|MIDDLE_BUTTON_DRAG),result);

Minor: surround the binary `|` operator with a space on either side.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 201:

> 199:         new Thread(() -> Application.launch(TestApp.class, (String[])null)).start();
> 200:         waitForLatch(loadLatch, 10, "Timeout waiting for loading webpage().");
> 201:         waitForLatch(startupLatch, 15, "Timeout waiting for FX runtime to start");

Since `startupLatch` will trigger first, you should either switch the two calls, or use a single latch (initialized to 2) with a single call to `waitForLatch`.

tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 215:

> 213:     public void resetTest() {
> 214:         Util.runAndWait(() -> {
> 215:             robot.mouseRelease(MouseButton.PRIMARY,MouseButton.MIDDLE,MouseButton.SECONDARY);

Minor: add a space after each `,`

-------------

PR: https://git.openjdk.java.net/jfx/pull/742