Re: RFR: 8255940: localStorage is null after window.close() [v3]

[Kevin Rushforth]

On Mon, 7 Feb 2022 08:14:50 GMT, Jay Bhaskar  wrote:

>> Issue: The current implementation of DOMWindow ::localStorage(..) returns 
>> null pointer in case of page is being closed. 
>> Fix: It should not return nullptr , as per the [w3c web storage 
>> spec](https://www.w3.org/TR/2016/REC-webstorage-20160419/) 
>> "User agents should expire data from the local storage areas only for 
>> security reasons or when requested to do so by the user. User agents should 
>> always avoid deleting data while a script that could access that data is 
>> running."
>
> Jay Bhaskar has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - Change Dir Path and use local Dir and set data before clearing 
> localstorage test
>  - Merge branch 'PRLocalstorage' of https://github.com/jaybhaskar/jfx into 
> PRLocalstorage
>  - Merge branch 'master' into PRLocalstorage

You haven't addressed one of my comments on the fix. I'll try to clarify what I 
meant so we're on the same page. Currently (meaning without your fix), the 
following check is done right after getting the page:


// FIXME: We should consider supporting access/modification to local storage
// after calling window.close(). See 
.
if (!page || !page->isClosing()) {
if (m_localStorage)
return m_localStorage.get();
}


There are three cases we might consider:

1. The page is null
2. The page is non-null and is _not_ closing
3. The page is non-null and _is_ closing

In the first two cases we currently return the local-storage if it exists. 
Unless I'm missinsg something, this should also be done for the case of 
non-null and closing (the third case), which simplifies to unconditionally 
returning the local-storage if it exists, or:


if (m_localStorage)
return m_localStorage.get();


If you don't do this, then case 1, the case of a `null` page, will return 
`nullptr`, which is a change in behavior.

The other important part of the change, which you already did, was to remove 
the `if (page->isClosing() return nullptr;` block.

I think those are the only two changes to the existing code that are needed, 
but it's possible I'm missing something.

As for the test, it looks better now. I noted just a few things that could be 
cleaned up below.

modules/javafx.web/src/main/native/Source/WebCore/page/DOMWindow.cpp line 861:

> 859: // after calling window.close(). See 
> .
> 860: if (m_localStorage)
> 861: return m_localStorage.get();

This is not needed here if you take my suggestion of doing it above.

modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java 
line 55:

> 53: 
> 54: private static final File LOCAL_STORAGE_DIR = new 
> File("LocalStorageDir");
> 55: private static final File PRE_LOCKED = new File("zoo_local_storage");

This looks like a copy / paste from `UserDataDirectoryTest`, and is unused (and 
not needed) in this test.

modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java 
line 59:

> 57: private static RandomAccessFile preLockedRaf;
> 58: private static FileLock preLockedLock;
> 59: private static final Random random = new Random();

These are not needed.

modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java 
line 112:

> 110: final WebEngine webEngine = getEngine();
> 111: webEngine.setJavaScriptEnabled(true);
> 112: webEngine.setUserDataDirectory(LOCAL_STORAGE_DIR);

This should be done for all tests, not just this one. Remember that you 
shouldn't assume any particular order for the tests (tests are intended to be 
stateless).

modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java 
line 126:

> 124: //get data
> 125: String s = (String) 
> view.getEngine().executeScript("document.getElementById('key').innerText;");
> 126: assertEquals(s, "1001");

The arguments should be switched (expected comes first).

modules/javafx.web/src/test/java/test/javafx/scene/web/LocalStorageTest.java 
line 137:

> 135: view.getEngine().executeScript("test_local_storage_set();");
> 136: String s = (String) 
> view.getEngine().executeScript("document.getElementById('key').innerText;");
> 137: assertEquals(s, "1001");

Switch arguments.

-

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

Re: Check if a label is ellipsized and then add a tooltip

[Davide Perini]

It's a good workaround even if I would have preferred something built in 
into the framework... :)


Thank you Kevin!


