Skip to content

8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation #1323

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

Closed
wants to merge 17 commits into from
Closed

8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation #1323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8d42bd3
Fix rtl text hittest issue
karthikpandelu Jan 9, 2024
b23e491
Code review changes
karthikpandelu Jan 11, 2024
cd270e1
Code review changes
karthikpandelu Jan 16, 2024
87255aa
Fix multiline text insertion index calculation issue
karthikpandelu Jan 17, 2024
b08de73
Review changes
karthikpandelu Jan 18, 2024
a58f754
Fix issue with RTL text within LTR text
karthikpandelu Jan 29, 2024
52ee61c
Fix issue with multiline text
karthikpandelu Jan 31, 2024
3012fae
Code review changes
karthikpandelu Feb 1, 2024
cc89f12
Inline node issue fix
karthikpandelu Feb 6, 2024
43f1f18
Fix emoji issue
karthikpandelu Feb 7, 2024
8732047
Review comments
karthikpandelu Feb 9, 2024
7228785
Code refactoring
karthikpandelu Feb 19, 2024
e381273
Merge branch 'master' into rtl_hittest_issue
karthikpandelu Feb 21, 2024
f36199f
Fix repeating text node issue
karthikpandelu Feb 27, 2024
1376441
Simplified approach
karthikpandelu Feb 28, 2024
130587c
Review comments
karthikpandelu Feb 29, 2024
7ef09bf
Add unit test
karthikpandelu Mar 4, 2024
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
31 changes: 25 additions & 6 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
View file
Open in desktop
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())
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why this code uses getClass() != obj.getClass() ?

perhaps a better choice might be the usual pattern

if(x == this) {
 return true;
} else if(x instanceof Hit h) {
 return charIndex == h.charIndex && ...
}
return false;

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are long discussions about this, and not really worth going into. They both have their place. Generally, the getClass versions are easier to get right without violating the equals contract. instanceof can be used, to allow comparisons with (trivial) subtypes, but then you must make equals final. Anything fancier violates the equals contract.

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
Open in desktop
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
Open in desktop
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()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the old code had a 0 check for getRuns.length, presumably to avoid the iterator creation.
the new code is probably fine, because most of the time we'll have the need for the iterator anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

getRuns returns an array, it won't create an Iterator.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right!

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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