Skip to content

8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs #1091

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 9 commits into from
Closed

8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs #1091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f741feb
Fix insertion index calculation issue in TextFlow
karthikpandelu Apr 14, 2023
e35f5bf
Fix insertion index calculation issue with emojis. Add additional tes…
karthikpandelu May 8, 2023
0360055
Address code review
karthikpandelu May 9, 2023
7919d5b
Initialize insertion index in PrismTextLayout
karthikpandelu May 15, 2023
1eb1174
Change insertion index initialization approach. Add additional test
karthikpandelu May 16, 2023
a43bbdb
Add new test. Optimizations to make the tests robust
karthikpandelu May 18, 2023
6684075
Stabilizing the system tests
karthikpandelu May 22, 2023
4d6dd9f
Merge master to resolve merge conflict
karthikpandelu May 22, 2023
ea940bf
Add out of bound check for insertion index
karthikpandelu Jun 5, 2023
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
21 changes: 20 additions & 1 deletion 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 @@ -422,12 +422,14 @@ public PathElement[] getCaretShape(int offset, boolean isLeading,
@Override
public Hit getHitInfo(float x, float y) {
int charIndex = -1;
int insertionIndex = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

[question]

Currently, there are a few scenarios when a negative insertionIndex is passed down to HitInfo. This will trigger a similar (and probably incorrect) computation of the insertion index in HitInfo, see for example JDK-8302511.

My question is - should we instead resolve the insertion index always?

Copy link
Member Author

Choose a reason for hiding this comment

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

After current fix, only scenario where insertionIndex not getting initialized will be when lineIndex >= getLineCount() is true. So even if insertionIndex -1, text will be null and HitInfo.getInsertionIndex() will not have any computation to perform. So we can resolve the insertion index always I think. Please let me know your thoughts on this.

boolean leading = false;

ensureLayout();
int lineIndex = getLineIndex(y);
if (lineIndex >= getLineCount()) {
charIndex = getCharCount();
insertionIndex = charIndex + 1;
} else {
if (isMirrored()) {
x = getMirroringWidth() - x;
Expand All @@ -450,13 +452,30 @@ public Hit getHitInfo(float x, float y) {
int[] trailing = new int[1];
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();
charIterator.setText(new String(getText()));
int next = charIterator.following(insertionIndex);
if (next == BreakIterator.DONE) {
insertionIndex += 1;
} else {
insertionIndex = next;
}
}
} else if (!leading) {
insertionIndex += 1;
}
} else {
//empty line, set to line break leading
charIndex = line.getStart();
leading = true;
insertionIndex = charIndex;
}
}
return new Hit(charIndex, -1, leading);
return new Hit(charIndex, insertionIndex, leading);
}

@Override
Expand Down
Loading