[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8269115: WebView paste event contains old data [v5]
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2022-02-25 14:53:02
Message-ID: EBuASlOyXZBuFrG53x4R7w1aU43KNWSpzdLm3U8goRY=.b69e1cf3-046e-4237-a066-f1fd888baf0a () github ! com
[Download RAW message or body]

On Fri, 25 Feb 2022 07:21:57 GMT, Jay Bhaskar <duke@openjdk.java.net> 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


[prev in list] [next in list] [prev in thread] [next in thread]