Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 13 commits into from

Conversation

@arapte
Copy link
Member

@arapte arapte commented Apr 11, 2020

The issue occurs because the key events are consumed by the ListView in Popup, which displays the items.
This is a regression of JDK-8077916. This change aadded several KeyMappings for focus traversals to ListView, which consume the Left, Right and Ctrl+A key events.

Fix:

  1. Remove the four focus traversal arrow KeyMappings from ListViewBehavior.
  2. Add the Ctrl + A KeyMapping to ListViewBehavior only if the ListView's selection mode is set to SelectionMode.MULTIPLE. ComboBox uses the ListView with SelectionMode.SINGLE mode.

Change unrelated to fix:
ComboBoxListViewBehavior adds KeyMapping for Up and Down keys, which are not invoked when the ComboBox popup is showing. When the popup is shown, the Up and Down key events are handled by the ListView and the KeyMapping code from ComboBoxListViewBehavior does not get executed. These KeyMapping are removed.
However this change is not needed for the fix. But this seems to be dead code.

Verification:
Added new unit tests to verify the change.
Also verified that the behavior ListView behaves same before and after this change.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/172/head:pull/172
$ git checkout pull/172

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 11, 2020

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Apr 11, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 14, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth self-requested a review Apr 21, 2020
@abhinayagarwal
Copy link
Contributor

@abhinayagarwal abhinayagarwal commented Apr 22, 2020

Hi @arapte,

Great work on the PR 👍

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 😉

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 22, 2020

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 :)

@arapte
Copy link
Member Author

@arapte arapte commented Apr 22, 2020

ListViewBehavior already has change listener attached to selectionModelProperty, waiting for us to re-use it

Hi @abhinayagarwal, Thanks for the suggestion. This sound good to me too. I shall make this change in next commit.

@abhinayagarwal
Copy link
Contributor

@abhinayagarwal abhinayagarwal commented Apr 23, 2020

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.

Changing the behavior inside selectAll is also a good option (and much cleaner as well)

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.

It's sad that we can't easily override (InputMapping created in) ListViewBehavior :(

@arapte
Copy link
Member Author

@arapte arapte commented Apr 29, 2020

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 :)

Thanks for the suggestion 👍 , I shall try this approach and update the PR. I am not sure if we already do this for any other control. Do you know any, if we do ? Not actively working on this issue, Will soon get back on this :)

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 29, 2020

the nearest to different input maps based on control state might be in listViewBehavior itself: it has differrent child maps for vertical/horizontal orientation. Could be possible to widen that a bit with another child map for vertical and in combo popup (provided it has a means to decide being in such a state for the sake of an interceptor, without api change that might be a simple entry in its properties)

@openjdk
Copy link

@openjdk openjdk bot commented Jun 17, 2020

@arapte this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout ComboBox_Editor
git fetch https://git.openjdk.java.net/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@openjdk openjdk bot added the merge-conflict label Jun 17, 2020
@arapte arapte changed the title 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [WIP}8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing Jul 21, 2020
@arapte arapte changed the title [WIP}8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [WIP]8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing Jul 21, 2020
@openjdk openjdk bot removed the rfr label Jul 21, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Once the merge conflicts and review comments are addressed, I'll put this back on my review queue.

@openjdk openjdk bot removed the merge-conflict label Jul 28, 2020
arapte added 2 commits Jul 30, 2020
@arapte arapte changed the title [WIP]8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing Jul 30, 2020
@openjdk openjdk bot added the rfr label Jul 30, 2020
@arapte
Copy link
Member Author

@arapte arapte commented Jul 30, 2020

Please review the updated change:

  1. Changed the approach to add a property named removeKeyMappingsForComboBoxEditor to ListView when creating it from ComboBox. Please do suggest if this name sounds Ok.
  2. When this property is present some KeyMappings that are needed for ComboBox Editor get removed from ListViewBehavior.
  3. I have considered to remove some more KeyMappings which seem like they should be passed on to ComboBox's Editor.(method removeKeyMappingsForComboBoxEditor())
  4. There is an existing issue with BehaviorBase: JDK-8250807.
    Due to this issue, KeyMappings from child InputMap do not get removed. So the keys CTRL+SHIFT+HOME and CTRL+SHIFT+END will still not work with ComboBox editor.
  5. Updated ComboBoxTest for additional keys.
  6. Keeping the tests added for ListView, as they seem reasonable and not present already.

