RFR: 8090123: Items are no longer visible when collection is changed [v6]

[Kevin Rushforth]

On Thu, 23 Feb 2023 05:21:37 GMT, Karthik P K <kpk at 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