Skip to content

Commit

Permalink
8319844: Text/TextFlow.hitTest() is incorrect in RTL orientation
Browse files Browse the repository at this point in the history 
Co-authored-by: John Hendrikx <[email protected]>
Reviewed-by: angorya, jhendrikx
  • Loading branch information
@karthikpandelu @hjohn
karthikpandelu and hjohn committed Mar 5, 2024
1 parent 8114559 commit 66d9681
Show file tree
Hide file tree
Showing 10 changed files with 1,100 additions and 98 deletions.
31 changes: 25 additions & 6 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import com.sun.javafx.geom.BaseBounds;
import com.sun.javafx.geom.Shape;

import java.util.Objects;

public interface TextLayout {

/* Internal flags Flags */
Expand Down Expand Up @@ -91,6 +93,28 @@ public Hit(int charIndex, int insertionIndex, boolean leading) {
public int getCharIndex() { return charIndex; }
public int getInsertionIndex() { return insertionIndex; }
public boolean isLeading() { return leading; }

@Override
public int hashCode() {
return Objects.hash(charIndex, insertionIndex, leading);
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
Hit other = (Hit) obj;
return charIndex == other.charIndex && insertionIndex == other.insertionIndex && leading == other.leading;
}

@Override
public String toString() {
return "Hit[charIndex=" + charIndex + ", insertionIndex=" + insertionIndex + ", leading=" + leading + "]";
}
}

/**
Expand Down Expand Up @@ -205,14 +229,9 @@ public Hit(int charIndex, int insertionIndex, boolean leading) {
*
* @param x x coordinate value.
* @param y y coordinate value.
* @param text text for which HitInfo needs to be calculated.
* It is expected to be null in the case of {@link javafx.scene.text.TextFlow}
* and non-null in the case of {@link javafx.scene.text.Text}
* @param textRunStart Text run start position.
* @param curRunStart starting position of text run where hit info is requested.
* @return returns a {@link Hit} object containing character index, insertion index and position of cursor on the character.
*/
public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart);
public Hit getHitInfo(float x, float y);

public PathElement[] getCaretShape(int offset, boolean isLeading,
float x, float y);
Expand Down
85 changes: 13 additions & 72 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,90 +421,44 @@ public PathElement[] getCaretShape(int offset, boolean isLeading,
}

