-
Bug
-
Resolution: Unresolved
-
P3
-
10
At the end is a test case (the fixtures can be added to core fx TextFieldTest more or less without change) that installs a textFormatter with a filter which tracks notifications, for this bug it simply counts its invocations. The fixtures invoke navigation methods backward/forward and some others for comparison. They test that for each navigation method, the filter is called exactly once. That's the case for all except forward/backward.
The culprit seems to be the forward/backward methods themselves:
public void forward() {
// user has moved caret to the right
final int textLength = getLength();
final int dot = getCaretPosition();
final int mark = getAnchor();
if (dot != mark) {
int pos = Math.max(dot, mark);
selectRange(pos, pos);
} else if (dot < textLength && textLength > 0) {
if (charIterator == null) {
charIterator = BreakIterator.getCharacterInstance();
}
charIterator.setText(getText());
int pos = charIterator.following(dot);
selectRange(pos, pos);
}
// following line is basically calling selectRange(pos, pos) again
deselect();
}
don't know when the last line might be needed - but suspect that if there use-cases where it is needed it should be called only if the range has not been set in the if/else blocks, so probably having to change to logic to
if (dot != mark) {
doStuff();
} else if (not-at-end-of-text) {
soOtherStuff();
} else { // at end of text, must deselect
deselect();
}
A quick workaround for application code is to subclass TextField and override selectRange(...) to do nothing if anchor/caretPosition are the same as the newly requested. Doing so will also workaround JDK-8210297.
public class XTextField extends TextField {
public XTextField() {
super();
}
/**
* @param text
*/
public XTextField(String text) {
super(text);
}
/**
* Overridden to return if anchor and caret are already at the given
* positions.
*
* Removes both double notification on backward/forward and
* notification on unrelated focusOwner changes.
*/
@Override
public void selectRange(int anchor, int caretPosition) {
if (getAnchor() == anchor && getCaretPosition() == caretPosition) return;
super.selectRange(anchor, caretPosition);
}
}
This class passes all tests of core TextInputControlTest, TextFieldTest and the fixtures below.
@RunWith(JUnit4.class)
public class TextFieldTest {
// replace with core variant to kick off the fx thread
@ClassRule
public static TestRule classRule = new JavaFXThreadingRule();
protected TextField txtField;//Empty string
protected TextField dummyTxtField;//With string value
@Before
public void setup() {
txtField = createTextField();
dummyTxtField = createTextField("dummy");
}
// to test XTextField, subclass and override these factory methods
// to return XTextField
protected TextField createTextField() {
return new TextField();
}
protected TextField createTextField(String text) {
return new TextField(text);
}
// TextFormatter notification on focus traversal
/**
* Test notification behavior on focus traversal.
* https://bugs.openjdk.java.net/browse/JDK-8210297
*/
@Test
public void testFormatterNotificationOnTab() {
TextField first = createTextField("first");
TextField second = createTextField("second");
TextField third = createTextField("third");
List<String> notifications = new ArrayList<>();
VBox box = new VBox(10, first, second, third);
Stage stage = new StageLoader(box).getStage();
first.requestFocus();
assertEquals(first, stage.getScene().getFocusOwner());
box.getChildren().forEach(node -> {
TextField field = (TextField) node;
UnaryOperator<TextFormatter.Change> filter = c -> {
notifications.add(c.getControlText());
return c;
};
field.setTextFormatter(new TextFormatter<>(filter));
});
second.requestFocus();
assertEquals("sanity: seond is focuOwner", second, stage.getScene().getFocusOwner());
assertEquals(notifications.toString(), 2, notifications.size());
}
// Textformatter notification on navigation.
/**
* Test notification on navigation: backward.
* this bug: fails
*/
@Test
public void testFormatterNotificationOnBackward() {
int pos = 3;
assertFormatterNotificationOnNavigation(
t -> t.selectRange(pos, pos),
t -> t.backward());
}
/**
* Test notification on navigation: forward.
* this bug: fails
*/
@Test
public void testFormatterNotificationOnForward() {
assertFormatterNotificationOnNavigation(t -> t.forward());
}
/**
* Test notification on navigation: enOffNextWord.
* for comparison - passes
*/
@Test
public void testFormatterNotificationOnForwardWord() {
assertFormatterNotificationOnNavigation(t -> t.endOfNextWord());
}
/**
* Test notification on navigation: home.
* for comparison - passes
*/
@Test
public void testFormatterNotificationOnHome() {
int pos = 3;
assertFormatterNotificationOnNavigation(
t -> t.selectRange(pos, pos),
t -> t.home());
}
/**
* Test that navigation produces a single notification of Textformatter.
*
* @param preFormatter the initial state transition before
* setting the formatter, may be null if initial state is default
* @param navigation the navigation
*/
protected void assertFormatterNotificationOnNavigation(
Consumer<TextField> preFormatter,
Consumer<TextField> navigation) {
assertFormatterNotificationOnNavigation(preFormatter, navigation, null, null);
}
/**
* Test that navigation produces a single notification of Textformatter.
*
* @param navigation the navigation
*/
protected void assertFormatterNotificationOnNavigation(Consumer<TextField> navigation) {
assertFormatterNotificationOnNavigation(null, navigation, null, null);
}
/**
* Test that navigation produces a single notification of textFormatter.
* A textField to navigate on is created with text "The quick brown fox"
* and a TextFormatter with a filter implemented to count the notifications.
*
* @param preFormatter the initial state transition before
* setting the formatter, may be null if initial state is default
* @param navigation the navigation
* @param anchorProvider the expected anchor after navigation - sanity check, may be null.
* @param caretProvider the expected caret after navigation - sanity check, may be null.
*/
protected void assertFormatterNotificationOnNavigation(
Consumer<TextField> preFormatter,
Consumer <TextField> navigation,
Function<TextField, Integer> anchorProvider,
Function<TextField, Integer> caretProvider) {
// textfield with text "The quick brown fox"
TextField txtField = createTextField("The quick brown fox");
// initial state - do before setting the formatter to not include in counting
if (preFormatter != null) {
preFormatter.accept(txtField);
}
IntegerProperty count = new SimpleIntegerProperty(0);
UnaryOperator<TextFormatter.Change> filter = c -> {
count.set(count.get() + 1);
return c;
};
TextFormatter<String> formatter = new TextFormatter<>(filter);
txtField.setTextFormatter(formatter);
// navigate
navigation.accept(txtField);
// sanity tests of anchor/caret pos
// here we are not really interested in those (they are tested extensively
// in TextInputControlTest) - just for digging if things go wrong unexpectedly
if (anchorProvider != null) {
int anchor = anchorProvider.apply(txtField);
assertEquals("sanity: anchor at end", anchor, txtField.getAnchor());
}
if (caretProvider != null) {
int caret = caretProvider.apply(txtField);
assertEquals("sanity: caret at end", caret, txtField.getCaretPosition());
}
assertEquals("must have received a single notification", 1, count.get());
}
}
The culprit seems to be the forward/backward methods themselves:
public void forward() {
// user has moved caret to the right
final int textLength = getLength();
final int dot = getCaretPosition();
final int mark = getAnchor();
if (dot != mark) {
int pos = Math.max(dot, mark);
selectRange(pos, pos);
} else if (dot < textLength && textLength > 0) {
if (charIterator == null) {
charIterator = BreakIterator.getCharacterInstance();
}
charIterator.setText(getText());
int pos = charIterator.following(dot);
selectRange(pos, pos);
}
// following line is basically calling selectRange(pos, pos) again
deselect();
}
don't know when the last line might be needed - but suspect that if there use-cases where it is needed it should be called only if the range has not been set in the if/else blocks, so probably having to change to logic to
if (dot != mark) {
doStuff();
} else if (not-at-end-of-text) {
soOtherStuff();
} else { // at end of text, must deselect
deselect();
}
A quick workaround for application code is to subclass TextField and override selectRange(...) to do nothing if anchor/caretPosition are the same as the newly requested. Doing so will also workaround JDK-8210297.
public class XTextField extends TextField {
public XTextField() {
super();
}
/**
* @param text
*/
public XTextField(String text) {
super(text);
}
/**
* Overridden to return if anchor and caret are already at the given
* positions.
*
* Removes both double notification on backward/forward and
* notification on unrelated focusOwner changes.
*/
@Override
public void selectRange(int anchor, int caretPosition) {
if (getAnchor() == anchor && getCaretPosition() == caretPosition) return;
super.selectRange(anchor, caretPosition);
}
}
This class passes all tests of core TextInputControlTest, TextFieldTest and the fixtures below.
@RunWith(JUnit4.class)
public class TextFieldTest {
// replace with core variant to kick off the fx thread
@ClassRule
public static TestRule classRule = new JavaFXThreadingRule();
protected TextField txtField;//Empty string
protected TextField dummyTxtField;//With string value
@Before
public void setup() {
txtField = createTextField();
dummyTxtField = createTextField("dummy");
}
// to test XTextField, subclass and override these factory methods
// to return XTextField
protected TextField createTextField() {
return new TextField();
}
protected TextField createTextField(String text) {
return new TextField(text);
}
// TextFormatter notification on focus traversal
/**
* Test notification behavior on focus traversal.
* https://bugs.openjdk.java.net/browse/JDK-8210297
*/
@Test
public void testFormatterNotificationOnTab() {
TextField first = createTextField("first");
TextField second = createTextField("second");
TextField third = createTextField("third");
List<String> notifications = new ArrayList<>();
VBox box = new VBox(10, first, second, third);
Stage stage = new StageLoader(box).getStage();
first.requestFocus();
assertEquals(first, stage.getScene().getFocusOwner());
box.getChildren().forEach(node -> {
TextField field = (TextField) node;
UnaryOperator<TextFormatter.Change> filter = c -> {
notifications.add(c.getControlText());
return c;
};
field.setTextFormatter(new TextFormatter<>(filter));
});
second.requestFocus();
assertEquals("sanity: seond is focuOwner", second, stage.getScene().getFocusOwner());
assertEquals(notifications.toString(), 2, notifications.size());
}
// Textformatter notification on navigation.
/**
* Test notification on navigation: backward.
* this bug: fails
*/
@Test
public void testFormatterNotificationOnBackward() {
int pos = 3;
assertFormatterNotificationOnNavigation(
t -> t.selectRange(pos, pos),
t -> t.backward());
}
/**
* Test notification on navigation: forward.
* this bug: fails
*/
@Test
public void testFormatterNotificationOnForward() {
assertFormatterNotificationOnNavigation(t -> t.forward());
}
/**
* Test notification on navigation: enOffNextWord.
* for comparison - passes
*/
@Test
public void testFormatterNotificationOnForwardWord() {
assertFormatterNotificationOnNavigation(t -> t.endOfNextWord());
}
/**
* Test notification on navigation: home.
* for comparison - passes
*/
@Test
public void testFormatterNotificationOnHome() {
int pos = 3;
assertFormatterNotificationOnNavigation(
t -> t.selectRange(pos, pos),
t -> t.home());
}
/**
* Test that navigation produces a single notification of Textformatter.
*
* @param preFormatter the initial state transition before
* setting the formatter, may be null if initial state is default
* @param navigation the navigation
*/
protected void assertFormatterNotificationOnNavigation(
Consumer<TextField> preFormatter,
Consumer<TextField> navigation) {
assertFormatterNotificationOnNavigation(preFormatter, navigation, null, null);
}
/**
* Test that navigation produces a single notification of Textformatter.
*
* @param navigation the navigation
*/
protected void assertFormatterNotificationOnNavigation(Consumer<TextField> navigation) {
assertFormatterNotificationOnNavigation(null, navigation, null, null);
}
/**
* Test that navigation produces a single notification of textFormatter.
* A textField to navigate on is created with text "The quick brown fox"
* and a TextFormatter with a filter implemented to count the notifications.
*
* @param preFormatter the initial state transition before
* setting the formatter, may be null if initial state is default
* @param navigation the navigation
* @param anchorProvider the expected anchor after navigation - sanity check, may be null.
* @param caretProvider the expected caret after navigation - sanity check, may be null.
*/
protected void assertFormatterNotificationOnNavigation(
Consumer<TextField> preFormatter,
Consumer <TextField> navigation,
Function<TextField, Integer> anchorProvider,
Function<TextField, Integer> caretProvider) {
// textfield with text "The quick brown fox"
TextField txtField = createTextField("The quick brown fox");
// initial state - do before setting the formatter to not include in counting
if (preFormatter != null) {
preFormatter.accept(txtField);
}
IntegerProperty count = new SimpleIntegerProperty(0);
UnaryOperator<TextFormatter.Change> filter = c -> {
count.set(count.get() + 1);
return c;
};
TextFormatter<String> formatter = new TextFormatter<>(filter);
txtField.setTextFormatter(formatter);
// navigate
navigation.accept(txtField);
// sanity tests of anchor/caret pos
// here we are not really interested in those (they are tested extensively
// in TextInputControlTest) - just for digging if things go wrong unexpectedly
if (anchorProvider != null) {
int anchor = anchorProvider.apply(txtField);
assertEquals("sanity: anchor at end", anchor, txtField.getAnchor());
}
if (caretProvider != null) {
int caret = caretProvider.apply(txtField);
assertEquals("sanity: caret at end", caret, txtField.getCaretPosition());
}
assertEquals("must have received a single notification", 1, count.get());
}
}
- relates to
-
JDK-8210297 TextFormatter filter getting called for all components instead of one.
-
- Open
-