On Fri, 3 Mar 2023 07:28:48 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: > > Stabilizing test The updated test seems stable now. I did leave a couple suggestions. Have you run the test this on all platforms? tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 62: > 60: * There is 1 test in this file. > 61: * Steps for testChoicBoxScrollOnCollectionChange() > 62: * 1. Create a ChoiceBox and add 50 items to it. Minor: the comment is wrong now (you add 150 items). tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 143: > 141: > 142: double rowHeight = > ContextMenuContentShim.getContextMenuRowHeight(popup); > 143: double screenHeight = > Screen.getPrimary().getBounds().getHeight(); I think using `getVisualBounds()` would be better. tests/system/src/test/java/test/robot/javafx/scene/ChoiceBoxScrollUpOnCollectionChangeTest.java line 144: > 142: double rowHeight = > ContextMenuContentShim.getContextMenuRowHeight(popup); > 143: double screenHeight = > Screen.getPrimary().getBounds().getHeight(); > 144: scrollChoiceBox((int) (screenHeight / rowHeight)); This seems to work, but it might be more robust to use `Math.ceil()` before casting to int, especially if you make the change to use the visual bounds. ------------- PR: https://git.openjdk.org/jfx/pull/1039
