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

[Ambarish Rapte]

On Tue, 1 Sep 2020 13:59:06 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> hmm .. this is getting unwieldy, isn't it ;)
>> 
>> The pain points:
>> - cascade of listeners (editable -> comboSkin -> properties -> behavior)
>> - dynamic change (add/remove) of mappings
>> - multiple key/value pairs for basically the same - though variant - state
>> 
>> My suggestion would be to take a step back (in solution path): near the beginning was the evaluation of using different
>> inputMaps for different state contexts. Which was not further evaluated because it looked like we could get away with
>> simply configuring the mappings - based on certain condition - once at instantiation time. Which has the advantage of
>> not touching too much code but unfortunely turned out to be not enough.   Meanwhile, I'm convinced that in the long run
>> there is no way around different inputMaps based on context: the differences in behavior (stand-alone vs. editable
>> combo-popup vs. not-editable combo-popup) are many - f.i. focus-only navigation doesn't make sense in the popup (should
>> be selection navigation always), left/right in a not-editable should trigger selection navigation .. and certainly
>> more. So we not only have to enable/disable certain mappings, but also re-map the triggered behavior.   That's too
>> broad for this issue, but we could take a step into that direction: use the InputMap/Mapping API to help - it was
>> designed for exactly such a differentiation :) The step would be to use interceptors (instead of dynamic modification
>> of the mappings list), they are available on both inputMap and mapping level. As a first step, we could use the latter:
>> keep the addition of mappings as-is (before the fix) and add interceptors to mappings for inclusion/exclusion based on
>> context. No listeners, no dynamic modification, just one marker in the properties .. hopefully :)  Raw code snippets:
>>     // let combo skin put a Supplier for editable as value
>>     getProperties().put("comboContext", (Supplier<Boolean>) () -> getSkinnable().isEditable());
>> 
>>     // let listView behavior use the supplier to build interceptors
>>     Supplier<Boolean> comboEditable = (Supplier<Boolean>) control.getProperties().get("comboContext");
>>     Predicate<KeyEvent> interceptIfInCombo = e -> comboEditable != null;
>>     Predicate<KeyEvent> interceptIfInEditableCombo = e -> comboEditable != null && comboEditable.get();
>>     
>>     if (comboEditable == null) {
>>         // add focus traversal mappings if not in combo popup
>>         addDefaultMapping(listViewInputMap, FocusTraversalInputMap.getFocusTraversalMappings());
>>     }
>>     // add mappings with appropriate interceptors
>>     addDefaultMapping(listViewInputMap,
>>         // missing api in KeyMapping: no constructor taking KeyCode and interceptor
>>         new KeyMapping(new KeyBinding(HOME), e -> selectFirstRow(), interceptIfInEditableCombo),
>>         new KeyMapping(new KeyBinding(END), e -> selectLastRow(), interceptIfInEditableCombo),
>>         new KeyMapping(new KeyBinding(HOME).shift(), e -> selectAllToFirstRow(), interceptIfInCombo),
>>         new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow(), interceptIfInCombo),
>>         ...
>> 
>> With this, the tests for key navigation are passing, the low-level mapping tests will have to be re-formulated to test
>> for not/intercepted vs. existence.
>> What do you think?
>
>> What do you think?
> 
> Suggestion looks promising, I shall try it and update.

@kleopatra  @kevinrushforth
Change looks pretty with interceptor.
Please take a look.

-------------

PR: https://git.openjdk.java.net/jfx/pull/172