Change is very specific to ComboBox editor, so it should not affect any other tests.

@kevinrushforth kevinrushforth self-requested a review Jul 30, 2020
Copy link
Collaborator

@kleopatra kleopatra left a comment

fix and tests look okay (added minor inline comments), verified that the tests for the fix are failing before and passing after, those added for completeness are fine also.

As noted in one of my comments (again? :), I don't like underscores .. not even in test methods - but as they are wide spread nothing to really complain about: just be consistent with yourself :)

Copy link
Collaborator

@kleopatra kleopatra left a comment

looks good :)

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 27, 2020

The fix looks good for an editable ComboBox.

If the ComboBox is not editable, it will have the effect of making the HOME and END keys no-ops, which is a (possibly unwanted) change in behavior. I checked a couple native Windows apps and they have the behavior I would expect: the arrow keys, and the HOME / END keys navigate the text field for editable combo boxes. HOME and END go to the beginning or end of the list for non-editable combo boxes.

While we could treat that as a follow-up issue, it would be worth thinking about whether we could limit the change to editable combo boxes.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Aug 28, 2020

If the ComboBox is not editable, it will have the effect of making the HOME and END keys no-ops, which is a (possibly unwanted) change in behavior. I checked a couple native Windows apps and they have the behavior I would expect: the arrow keys, and the HOME / END keys navigate the text field for editable combo boxes. HOME and END go to the beginning or end of the list for non-editable combo boxes.

While we could treat that as a follow-up issue, it would be worth thinking about whether we could limit the change to editable combo boxes.

outch .. how did I overlook that .. (seems reviewing doesn't belong to my strengths ;)

This fix should not break (correct) existing behavior, so back to thinking ..

@arapte
Copy link
Member Author

@arapte arapte commented Aug 28, 2020

If the ComboBox is not editable, it will have the effect of making the HOME and END keys no-ops, which is a (possibly unwanted) change in behavior.

I have updated PR with changes for non editable ComboBox.
I could think of adding another property to propagate the editable property of ComboBox to ListViewBehavior. So now this fix adds another property editableComboBoxEditor, not sure if there is other way to handle it.
The change adds HOME and END KeyMappings when ComboBox is non editable and removes them when ComboBox is editable.
If the change sounds Ok, I shall include test in next commit.

Also, there is one change in the if condition that was suggested by Jeanette before,
if (!Boolean.TRUE.equals(control.getProperties().containsKey("excludeKeyMappingsForComboBoxEditor")))
is changed to,
if (Boolean.FALSE.equals(control.getProperties().containsKey("excludeKeyMappingsForComboBoxEditor")))

It seems safe as control.getProperties().containsKey() returns either true or false.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

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?

@arapte
Copy link
Member Author

@arapte arapte commented Aug 29, 2020

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.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Aug 29, 2020

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?

@arapte
Copy link
Member Author

@arapte arapte commented Sep 1, 2020

What do you think?

Suggestion looks promising, I shall try it and update.

arapte added 2 commits Sep 7, 2020
@arapte
Copy link
Member Author

@arapte arapte commented Sep 7, 2020

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

Copy link
Collaborator

@kleopatra kleopatra left a comment

Fix looks fine and indeed pretty :) Verified tests failing before and passing after the fix.

Left some minor comments inline.

@arapte
Copy link
Member Author

@arapte arapte commented Sep 11, 2020

Left some minor comments inline.

Updated PR with corrections, please take a look.

Copy link
Collaborator

@kleopatra kleopatra left a comment

looks good to me now :)

@openjdk
Copy link

@openjdk openjdk bot commented Oct 1, 2020

@arapte This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: kcr, fastegal

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 35 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 1, 2020
@arapte
Copy link
Member Author

@arapte arapte commented Oct 2, 2020

/integrate

@openjdk openjdk bot closed this Oct 2, 2020
@openjdk openjdk bot added integrated and removed ready labels Oct 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 2, 2020

@arapte Since your change was applied there have been 35 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 77a183e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk openjdk bot removed the rfr label Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants