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

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

Closed
wants to merge 1 commit into from
Closed

8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,10 @@ protected TextInputControl(final Content content) {
int start = sel.getStart();
int end = sel.getEnd();
int length = txt.length();
if (end > start + length) end = length;
if (start > length-1) start = end = 0;
// Ensure that the last character to get is within the bounds of the txt string
if (end >= start + length) end = length-1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, I think the existing test is simply wrong: Why should the start value matter when checking whether the end value needs to be clamped -- it's an end point not a number of characters. I think the entire problem might be the fact that it is comparing against start + length. Second, the value to be clamped to was correct before your change. The substring method to which end is passed is documented as subtracting 1.

I think the test should be something like:

    if (end > length) end = length;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second thought: correct clamping or not, the start/end indices of selection are invalid whenever text was removed/added at an index before the selection - they are in coordinates of the old text, not the new. If they point to an index > new textLength, incorrect clamping will throw. If they are both < new textLength, it'll fire an intermediate changeEvent with incorrect selection, the total sequence beining
oldSelectedText -> incorrectly-shifted-selectedText
incorrectly-shifted-selectedText -> empty

The correct change notification would be, because at the end of the text change, the selection is cleared,
oldSelectedText -> empty

To me, it looks like using a binding here is not the best of options.

// In case the start is after the whole txt, nothing valid is selected. Thus, return the default.
if (start >= length) return "";
Copy link
Member

@kevinrushforth kevinrushforth Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems OK, and might be clearer than the existing code, but the existing code is correct, and produces the same effect.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it is correct - but while we are changing the impl, we might go the whole way and clean up :) my pref would be to add the check for start to the short-circuit at the beginning of the method, something like

    String txt = text.get();
    IndexRange sel = selection.get();
    if (txt == null || sel == null || sel.getStart() >= text.getLength()) return "";

return txt.substring(start, end);
}
});
Expand Down
104 changes: 104 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

package test.javafx.scene.control;

import javafx.application.Platform;
import javafx.beans.InvalidationListener;
import javafx.beans.Observable;
import javafx.beans.property.BooleanProperty;
Expand All @@ -35,17 +36,21 @@
import javafx.beans.value.ObservableValue;
import javafx.css.CssMetaData;
import javafx.css.StyleableProperty;
import javafx.event.Event;
import javafx.event.EventHandler;
import javafx.scene.Scene;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyEvent;
import javafx.scene.input.Clipboard;
import javafx.scene.input.ClipboardContent;
import javafx.scene.layout.VBox;
import javafx.scene.text.Font;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;
import java.util.Arrays;
import java.util.Collection;
import java.util.concurrent.Semaphore;

import javafx.scene.control.IndexRange;
import javafx.scene.control.PasswordField;
import javafx.scene.control.TextArea;
Expand Down Expand Up @@ -2043,6 +2048,105 @@ public void caretAndAnchorPositionAfterSettingText() {
tk.firePulse();
}

// Test for case 1 of JDK-8176270
@Test public void addingListenerWorks() {
VBox vBox = new VBox();
TextField textField = new TextField();
textField.setText("1234 5678");
vBox.getChildren().add(textField);
textField.selectedTextProperty()
.addListener((observable -> {}));

Scene scene = new Scene(vBox);
Stage stage = new Stage();
stage.setScene(scene);
stage.show();
}

// Test for case 2 of JDK-8176270
@Test public void replaceSelectionWorks() throws Exception {
VBox vBox = new VBox();
TextField textField = new TextField();
textField.setText("1234 5678");
vBox.getChildren().add(textField);
textField.selectedTextProperty()
.addListener((observable -> {}));

Scene scene = new Scene(vBox);
Stage stage = new Stage();
stage.setScene(scene);
stage.show();

textField.selectedTextProperty()
.addListener(observable -> {
// accessing the selectedTextProperty causes a
// StringOutOfBoundsException
observable.toString();
});
textField.positionCaret(5);
Semaphore semaphore = new Semaphore(0);
Platform.runLater(semaphore::release);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since controls tests run using the StubToolkit, Platform.runLater does not do what you expect. If you need to run a pulse to get some code to run that normally would run as part of pulse, you can call that method directly. See other controls tests for how this is done.

semaphore.acquire();

// select 2nd word
textField.selectNextWord();
semaphore = new Semaphore(0);
Platform.runLater(semaphore::release);
semaphore.acquire();

// replace selection
Platform.runLater(() -> {Event.fireEvent(scene, new KeyEvent(KeyEvent.KEY_PRESSED, "", KeyCode.DIGIT0.getName(), KeyCode.DIGIT0, false, false, false, false));});
Platform.runLater(() -> {Event.fireEvent(scene, new KeyEvent(KeyEvent.KEY_RELEASED, "", KeyCode.DIGIT0.getName(), KeyCode.DIGIT0, false, false, false, false));});
semaphore = new Semaphore(0);
Platform.runLater(semaphore::release);
semaphore.acquire();
}

// Test for workaround of JDK-8176270
@Test public void accessingTheValueInInvalidationListenerWorks() throws Exception {
VBox vBox = new VBox();
TextField textField = new TextField();
textField.setText("1234 5678");
vBox.getChildren().add(textField);
textField.selectedTextProperty()
.addListener((observable -> {}));

Scene scene = new Scene(vBox);
Stage stage = new Stage();
stage.setScene(scene);
stage.show();

textField.selectedTextProperty()
.addListener(new InvalidationListener() {
@Override
public void invalidated(Observable observable) {
Platform.runLater(() -> observable.toString());
}
});

textField.positionCaret(5);
Semaphore semaphore = new Semaphore(0);
Platform.runLater(semaphore::release);
semaphore.acquire();

// select 2nd word
textField.selectNextWord();
semaphore = new Semaphore(0);
Platform.runLater(semaphore::release);
semaphore.acquire();

// replace selection
Platform.runLater(() -> {Event.fireEvent(scene,
new KeyEvent(KeyEvent.KEY_PRESSED, "", KeyCode.DIGIT0.getName(), KeyCode.DIGIT0,
false, false, false, false));});
Platform.runLater(() -> {Event.fireEvent(scene,
new KeyEvent(KeyEvent.KEY_RELEASED, "", KeyCode.DIGIT0.getName(), KeyCode.DIGIT0,
false, false, false, false));});
semaphore = new Semaphore(0);
Platform.runLater(semaphore::release);
semaphore.acquire();
}

// TODO tests for Content firing event notification properly

// TODO tests for Content not allowing illegal characters
Expand Down