Il 24/02/2022 01:21, Kevin Rushforth ha scritto:
I don't know of a way to determine whether the text of a labeled is 
currently being displayed truncated with ellipses. There isn't a 
(read-only) overrun or displayedText property, either of which would 
allow an app to know.


The only thing I can think of to work around this is to estimate the 
size that the text would need and compare it to the available size.


-- Kevin


On 2/23/2022 5:03 AM, Davide Perini wrote:

Hi,
I have a label inside a gridpane.

It is possible to check if the label is ellipsized and if yes, add a 
tootltip to let the user read the entire label when overing the label 
with the mouse?


Thanks
Davide

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v3]

[Kevin Rushforth]

On Fri, 25 Feb 2022 17:54:48 GMT, Hima Bindu Meda  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.
>
> Hima Bindu Meda has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use ESC key so that the popup, if any, disappears

Looks good. The test now passes for me on all platforms.

-

Marked as reviewed by kcr (Lead).

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Hima Bindu Meda]

On Fri, 25 Feb 2022 13:35:16 GMT, Kevin Rushforth  wrote:

>> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 
>> 212:
>> 
>>> 210: Util.runAndWait(() -> {
>>> 211: robot.mouseRelease(MouseButton.PRIMARY, 
>>> MouseButton.MIDDLE, MouseButton.SECONDARY);
>>> 212: robot.mouseClick(MouseButton.PRIMARY);
>> 
>> I presume clicking the left mouse button is done to dismiss the popup that 
>> will happens when dragging with the right mouse button? This doesn't work on 
>> Windows, so subsequent tests fail. If you change it to pressing the `ESC` 
>> key, then it should work:
>> 
>> 
>> robot.keyType(KeyCode.ESCAPE);
>
> Alternatively, moving the mouse up and/or to the left after releasing all the 
> buttons and before clicking the middle button will work (on Windows the popup 
> doesn't display until after you release the middle button, and is displayed 
> at the then-current mouse position). Pressing the ESC seems easier, but it's 
> up to you.

Added code to press ESC key.Thanks for the sharing the observation.

-

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v3]

[Hima Bindu Meda]

> 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.

Hima Bindu Meda has updated the pull request incrementally with one additional 
commit since the last revision:

  Use ESC key so that the popup, if any, disappears

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/742/files
  - new: https://git.openjdk.java.net/jfx/pull/742/files/4f88401f..84b7184f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=742=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=742=01-02

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/742.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/742/head:pull/742

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

Re: RFR: 8269115: WebView paste event contains old data [v5]

[Kevin Rushforth]

On Fri, 25 Feb 2022 07:21:57 GMT, Jay Bhaskar  wrote:

>> Issue: The DataObject uses m_availMimeTypes as Vector of strings, and 
>> appending mime types in pasteboard operation like setPlainText, Hence it 
>> will append duplicate mime types in m_availMimeTypes , in this case the 
>> length clipboardData.types would be wrong, and duplicates data would be 
>> copied to clipboard target.
>> Solution: Use ListHashSet data Structure instead of Vector for 
>> m_availMimeTypes.
>
> Jay Bhaskar has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - formating change
>  - formating change

The fix looks good to me, but the test still needs additional changes. Then new 
test should fail without your fix and pass with your fix (as of now it passes 
either with or without your fix, so it isn't actually testing the fix).

Also, I think you might have misunderstood Ambarish's comment about the test. 
Rather than modifying the existing `clipboardGetDataOnPaste ` test method, I 
recommend to leave it alone and add a new test method.

modules/javafx.web/src/test/java/test/javafx/scene/web/HTMLEditingTest.java 
line 91:

> 89: executeScript("srcInput.defaultValue").toString(), 
> defaultText);
> 90: assertEquals("Source clipboard onpaste data", 
> getEngine().executeScript("srcInput.value").
> 91: toString(), clipboardData + defaultText);

The expected and actual values are backwards; the first of the two objects 
being compared should be the expected value. I see that this is a preexisting 
bug in the test. I recommend fixing this in the new test (while leaving the 
existing test method as is).

modules/javafx.web/src/test/java/test/javafx/scene/web/HTMLEditingTest.java 
line 96:

> 94: assertEquals("Target onpaste data size", getEngine().
> 95: 
> executeScript("document.getElementById(\"clipboardData\").innerText").toString(),
> 96: "2");

Same comment as above about expected and actual being backwards.

-

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Kevin Rushforth]

