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

[Jeanette Winzenburg]

On Wed, 22 Apr 2020 06:43:31 GMT, Abhinay Agarwal <github.com+3197675+abhinayagarwal at openjdk.org> wrote:

> Don't you think that all the changes in `ListViewSkin` can be moved to `ListViewBehavior`? All that we do in the skin
> class is to call `ListViewBehavior#updateSelectionModeKeyMapping`, which smells like feature envy.
> Moreover, `ListViewBehavior` already has change listener attached to `selectionModelProperty`, waiting for us to re-use
> it ��

good point :) Though - I don't like listeners to control properties in behaviors, and much less listeners to path
properties (they tend to not getting cleaned on dispose).

In the particular case of behaviors of controls with selectionModels they do (must?) because the selectionModel is not
api complete (having no notion of anchor), so they jump in to cover up. Hopefully that design flaw will be fixed at
some time in future, which would remove the existing listener, anyway. With just another responsibility - difference
based on selectionMode - such cleanup would be harder.

Here the basic approach is to add/remove a keyMapping for multiple/single selection. Compared to current state, there's
a subtle side-effect (the event bubbles up if the mapping if removed). We can achieve the same behavior (with the same
side-effect) by making the mapping consume depending on whether it is handled (to select all) or not.

In code that would be pattern like:

    // in constructor

        KeyMapping selectAllMapping;
        addDefaultMapping(listViewInputMap,
              ...
             selectAll = new KeyMapping(new KeyBinding(A).shortcut(), this:: selectAll),
             ...
         };
         selectAllMapping.setAutoConsume(false);

    // selectAll with modified signature
    /**
      * Calls selectAll on selectionModel and consumes the event, if
      * the model is available and in multimode,
      * does nothing otherwise.
      */
    private void selectAll(KeyEvent key) {
        MultipleSelectionModel<T> sm = getNode().getSelectionModel();
       // do nothing, let it bubble up
        if (sm == null || sm.getSelectionMode() == SelectionMode.SINGLE) return;
       // handle and consume
        sm.selectAll();
        key.consume();
    }

BTW, there are other keys that don't work as expected (from the perspective of the editor in the combo): f.i.
shift-home/end is mapped to scrollToFirst/LastRow - that's hampering ux if f.i. the user has typed some input, uses
them and sees her input lost because first/last row is selected. Sry to  not have noticed earlier in my bug report :(

So whatever approach we choose (mappings being removed/added or their handlers not/consuming doesn't matter), we would
have to do it for several keys. Plus we have the side-effect mentioned above. The mass of change _for all listviews_
has a certain risk of breaking existing code. Think f.i. global accelerators that might (or not) get triggered
depending on selection mode.

On the other hand, different mappings are needed only when the list resides in the combo's popup (and probably only if
the combo is editable, didn't dig though). An alternative might be a different  inputMap (or containing different
mappings) when used in combo's popup (which is similar to what Swing/X does .. no wonder I would prefer it :)

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

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