[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [v8]
From:       Jeanette Winzenburg <fastegal () openjdk ! java ! net>
Date:       2020-08-29 10:42:54
Message-ID: YzW8gHSEKus7QH5V8TjTSdq5PBspkRHUEJ4Uj2eA75s=.01ef1c5a-0010-46f3-812a-4700c8fb749c () github ! com
[Download RAW message or body]

On Sat, 29 Aug 2020 09:11:26 GMT, Ambarish Rapte <arapte@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


[prev in list] [next in list] [prev in thread] [next in thread]