On Wed, 22 Feb 2023 11:08:39 GMT, Karthik P K <[email protected]> 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
