Skip to content

Commit

Permalink
8268877: TextInputControlSkin: incorrect inputMethod event handler af…
Browse files Browse the repository at this point in the history 
…ter switching skin

Reviewed-by: aghaisas, kcr
  • Loading branch information
@kevinrushforth
Andy Goryachev authored and kevinrushforth committed Nov 14, 2022
1 parent 7ec0852 commit a13630f
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 85 deletions.
126 changes: 66 additions & 60 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextInputControlSkin.java
View file
Expand Up @@ -74,6 +74,7 @@
import javafx.scene.control.TextInputControl;
import javafx.scene.input.InputMethodEvent;
import javafx.scene.input.InputMethodHighlight;
import javafx.scene.input.InputMethodRequests;
import javafx.scene.input.InputMethodTextRun;
import javafx.scene.layout.StackPane;
import javafx.scene.paint.Color;
Expand Down Expand Up @@ -192,7 +193,8 @@ public static enum Direction {
// Holds concrete attributes for the composition runs
private List<Shape> imattrs = new java.util.ArrayList<Shape>();

private EventHandler<InputMethodEvent> inputMethodTextChangedHandler;
private final EventHandler<InputMethodEvent> inputMethodTextChangedHandler = this::handleInputMethodEvent;
private InputMethodRequests inputMethodRequests;


/* ************************************************************************
Expand Down Expand Up @@ -325,82 +327,86 @@ protected boolean computeValue() {
});
}

// FIXME: JDK-8268877 - incorrectly wired handler on replacing skin
if (control.getOnInputMethodTextChanged() == null) {
inputMethodTextChangedHandler = this::handleInputMethodEvent;
control.setOnInputMethodTextChanged(inputMethodTextChangedHandler);
}
control.addEventHandler(InputMethodEvent.INPUT_METHOD_TEXT_CHANGED, inputMethodTextChangedHandler);
}

control.setInputMethodRequests(new ExtendedInputMethodRequests() {
@Override public Point2D getTextLocation(int offset) {
Scene scene = getSkinnable().getScene();
Window window = scene != null ? scene.getWindow() : null;
if (window == null) {
return new Point2D(0, 0);
@Override
public void install() {
super.install();

TextInputControl control = getSkinnable();

if (control.getInputMethodRequests() == null) {
inputMethodRequests = new ExtendedInputMethodRequests() {
@Override public Point2D getTextLocation(int offset) {
Scene scene = control.getScene();
Window window = scene != null ? scene.getWindow() : null;
if (window == null) {
return new Point2D(0, 0);
}
// Don't use imstart here because it isn't initialized yet.
Rectangle2D characterBounds = getCharacterBounds(control.getSelection().getStart() + offset);
Point2D p = control.localToScene(characterBounds.getMinX(), characterBounds.getMaxY());
Point2D location = new Point2D(window.getX() + scene.getX() + p.getX(),
window.getY() + scene.getY() + p.getY());
return location;
}
// Don't use imstart here because it isn't initialized yet.
Rectangle2D characterBounds = getCharacterBounds(control.getSelection().getStart() + offset);
Point2D p = getSkinnable().localToScene(characterBounds.getMinX(), characterBounds.getMaxY());
Point2D location = new Point2D(window.getX() + scene.getX() + p.getX(),
window.getY() + scene.getY() + p.getY());
return location;
}

@Override public int getLocationOffset(int x, int y) {
return getInsertionPoint(x, y);
}

@Override public void cancelLatestCommittedText() {
// TODO
}
@Override public int getLocationOffset(int x, int y) {
return getInsertionPoint(x, y);
}

@Override public String getSelectedText() {
TextInputControl control = getSkinnable();
IndexRange selection = control.getSelection();
@Override public void cancelLatestCommittedText() {
// TODO
}

return control.getText(selection.getStart(), selection.getEnd());
}
@Override public String getSelectedText() {
IndexRange selection = control.getSelection();
return control.getText(selection.getStart(), selection.getEnd());
}

@Override public int getInsertPositionOffset() {
int caretPosition = getSkinnable().getCaretPosition();
if (caretPosition < imstart) {
return caretPosition;
} else if (caretPosition < imstart + imlength) {
return imstart;
} else {
return caretPosition - imlength;
@Override public int getInsertPositionOffset() {
int caretPosition = control.getCaretPosition();
if (caretPosition < imstart) {
return caretPosition;
} else if (caretPosition < imstart + imlength) {
return imstart;
} else {
return caretPosition - imlength;
}
}
}

@Override public String getCommittedText(int begin, int end) {
TextInputControl control = getSkinnable();
if (begin < imstart) {
if (end <= imstart) {
return control.getText(begin, end);
@Override public String getCommittedText(int begin, int end) {
if (begin < imstart) {
if (end <= imstart) {
return control.getText(begin, end);
} else {
return control.getText(begin, imstart) + control.getText(imstart + imlength, end + imlength);
}
} else {
return control.getText(begin, imstart) + control.getText(imstart + imlength, end + imlength);
return control.getText(begin + imlength, end + imlength);
}
} else {
return control.getText(begin + imlength, end + imlength);
}
}

@Override public int getCommittedTextLength() {
return getSkinnable().getText().length() - imlength;
}
});
@Override public int getCommittedTextLength() {
return control.getText().length() - imlength;
}
};
control.setInputMethodRequests(inputMethodRequests);
}
}

@Override
public void dispose() {
if (getSkinnable() == null) return;
// the inputMethodEvent handler installed by this skin must be removed to prevent a memory leak
// while a handler installed by the control must not be removed
if (getSkinnable().getOnInputMethodTextChanged() == inputMethodTextChangedHandler) {
getSkinnable().setOnInputMethodTextChanged(null);
if (getSkinnable() == null) {
return;
}

getSkinnable().removeEventHandler(InputMethodEvent.INPUT_METHOD_TEXT_CHANGED, inputMethodTextChangedHandler);

if (getSkinnable().getInputMethodRequests() == inputMethodRequests) {
getSkinnable().setInputMethodRequests(null);
}
// cleanup to guard against potential NPE
getSkinnable().setInputMethodRequests(null);
super.dispose();
}

Expand Down
122 changes: 97 additions & 25 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -25,32 +25,42 @@

package test.javafx.scene.control.skin;

import static javafx.collections.FXCollections.observableArrayList;
import static javafx.scene.control.ControlShim.installDefaultSkin;
import static javafx.scene.control.SkinBaseShim.unregisterChangeListeners;
import static javafx.scene.control.skin.TableSkinShim.getCells;
import static javafx.scene.control.skin.TableSkinShim.getTableViewSkin;
import static javafx.scene.control.skin.TableSkinShim.getVirtualFlow;
import static javafx.scene.control.skin.TableSkinShim.isDirty;
import static javafx.scene.control.skin.TableSkinShim.isFixedCellSizeEnabled;
import static javafx.scene.control.skin.TextInputSkinShim.getPromptNode;
import static javafx.scene.control.skin.TextInputSkinShim.getScrollPane;
import static javafx.scene.control.skin.TextInputSkinShim.getTextNode;
import static javafx.scene.control.skin.TextInputSkinShim.getTextTranslateX;
import static javafx.scene.control.skin.TextInputSkinShim.setHandlePressed;
import static javafx.scene.layout.Region.USE_COMPUTED_SIZE;
import static javafx.scene.layout.Region.USE_PREF_SIZE;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.attemptGC;
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.replaceSkin;
import static test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils.getCell;

import java.lang.ref.WeakReference;
import java.util.List;
import java.util.stream.Collectors;

import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;

import com.sun.javafx.tk.Toolkit;

import static javafx.collections.FXCollections.*;
import static javafx.scene.control.ControlShim.*;
import static javafx.scene.control.SkinBaseShim.*;
import static javafx.scene.control.skin.TableSkinShim.*;
import static javafx.scene.control.skin.TableSkinShim.getVirtualFlow;
import static javafx.scene.control.skin.TextInputSkinShim.*;
import static org.junit.Assert.*;
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
import static test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils.*;

import javafx.beans.property.SimpleStringProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.event.Event;
import javafx.event.EventHandler;
import javafx.geometry.Point2D;
import javafx.geometry.Pos;
import javafx.scene.Scene;
import javafx.scene.control.Button;
Expand Down Expand Up @@ -82,13 +92,22 @@
import javafx.scene.control.skin.TreeTableRowSkin;
import javafx.scene.control.skin.VirtualFlow;
import javafx.scene.input.InputMethodEvent;
import javafx.scene.input.InputMethodRequests;
import javafx.scene.input.ScrollEvent;
import javafx.scene.layout.Pane;
import javafx.scene.layout.VBox;
import javafx.scene.shape.Rectangle;
import javafx.scene.text.Font;
import javafx.scene.text.Text;
import javafx.stage.Stage;

import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;

import com.sun.javafx.tk.Toolkit;

import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
import test.com.sun.javafx.scene.control.test.Person;

Expand Down Expand Up @@ -1264,7 +1283,6 @@ public void testTextInputOnInputMethodTextChangedReplacedHandler() {
* Test that handler installed by skin is reset on replacing skin.
* Here we test the effect by firing an inputEvent.
*/
@Ignore("JDK-8268877")
@Test
public void testTextInputOnInputMethodTextChangedEvent() {
String initialText = "some text";
Expand All @@ -1281,18 +1299,72 @@ public void testTextInputOnInputMethodTextChangedEvent() {
}

/**
* Test that handler installed by skin is reset on replacing skin.
* Here we test the instance of the handler.
* Test that handler installed by the user is not reset on replacing skin.
*/
@Ignore("JDK-8268877")
@Test
public void testTextInputOnInputMethodTextChangedHandler() {
public void testTextInputUserOnInputMethodTextChangedHandler() {
TextField field = new TextField("some text");
EventHandler<InputMethodEvent> h = (ev) -> { };
field.setOnInputMethodTextChanged(h);

installDefaultSkin(field);

EventHandler<? super InputMethodEvent> handler = field.getOnInputMethodTextChanged();

replaceSkin(field);
assertNotSame("replaced skin must replace skin handler", handler, field.getOnInputMethodTextChanged());
assertNotNull("handler must not be null ", field.getOnInputMethodTextChanged());

assertSame("user handler must not be changed", h, handler);
assertSame("replaced skin must not change handler", handler, field.getOnInputMethodTextChanged());
}

/**
* Test that input method requests installed by skin is reset on replacing skin.
*/
@Test
public void testTextInput_InputMethodRequestsIsResetOnReplacingSkin() {
TextField t = new TextField();
installDefaultSkin(t);
InputMethodRequests im = t.getInputMethodRequests();

replaceSkin(t);
InputMethodRequests im2 = t.getInputMethodRequests();

assertNotEquals("InputMethodRequests set by an old skin must be replaced by the new skin", im, im2);
}

/**
* Test that the user input method requests is not affected by the skin.
*/
@Test
public void testTextInput_UserMethodRequestsNotAffectedBySkin() {
InputMethodRequests im = createInputMethodRequests();
TextField t = new TextField();
t.setInputMethodRequests(im);
installDefaultSkin(t);
assertEquals("skin must not alter user-set InputMethodRequests", im, t.getInputMethodRequests());
}

protected static InputMethodRequests createInputMethodRequests() {
return new InputMethodRequests() {
@Override
public Point2D getTextLocation(int offset) {
return new Point2D(0, 0);
}

@Override
public int getLocationOffset(int x, int y) {
return 0;
}

@Override
public void cancelLatestCommittedText() {
}

@Override
public String getSelectedText() {
return "";
}
};
}


Expand Down

1 comment on commit a13630f

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.