@Override
public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart) {
public Hit getHitInfo(float x, float y) {
int charIndex = -1;
int insertionIndex = -1;
boolean leading = false;
int relIndex = 0;
int textWidthPrevLine = 0;

ensureLayout();
int lineIndex = getLineIndex(y, text, curRunStart);
int lineIndex = getLineIndex(y);
if (lineIndex >= getLineCount()) {
charIndex = getCharCount();
insertionIndex = charIndex + 1;
} else {
if (isMirrored()) {
x = getMirroringWidth() - x;
}
TextLine line = lines[lineIndex];
TextRun[] runs = line.getRuns();
RectBounds bounds = line.getBounds();
TextRun run = null;
x -= bounds.getMinX();
//TODO binary search
if (text == null || spans == null) {
for (int i = 0; i < runs.length; i++) {
run = runs[i];
if (x < run.getWidth()) break;
if (i + 1 < runs.length) {
if (runs[i + 1].isLinebreak()) break;
x -= run.getWidth();
}
}
} else {
for (int i = 0; i < lineIndex; i++) {
for (TextRun r: lines[i].runs) {
if (r.getTextSpan() != null && r.getStart() >= textRunStart && r.getTextSpan().getText().equals(text)) {
textWidthPrevLine += r.getLength();
}
}
for (int i = 0; i < runs.length; i++) {
run = runs[i];
if (x < run.getWidth()) {
break;
}
int prevNodeLength = 0;
boolean isPrevNodeExcluded = false;
for (TextRun r: runs) {
if (!r.getTextSpan().getText().equals(text) || (r.getStart() < textRunStart && r.getTextSpan().getText().equals(text))) {
prevNodeLength += r.getWidth();
continue;
}
if (r.getTextSpan() != null && r.getTextSpan().getText().equals(text)) {
BaseBounds textBounds = new BoxBounds();
getBounds(r.getTextSpan(), textBounds);
if (textBounds.getMinX() == 0 && !isPrevNodeExcluded) {
x -= prevNodeLength;
isPrevNodeExcluded = true;
}
if (x > r.getWidth()) {
x -= r.getWidth();
relIndex += r.getLength();
continue;
}
run = r;
if (i + 1 < runs.length) {
if (runs[i + 1].isLinebreak()) {
break;
}
x -= run.getWidth();
}
}

if (run != null) {
int[] trailing = new int[1];
if (text != null && spans != null) {
charIndex = run.getOffsetAtX(x, trailing);
charIndex += textWidthPrevLine;
charIndex += relIndex;
} else {
charIndex = run.getStart() + run.getOffsetAtX(x, trailing);
}
charIndex = run.getStart() + run.getOffsetAtX(x, trailing);
leading = (trailing[0] == 0);

insertionIndex = charIndex;
if (getText() != null && insertionIndex < getText().length) {
if (!leading) {
BreakIterator charIterator = BreakIterator.getCharacterInstance();
if (text != null) {
charIterator.setText(text);
} else {
charIterator.setText(new String(getText()));
}
charIterator.setText(new String(getText()));
int next = charIterator.following(insertionIndex);
if (next == BreakIterator.DONE) {
insertionIndex += 1;
Expand Down Expand Up @@ -749,30 +703,17 @@ public boolean setTabSize(int spaces) {
* *
**************************************************************************/

private int getLineIndex(float y, String text, int runStart) {
private int getLineIndex(float y) {
int index = 0;
float bottom = 0;
/* Initializing textFound as true when text is null
* because when this function is called for TextFlow text parameter will be null */
boolean textFound = (text == null);

int lineCount = getLineCount();
while (index < lineCount) {
if (!textFound) {
for (TextRun r : lines[index].runs) {
if (r.getTextSpan() == null || (r.getStart() == runStart && r.getTextSpan().getText().equals(text))) {
/* Span will present only for Rich Text.
* Hence making textFound as true */
textFound = true;
break;
}
}
}
bottom += lines[index].getBounds().getHeight() + spacing;
if (index + 1 == lineCount) {
bottom -= lines[index].getLeading();
}
if (bottom > y && textFound) {
if (bottom > y) {
break;
}
index++;
Expand Down
40 changes: 23 additions & 17 deletions modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1021,27 +1021,33 @@ public final BooleanProperty caretBiasProperty() {
public final HitInfo hitTest(Point2D point) {
if (point == null) return null;
TextLayout layout = getTextLayout();

double x = point.getX() - getX();
double y = point.getY() - getY() + getYRendering();
GlyphList[] runs = getRuns();
int runIndex = 0;
if (runs.length != 0) {
double ptY = localToParent(x, y).getY();
while (runIndex < runs.length - 1) {
if (ptY > runs[runIndex].getLocation().y && ptY < runs[runIndex + 1].getLocation().y) {
break;
}
runIndex++;
}

int textRunStart = findFirstRunStart();

double px = x;
double py = y;

if (isSpan()) {
Point2D pPoint = localToParent(point);
px = pPoint.getX();
py = pPoint.getY();
}
int textRunStart = 0;
int curRunStart = 0;
if (runs.length != 0) {
textRunStart = ((TextRun) runs[0]).getStart();
curRunStart = ((TextRun) runs[runIndex]).getStart();
TextLayout.Hit h = layout.getHitInfo((float)px, (float)py);
return new HitInfo(h.getCharIndex() - textRunStart, h.getInsertionIndex() - textRunStart, h.isLeading());
}

private int findFirstRunStart() {
int start = Integer.MAX_VALUE;
for (GlyphList r: getRuns()) {
int runStart = ((TextRun) r).getStart();
if (runStart < start) {
start = runStart;
}
}
TextLayout.Hit h = layout.getHitInfo((float)x, (float)y, getText(), textRunStart, curRunStart);
return new HitInfo(h.getCharIndex(), h.getInsertionIndex(), h.isLeading());
return start;
}

private PathElement[] getRange(int start, int end, int type) {
Expand Down
2 changes: 1 addition & 1 deletion modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public final HitInfo hitTest(javafx.geometry.Point2D point) {
TextLayout layout = getTextLayout();
double x = point.getX();
double y = point.getY();
TextLayout.Hit h = layout.getHitInfo((float)x, (float)y, null, 0, 0);
TextLayout.Hit h = layout.getHitInfo((float)x, (float)y);
return new HitInfo(h.getCharIndex(), h.getInsertionIndex(), h.isLeading());
} else {
return null;
Expand Down
2 changes: 1 addition & 1 deletion modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public Shape getShape(int type, TextSpan filter) {
}

@Override
public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart) {
public Hit getHitInfo(float x, float y) {
// TODO this probably needs to be entirely rewritten...
if (getText() == null) {
return new Hit(0, -1, true);
Expand Down
2 changes: 1 addition & 1 deletion tests/system/src/test/.classpath
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<classpathentry combineaccessrules="false" kind="src" path="/graphics">
<attributes>
<attribute name="module" value="true"/>
<attribute name="add-exports" value="javafx.graphics/com.sun.glass.ui=ALL-UNNAMED:javafx.graphics/com.sun.glass.ui.monocle=ALL-UNNAMED:javafx.graphics/com.sun.javafx.sg.prism=ALL-UNNAMED:javafx.graphics/com.sun.prism.impl=ALL-UNNAMED:javafx.graphics/com.sun.javafx.image.impl=ALL-UNNAMED:javafx.graphics/com.sun.glass.events=ALL-UNNAMED:javafx.graphics/com.sun.javafx.application=ALL-UNNAMED:javafx.graphics/com.sun.javafx.css=ALL-UNNAMED:javafx.graphics/com.sun.javafx.geom=ALL-UNNAMED:javafx.graphics/com.sun.javafx.tk=ALL-UNNAMED:javafx.graphics/com.sun.glass.ui.mac=ALL-UNNAMED"/>
<attribute name="add-exports" value="javafx.graphics/com.sun.glass.ui=ALL-UNNAMED:javafx.graphics/com.sun.glass.ui.monocle=ALL-UNNAMED:javafx.graphics/com.sun.javafx.sg.prism=ALL-UNNAMED:javafx.graphics/com.sun.prism.impl=ALL-UNNAMED:javafx.graphics/com.sun.javafx.image.impl=ALL-UNNAMED:javafx.graphics/com.sun.glass.events=ALL-UNNAMED:javafx.graphics/com.sun.javafx.application=ALL-UNNAMED:javafx.graphics/com.sun.javafx.css=ALL-UNNAMED:javafx.graphics/com.sun.javafx.geom=ALL-UNNAMED:javafx.graphics/com.sun.javafx.tk=ALL-UNNAMED:javafx.graphics/com.sun.glass.ui.mac=ALL-UNNAMED:javafx.graphics/com.sun.javafx.scene.text=ALL-UNNAMED:javafx.graphics/com.sun.javafx.text=ALL-UNNAMED:javafx.graphics/com.sun.javafx.font=ALL-UNNAMED"/>
</attributes>
</classpathentry>
<classpathentry combineaccessrules="false" kind="src" path="/controls">
Expand Down
2 changes: 2 additions & 0 deletions tests/system/src/test/addExports
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
--add-exports javafx.graphics/com.sun.javafx.image=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.javafx.sg.prism=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.javafx.scene=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.javafx.scene.text=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.javafx.text=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.javafx.tk=ALL-UNNAMED
--add-exports javafx.graphics/com.sun.prism.impl=ALL-UNNAMED
#
Expand Down
Loading

0 comments on commit 66d9681

Please sign in to comment.