On Wed, 19 Aug 2020 13:06:34 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> The issue occurs because the key events are consumed by the `ListView` in >> `Popup`, which displays the items. >> This is a regression of >> [JDK-8077916](https://bugs.openjdk.java.net/browse/JDK-8077916). This change >> aadded several >> `KeyMapping`s for focus traversals to `ListView`, which consume the `Left`, >> `Right` and `Ctrl+A` key events. >> Fix: >> 1. Remove the four focus traversal arrow `KeyMapping`s from >> `ListViewBehavior`. >> 2. Add the `Ctrl + A` `KeyMapping` to `ListViewBehavior` only if the >> `ListView`'s selection mode is set to >> `SelectionMode.MULTIPLE`. `ComboBox` uses the `ListView` with >> `SelectionMode.SINGLE` mode. >> Change unrelated to fix: >> `ComboBoxListViewBehavior` adds `KeyMapping` for `Up` and `Down` keys, which >> are not invoked when the `ComboBox` popup >> is showing. When the popup is shown, the Up and Down key events are handled >> by the `ListView` and the `KeyMapping` code >> from `ComboBoxListViewBehavior` does not get executed. These KeyMapping are >> removed. However this change is not needed >> for the fix. But this seems to be dead code. Verification: >> Added new unit tests to verify the change. >> Also verified that the behavior ListView behaves same before and after this >> change. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > change approach from removing to excluding fix and tests look okay (added minor inline comments), verified that the tests for the fix are failing before and passing after, those added for completeness are fine also. As noted in one of my comments (again? :), I don't like underscores .. not even in test methods - but as they are wide spread nothing to really complain about: just be consistent with yourself :) modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 181: > 180: > 181: @Test public void test_SwitchingSelectionModel() { > 182: ListView<String> listView = new ListView<>(); maybe the name could be improved by adding _what_ it's testing: something like testCtrlAWhenSwitchingSelectionModel? modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 1509: > 1508: > 1509: @Test public void test_switchingSelectionMode() { > 1510: ListView<String> listView = new ListView<>(); same as naming suggestion to the other Switching test above modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 1536: > 1535: assertEquals(4, sm.getSelectedItems().size()); > 1536: > 1537: sl.dispose(); wondering about these repeated blocks? The flow is single: (default on start) -> multiple -> single -> multiple. Looks like the last two are repeating the first two .. what do I overlook? modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2055: > 2054: .observableArrayList("Item1", "Item2")); > 2055: listView.setCellFactory(TextFieldListCell.forListView()); > 2056: StageLoader sl = new StageLoader(listView); hmm .. why the textFieldListCell? It's just a plain listCell if not in editing state .. or not? modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java line 1344: > 1343: > 1344: @Test public void test_EditorKeyInputsWhenPopupIsShowing() { > 1345: final ComboBox<String> cb = new > ComboBox<>(FXCollections.observableArrayList("a", "b", "c")); minor nit: naming is inconsistent to the other added test below (testExcludeKeyMappingsForComboBoxEditor) - either use an underscore or not (my personal preference is to not use them at all, following general java naming conventions also in tests .. but definitely not overly important :) ------------- Changes requested by fastegal (Committer). PR: https://git.openjdk.java.net/jfx/pull/172
