On Thu, 24 Feb 2022 18:46:21 GMT, Hima Bindu Meda <d...@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
