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

[Jeanette Winzenburg]

On Tue, 4 Aug 2020 10:07:14 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> good that we agree on where to put it :)
>> 
>> Will have to think about about the remove installed mappings vs. change existing code to not installing. Right now,
>> feels a bit smelly .. but I see the constraints (and might agree :) Will come back!
>
> btw, using a key-value pair in properties is just fine with me (might have been unclear, sry) - nothing else to do
> short of changing api (which feels too heavy weight)
> One nit-pit for checking for its existence, an alternative slightly simplified pattern might be
> 
>     if (Boolean.TRUE.equals(control.getProperties().get(....)) {
>         removeMappings(...);
>     }

okay, I still favor the not-adding-if-not-needed approach - and it's not _that_ much of a change to existing code.
Basically, in the constructor of ListViewBehavior

- in a first block, do not add the mappings that (currently) are removed again if the  combo flag is set
-in a second block,  add the mappings for a stand-alone listView to all inputMaps as needed

code snippets:

        // not needed if in combo popup
        //  addDefaultMapping(listViewInputMap, FocusTraversalInputMap.getFocusTraversalMappings());
        addDefaultMapping(listViewInputMap,
                // not needed if in combo popup
                // new KeyMapping(HOME, e -> selectFirstRow()),
                ...
               // add those that are always needed
        }

        // same for child input maps
        addDefaultMappings(verticalInputMap, ...

        // add those for stand-alone listView
        // handle mappings that are needed only in a stand-alone listView
        if (!Boolean.TRUE.equals(control.getProperties().get("removeKeyMappingsForComboBoxEditor"))) {
            addDefaultMapping(listViewInputMap, FocusTraversalInputMap.getFocusTraversalMappings());
            addDefaultMapping(listViewInputMap,
                    new KeyMapping(HOME, e -> selectFirstRow()),
                    new KeyMapping(END, e -> selectLastRow()),
                    new KeyMapping(new KeyBinding(HOME).shift(), e -> selectAllToFirstRow()),
                    new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow()),
                    new KeyMapping(new KeyBinding(A).shortcut(), e -> selectAll()),
                    new KeyMapping(new KeyBinding(HOME).shortcut(), e -> focusFirstRow()),
                    new KeyMapping(new KeyBinding(END).shortcut(), e -> focusLastRow())
            );

            addDefaultMapping(verticalListInputMap,
                    new KeyMapping(new KeyBinding(HOME).shortcut().shift(), e -> discontinuousSelectAllToFirstRow()),
                    new KeyMapping(new KeyBinding(END).shortcut().shift(), e -> discontinuousSelectAllToLastRow())
            );
            
        }

Tests are passing as expected, and the [open issue JDK-8250807](https://bugs.openjdk.java.net/browse/JDK-8250807)
doesn't stand in the way :)

As to testing, independent of which approach is chosen at the end: I would suggest to not only test the macroscopic
behavior (look if key events on combo/listView have the expected effect) but also test the base layer, that is whether
or not the mappings are not/added to the inputMaps.
 
One open (maybe not-an) issue might be the handling of the horizontal map which currently is not touched (but probably
will, once the deep removal of [JDK-8250807](https://bugs.openjdk.java.net/browse/JDK-8250807) is fixed) : while not
very probable, client code might change the listView in the popup to be horizontal. What would be the expected UI in
that case? Anyway, could be left open for the scope of this issue, IMO.

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

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