RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [v11]

[Jeanette Winzenburg]

On Mon, 7 Sep 2020 13:33:48 GMT, Ambarish Rapte <arapte at 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