On Thu, 23 Feb 2023 05:21:37 GMT, Karthik P K <k...@openjdk.org> wrote:
>> When a large number of items were scrolled in the `ChoiceBox`, the scrolled
>> offset was carried forward when the list is replaced with small number of
>> items. Hence the scroll up arrow was displayed with empty popup.
>>
>> Changed code to scroll to top before popup display when content height of
>> `ChoiceBox` is smaller than the scrolled offset.
>>
>> Added system test to validate the fix.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
The fix seems fine. The test is not completely stable, though. I ran it several
times and it fails occasionally. I left a comment pointing to a problem in the
test (sleeping on the wrong thread) that might explain it. Even if not, that
needs to be fixed. I left a couple minor comments as well.
tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java
line 40:
> 38: import javafx.scene.control.skin.ChoiceBoxSkin;
> 39: import javafx.scene.control.skin.ChoiceBoxSkinNodesShim;
> 40: import javafx.scene.control.skin.ContextMenuSkin;
Minor: unused import can be removed
tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java
line 73:
> 71: public class ChoiceBoxScrollUpOnCollectionChangeTest {
> 72: static CountDownLatch startupLatch = new CountDownLatch(1);
> 73: static CountDownLatch scrollLatch = new CountDownLatch(1);
Minor: unused field can be removed
tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java
line 101:
> 99: }
> 100:
> 101: Util.sleep(400); // Wait for up arrow to get loaded in UI
Sleeping on the JavaFX application thread is both ineffective and undesirable.
If the sleep is needed, then split this into two invocations of `runAndWait`
(before and after the sleep), and do the sleep on the test thread.
-------------
PR: https://git.openjdk.org/jfx/pull/1039
