On Mon, 7 Sep 2020 13:33:48 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: > > rename tests Fix looks fine and indeed pretty :) Verified tests failing before and after the fix. Left some minor comments inline. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java line 87: > 85: Predicate<KeyEvent> pIsInComboBox = e -> isListViewOfComboBox != > null; > 86: Predicate<KeyEvent> pIsInEditableComboBox = > 87: e -> isListViewOfComboBox != null && > isListViewOfComboBox.get(); nit-pick about naming: I think we don't use (crippled) hungarian notation, or do we? If we don't, the leading "p" should be removed, isIn/Editable/Combo is expressive enough (and no need to differentiate by type of functional interface, IMO) modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java line 509: > 507: getProperties().put("selectFirstRowByDefault", false); > 508: getProperties().put("editableComboBoxEditor", > (Supplier<Boolean>) () -> > getSkinnable().isEditable()); 509: } nit-pick about naming: it's the editable state of the combo (vs. the editable state of the editor) that's in the center of interest, so maybe rename the key to express that? Like "editableComboBox"? Another thingy (which I think is a bit under-used in the fx code-base;) - how about a code comment to why this marker is added and which collaborator is using it? modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java line 1347: > 1345: final ComboBox<String> cb = new > ComboBox<>(FXCollections.observableArrayList("a", "b", "c")); > 1346: cb.setEditable(true); > 1347: StageLoader sl = new StageLoader(cb); shouldn't there be an analogous functional test for not-editable combo? There are both functional and low-level for editable, but only low-level for not editable. modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java line 1514: > 1512: return > ((KeyMapping)mappings.get(i)).getInterceptor().test(null); > 1513: } > 1514: his throws an NPE without the fix - which makes the test using this method still fail (good!), but a bit unspecific for my taste. A slight re-organisation of the helpers might help: move the asserts down instead of returning a boolean. modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java line 2083: > 2081: private boolean testInterceptor(ObservableList<?> mappings, > KeyBinding binding) { > 2082: int i = mappings.indexOf(new KeyMapping(binding, null)); > 2083: return > ((KeyMapping)mappings.get(i)).getInterceptor().test(null); same as for ComboBoxTest ------------- Changes requested by fastegal (Committer). PR: https://git.openjdk.java.net/jfx/pull/172
