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

[Jeanette Winzenburg]

On Sat, 29 Aug 2020 09:11:26 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> I haven't tested it, but it looks like it should work. I left a couple of minor suggestions below.
>> 
>> Would it be possible to add some tests to verify the behavior of HOME and END for editable and non-editable ComboBox
>> controls?
>
>> Would it be possible to add some tests to verify the behavior of HOME and END for editable and non-editable ComboBox
>> controls?
> 
> @kevinrushforth @kleopatra
> Please check the updated changes. _ComboBoxTest_ and _ListViewTest_ both have minor modifications in how they access
> _KeyMappings_. and added test for verifying **HOME** and **END** key with both editable and non editable ComboBox.

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?

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

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