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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c2beb0d
8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if …
arapte Apr 11, 2020
f0c94d6
Merge with master
arapte Jul 28, 2020
e9f7d40
changes to address to review
arapte Jul 30, 2020
7f52824
revert unintended line deletion
arapte Jul 30, 2020
f81ed02
make removeKeyMappingsForComboBoxEditor() private
arapte Aug 4, 2020
0353814
change property existence check
arapte Aug 6, 2020
f58ee4a
change approach from removing to excluding
arapte Aug 19, 2020
22f79d6
Corrections in test
arapte Aug 26, 2020
2539a5a
Fix for non editable ComboBox
arapte Aug 28, 2020
b948ef1
add test, modify ifs
arapte Aug 29, 2020
de91bed
Change approach to use Interceptor
arapte Sep 7, 2020
0755797
rename tests
arapte Sep 7, 2020
697e439
Review: update
arapte Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...les/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/BehaviorBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ protected void addDefaultMapping(InputMap<N> inputMap, Mapping<?>... newMapping)

for (Mapping<?> mapping : newMapping) {
// check if a mapping already exists, and if so, do not add this mapping
// TODO this is insufficient as we need to check entire InputMap hierarchy
// TODO: JDK-8250807: this is insufficient as we need to check entire InputMap hierarchy
// for (Mapping<?> existingMapping : existingMappings) {
// if (existingMapping != null && existingMapping.equals(mapping)) {
// return;
Expand Down Expand Up @@ -116,6 +116,7 @@ protected InputMap<N> createInputMap() {
}

protected void removeMapping(Object key) {
// TODO: JDK-8250807: Traverse the child maps of getInputMap() and remove the mapping from them.
InputMap<?> inputMap = getInputMap();
inputMap.lookupMapping(key).ifPresent(mapping -> {
inputMap.getMappings().remove(mapping);
Expand Down
46 changes: 0 additions & 46 deletions ...ontrols/src/main/java/com/sun/javafx/scene/control/behavior/ComboBoxListViewBehavior.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,55 +26,9 @@
package com.sun.javafx.scene.control.behavior;

import javafx.scene.control.ComboBox;
import javafx.scene.control.ComboBoxBase;
import javafx.scene.control.SelectionModel;
import com.sun.javafx.scene.control.inputmap.InputMap;

import static javafx.scene.input.KeyCode.DOWN;
import static javafx.scene.input.KeyCode.UP;

public class ComboBoxListViewBehavior<T> extends ComboBoxBaseBehavior<T> {

/***************************************************************************
* *
* Constructors *
* *
**************************************************************************/

/**
*
*/
public ComboBoxListViewBehavior(final ComboBox<T> comboBox) {
super(comboBox);

// Add these bindings as a child input map, so they take precedence
InputMap<ComboBoxBase<T>> comboBoxListViewInputMap = new InputMap<>(comboBox);
comboBoxListViewInputMap.getMappings().addAll(
new InputMap.KeyMapping(UP, e -> selectPrevious()),
new InputMap.KeyMapping(DOWN, e -> selectNext())
);
addDefaultChildMap(getInputMap(), comboBoxListViewInputMap);
}

/***************************************************************************
* *
* Key event handling *
* *
**************************************************************************/

private ComboBox<T> getComboBox() {
return (ComboBox<T>) getNode();
}

private void selectPrevious() {
SelectionModel<T> sm = getComboBox().getSelectionModel();
if (sm == null) return;
sm.selectPrevious();
}

private void selectNext() {
SelectionModel<T> sm = getComboBox().getSelectionModel();
if (sm == null) return;
sm.selectNext();
}
}
31 changes: 19 additions & 12 deletions ...javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@

import java.util.ArrayList;
import java.util.List;
import java.util.function.Predicate;
import java.util.function.Supplier;

import static com.sun.javafx.scene.control.inputmap.InputMap.*;
import static javafx.scene.input.KeyCode.*;
Expand Down Expand Up @@ -78,12 +80,19 @@ public ListViewBehavior(ListView<T> control) {
listViewInputMap = createInputMap();

// add focus traversal mappings
addDefaultMapping(listViewInputMap, FocusTraversalInputMap.getFocusTraversalMappings());
Supplier<Boolean> isListViewOfComboBox =
(Supplier<Boolean>) control.getProperties().get("editableComboBoxEditor");
Predicate<KeyEvent> pIsInComboBox = e -> isListViewOfComboBox != null;
Predicate<KeyEvent> pIsInEditableComboBox =
e -> isListViewOfComboBox != null && isListViewOfComboBox.get();
arapte marked this conversation as resolved.
Show resolved Hide resolved
if (isListViewOfComboBox == null) {
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(HOME), e -> selectFirstRow(), pIsInEditableComboBox),
new KeyMapping(new KeyBinding(END), e -> selectLastRow(), pIsInEditableComboBox),
new KeyMapping(new KeyBinding(HOME).shift(), e -> selectAllToFirstRow(), pIsInComboBox),
new KeyMapping(new KeyBinding(END).shift(), e -> selectAllToLastRow(), pIsInComboBox),
new KeyMapping(new KeyBinding(PAGE_UP).shift(), e -> selectAllPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shift(), e -> selectAllPageDown()),

Expand All @@ -98,9 +107,9 @@ public ListViewBehavior(ListView<T> control) {
new KeyMapping(F2, e -> activate()),
new KeyMapping(ESCAPE, e -> cancelEdit()),

new KeyMapping(new KeyBinding(A).shortcut(), e -> selectAll()),
new KeyMapping(new KeyBinding(HOME).shortcut(), e -> focusFirstRow()),
new KeyMapping(new KeyBinding(END).shortcut(), e -> focusLastRow()),
new KeyMapping(new KeyBinding(A).shortcut(), e -> selectAll(), pIsInComboBox),
new KeyMapping(new KeyBinding(HOME).shortcut(), e -> focusFirstRow(), pIsInComboBox),
new KeyMapping(new KeyBinding(END).shortcut(), e -> focusLastRow(), pIsInComboBox),
new KeyMapping(new KeyBinding(PAGE_UP).shortcut(), e -> focusPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shortcut(), e -> focusPageDown()),

Expand Down Expand Up @@ -145,10 +154,9 @@ public ListViewBehavior(ListView<T> control) {
new KeyMapping(new KeyBinding(DOWN).shortcut().shift(), e -> discontinuousSelectNextRow()),
new KeyMapping(new KeyBinding(PAGE_UP).shortcut().shift(), e -> discontinuousSelectPageUp()),
new KeyMapping(new KeyBinding(PAGE_DOWN).shortcut().shift(), e -> discontinuousSelectPageDown()),
new KeyMapping(new KeyBinding(HOME).shortcut().shift(), e -> discontinuousSelectAllToFirstRow()),
new KeyMapping(new KeyBinding(END).shortcut().shift(), e -> discontinuousSelectAllToLastRow())
new KeyMapping(new KeyBinding(HOME).shortcut().shift(), e -> discontinuousSelectAllToFirstRow(), pIsInComboBox),
new KeyMapping(new KeyBinding(END).shortcut().shift(), e -> discontinuousSelectAllToLastRow(), pIsInComboBox)
);

addDefaultChildMap(listViewInputMap, verticalListInputMap);

// --- horizontal listview
Expand Down Expand Up @@ -198,7 +206,6 @@ public ListViewBehavior(ListView<T> control) {
}



/***************************************************************************
* *
* Implementation of BehaviorBase API *
Expand Down
2 changes: 2 additions & 0 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.sun.javafx.scene.control.behavior.ComboBoxListViewBehavior;

import java.util.List;
import java.util.function.Supplier;

import javafx.beans.InvalidationListener;
import javafx.beans.WeakInvalidationListener;
Expand Down Expand Up @@ -504,6 +505,7 @@ private ListView<T> createListView() {

{
getProperties().put("selectFirstRowByDefault", false);
getProperties().put("editableComboBoxEditor", (Supplier<Boolean>) () -> getSkinnable().isEditable());
}
arapte marked this conversation as resolved.
Show resolved Hide resolved

@Override protected double computeMinHeight(double width) {
Expand Down
180 changes: 179 additions & 1 deletion modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,16 @@

package test.javafx.scene.control;

import com.sun.javafx.scene.control.behavior.FocusTraversalInputMap;
import com.sun.javafx.scene.control.behavior.ListViewBehavior;
import com.sun.javafx.scene.control.inputmap.InputMap;
import com.sun.javafx.scene.control.inputmap.InputMap.KeyMapping;
import com.sun.javafx.scene.control.inputmap.KeyBinding;
import com.sun.javafx.tk.Toolkit;

import test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory;
import test.com.sun.javafx.scene.control.infrastructure.KeyModifier;
import test.com.sun.javafx.scene.control.infrastructure.MouseEventFirer;
import com.sun.javafx.tk.Toolkit;
import javafx.css.PseudoClass;

import test.com.sun.javafx.scene.control.infrastructure.KeyEventFirer;
Expand Down Expand Up @@ -1334,6 +1341,177 @@ public void defaultConverterCanHandleIncorrectType_2() {
sl.dispose();
}

@Test public void testEditorKeyInputsWhenPopupIsShowing() {
final ComboBox<String> cb = new ComboBox<>(FXCollections.observableArrayList("a", "b", "c"));
cb.setEditable(true);
StageLoader sl = new StageLoader(cb);
arapte marked this conversation as resolved.
Show resolved Hide resolved
KeyEventFirer keyboard = new KeyEventFirer(cb);

// Show the popup
assertFalse(cb.isShowing());
cb.requestFocus();
cb.getEditor().setText("ABC DEF");
assertEquals("ABC DEF", cb.getEditor().getText());
keyboard.doDownArrowPress(KeyModifier.ALT);
// Sanity
assertTrue(cb.isShowing());
assertEquals(0, cb.getEditor().getCaretPosition());

// LEFT, RIGHT keys with CTRL, SHIFT modifiers
// Test RIGHT key
keyboard.doRightArrowPress();
assertEquals(1, cb.getEditor().getCaretPosition());

// Test KP_RIGHT key
keyboard.doKeyPress(KeyCode.KP_RIGHT);
assertEquals(2, cb.getEditor().getCaretPosition());

// Test LEFT key
keyboard.doLeftArrowPress();
assertEquals(1, cb.getEditor().getCaretPosition());

// Test KP_LEFT key
keyboard.doKeyPress(KeyCode.KP_LEFT);
assertEquals(0, cb.getEditor().getCaretPosition());

// Test SHIFT + RIGHT key
keyboard.doKeyPress(KeyCode.RIGHT, KeyModifier.SHIFT);
assertEquals("A", cb.getEditor().getSelectedText());
assertEquals(1, cb.getEditor().getCaretPosition());

// Test SHIFT + LEFT key
keyboard.doKeyPress(KeyCode.LEFT, KeyModifier.SHIFT);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(0, cb.getEditor().getCaretPosition());

// Test CTRL + RIGHT key
keyboard.doKeyPress(KeyCode.RIGHT, KeyModifier.CTRL);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(4, cb.getEditor().getCaretPosition());

// Test CTRL + LEFT key
keyboard.doKeyPress(KeyCode.LEFT, KeyModifier.CTRL);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(0, cb.getEditor().getCaretPosition());

// Test CTRL + SHIFT + RIGHT key
keyboard.doKeyPress(KeyCode.RIGHT, KeyModifier.CTRL, KeyModifier.SHIFT);
assertEquals("ABC ", cb.getEditor().getSelectedText());
assertEquals(4, cb.getEditor().getCaretPosition());

// Test CTRL + SHIFT + LEFT key
keyboard.doKeyPress(KeyCode.LEFT, KeyModifier.CTRL, KeyModifier.SHIFT);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(0, cb.getEditor().getCaretPosition());

// HOME, END keys with CTRL, SHIFT modifiers
// Test END key
keyboard.doKeyPress(KeyCode.END);
assertEquals(7, cb.getEditor().getCaretPosition());

// Test HOME key
keyboard.doKeyPress(KeyCode.HOME);
assertEquals(0, cb.getEditor().getCaretPosition());

// Test SHIFT + END key
keyboard.doKeyPress(KeyCode.END, KeyModifier.SHIFT);
assertEquals(cb.getEditor().getText(), cb.getEditor().getSelectedText());
assertEquals(7, cb.getEditor().getCaretPosition());

// Test SHIFT + HOME key
keyboard.doKeyPress(KeyCode.HOME, KeyModifier.SHIFT);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(0, cb.getEditor().getCaretPosition());

// Test CTRL + END key
keyboard.doKeyPress(KeyCode.END, KeyModifier.CTRL);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(7, cb.getEditor().getCaretPosition());

// Test CTRL + HOME key
keyboard.doKeyPress(KeyCode.HOME, KeyModifier.CTRL);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(0, cb.getEditor().getCaretPosition());

// Test CTRL + SHIFT + END key
keyboard.doKeyPress(KeyCode.END, KeyModifier.CTRL, KeyModifier.SHIFT);
assertEquals(cb.getEditor().getText(), cb.getEditor().getSelectedText());
assertEquals(7, cb.getEditor().getCaretPosition());

// Test CTRL + SHIFT + HOME key
keyboard.doKeyPress(KeyCode.HOME, KeyModifier.CTRL, KeyModifier.SHIFT);
assertEquals("", cb.getEditor().getSelectedText());
assertEquals(0, cb.getEditor().getCaretPosition());

// Test CTRL + A key
keyboard.doLeftArrowPress();
assertEquals("", cb.getEditor().getSelectedText());
keyboard.doKeyPress(KeyCode.A, KeyModifier.getShortcutKey());
assertEquals(cb.getEditor().getText(), cb.getEditor().getSelectedText());

// Sanity
assertTrue(cb.isShowing());

sl.dispose();
}

@Test public void testInterceptedKeyMappingsForComboBoxEditor() {
final ComboBox<String> cb = new ComboBox<>(FXCollections.observableArrayList("a", "b", "c"));
StageLoader sl = new StageLoader(cb);

ListView listView = (ListView) ((ComboBoxListViewSkin)cb.getSkin()).getPopupContent();
ListViewBehavior lvBehavior = (ListViewBehavior)ControlSkinFactory.getBehavior(listView.getSkin());
InputMap<ListView<?>> lvInputMap = lvBehavior.getInputMap();
ObservableList<?> inputMappings = lvInputMap.getMappings();
// In ListViewBehavior KeyMappings for vertical orientation are added under 3rd child InputMap
InputMap<ListView<?>> verticalInputMap = lvInputMap.getChildInputMaps().get(2);
ObservableList<?> verticalInputMappings = verticalInputMap.getMappings();

cb.setEditable(true);
testKeyMappingsForEditableCB(inputMappings);
testCommonKeyMappings(inputMappings, verticalInputMappings);

cb.setEditable(false);
testKeyMappingsForNonEditableCB(inputMappings);
testCommonKeyMappings(inputMappings, verticalInputMappings);

sl.dispose();
}

private void testKeyMappingsForEditableCB(ObservableList<?> inputMappings) {
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.HOME)));
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.END)));
}

private void testKeyMappingsForNonEditableCB(ObservableList<?> inputMappings) {
assertFalse(testInterceptor(inputMappings, new KeyBinding(KeyCode.HOME)));
assertFalse(testInterceptor(inputMappings, new KeyBinding(KeyCode.END)));
}

private void testCommonKeyMappings(ObservableList<?> inputMappings,
ObservableList<?> verticalInputMappings) {
// Verify FocusTraversalInputMap
for(InputMap.Mapping<?> mapping : FocusTraversalInputMap.getFocusTraversalMappings()) {
assertFalse(inputMappings.contains(mapping));
}

// Verify default InputMap
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.HOME).shift()));
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.END).shift()));
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.HOME).shortcut()));
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.END).shortcut()));
assertTrue(testInterceptor(inputMappings, new KeyBinding(KeyCode.A).shortcut()));

// Verify vertical child InputMap
assertTrue(testInterceptor(verticalInputMappings, new KeyBinding(KeyCode.HOME).shortcut().shift()));
assertTrue(testInterceptor(verticalInputMappings, new KeyBinding(KeyCode.END).shortcut().shift()));
}

private boolean testInterceptor(ObservableList<?> mappings, KeyBinding binding) {
int i = mappings.indexOf(new KeyMapping(binding, null));
return ((KeyMapping)mappings.get(i)).getInterceptor().test(null);
}

arapte marked this conversation as resolved.
Show resolved Hide resolved
@Test public void test_rt36280_nonEditable_enterHidesShowingPopup() {
final ComboBox<String> cb = new ComboBox<>(FXCollections.observableArrayList("a", "b", "c"));
StageLoader sl = new StageLoader(cb);
Expand Down
Loading