On Fri, 25 Feb 2022 13:27:42 GMT, Kevin Rushforth  wrote:

>> Hima Bindu Meda has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Refactor variable names and update the formatting
>
> tests/system/src/test/java/test/robot/javafx/web/PointerEventTest.java line 
> 212:
> 
>> 210: Util.runAndWait(() -> {
>> 211: robot.mouseRelease(MouseButton.PRIMARY, MouseButton.MIDDLE, 
>> MouseButton.SECONDARY);
>> 212: robot.mouseClick(MouseButton.PRIMARY);
> 
> I presume clicking the left mouse button is done to dismiss the popup that 
> will happens when dragging with the right mouse button? This doesn't work on 
> Windows, so subsequent tests fail. If you change it to pressing the `ESC` 
> key, then it should work:
> 
> 
> robot.keyType(KeyCode.ESCAPE);

Alternatively, moving the mouse up and/or to the left after releasing all the 
buttons and before clicking the middle button will work (on Windows the popup 
doesn't display until after you release the middle button, and is displayed at 
the then-current mouse position). Pressing the ESC seems easier, but it's up to 
you.

-

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Kevin Rushforth]

On Fri, 25 Feb 2022 11:15:23 GMT, Hima Bindu Meda  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.
>
> Hima Bindu Meda has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Refactor variable names and update the formatting

All changes look good. While doing final testing, I am seeing test failures on 
Windows caused by the popup not being dismissed. See below.

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

> 210: Util.runAndWait(() -> {
> 211: robot.mouseRelease(MouseButton.PRIMARY, MouseButton.MIDDLE, 
> MouseButton.SECONDARY);
> 212: robot.mouseClick(MouseButton.PRIMARY);

I presume clicking the left mouse button is done to dismiss the popup that will 
happens when dragging with the right mouse button? This doesn't work on 
Windows, so subsequent tests fail. If you change it to pressing the `ESC` key, 
then it should work:


robot.keyType(KeyCode.ESCAPE);

-

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Hima Bindu Meda]

On Fri, 25 Feb 2022 00:30:28 GMT, Kevin Rushforth  wrote:

>> 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.
>
> GitHub is showing more context than it should have, so my comment might be 
> confusing. I only meant to suggest that you revert the reformatting of the 
> existing constructor. Everything inside the `#if` looks fine.

Reverted the change from already existing webkit shared code.

>> 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).
>
>> move the runAndWait outside the list...
> 
> I meant "loop"

updated the indentation

-

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Hima Bindu Meda]

On Thu, 24 Feb 2022 23:53:04 GMT, Kevin Rushforth  wrote:

>> Hima Bindu Meda has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Refactor variable names and update the formatting
>
> 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.

Agree.Updated the name to "buttonMask" and refactored accordingly.

> 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.

Formatted as per comment and aligned. Please review.

> 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`?

As the w3c spec also mentions that buttons gives the current state of the 
device buttons as a bitmask, MouseButtonMask is more suitable.Made the changes 
accordingly.

> 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.

Used single latch and initialised 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.

changed to buttons

> 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.

changes done as per the comment.

> 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.

done.

> 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`.

Added single latch initialised to 2.

> 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 `,`

done.

-

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

Re: RFR: 8278759 : PointerEvent: buttons property set to 0 when mouse down [v2]

[Hima Bindu Meda]

> 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.

Hima Bindu Meda has updated the pull request incrementally with one additional 
commit since the last revision:

  Refactor variable names and update the formatting

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/742/files
  - new: https://git.openjdk.java.net/jfx/pull/742/files/ec32ba93..4f88401f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=742=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=742=00-01

  Stats: 60 lines in 7 files changed: 4 ins; 7 del; 49 mod
  Patch: https://git.openjdk.java.net/jfx/pull/742.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/742/head:pull/742

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