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

[Kevin Rushforth]

On Wed, 22 Feb 2023 11:08:39 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:
> 
>   Updating test

I added a few minor comments inline. I'll let others do the formal review.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ContextMenuContent.java line 346:

> 344:         itemsContainer.relocate(x, y);
> 345: 
> 346:         if(contentHeight < Math.abs(ty)){

Minor: add space after `if`

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ContextMenuContent.java line 822:

> 820:     }
> 821: 
> 822:     boolean isUpArrowVisible() {

If these two new methods are only added for testing (which it looks like they are), please add a comment to that effect.

tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 95:

> 93:     private void scrollChoiceBox(int scrollAmt) throws Exception {
> 94:         Util.runAndWait(() -> {
> 95:             for (int i=0; i<scrollAmt; i++) {

Minor: add spaces around `=` and `<`.

tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 102:

> 100:             try {
> 101:                 Thread.sleep(400); // Wait for up arrow to get loaded in UI
> 102:             } catch (Exception e) {}

Minor: If you use `Util.sleep` you don't need a try/catch.

tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 118:

> 116:         Util.runAndWait(() -> {
> 117:             ObservableList<String> items = FXCollections.observableArrayList();
> 118:             for(int i=0 ; i<count ; i++) {

Minor: add space after `for` and spaces around `=` and `<`.

-------------

PR: https://git.openjdk.org/jfx/pull/1039