8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs #1091
Conversation
|
👋 Welcome back kpk! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Reviewers: @prrace @andy-goryachev-oracle |
|
I still see the problem in the Monkey Tester. Open TextFlow page, select Emojis text, 48 size, and slowly move the mouse across the text. You can see the insertionIndex differ between Text and TextFlow (and incorrect in the latter case). See here, the vertical line is where the mouse is (middle of the heart symbol):
|
andy-goryachev-oracle
left a comment
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
could you please check with the Monkey Tester?
I could be doing something wrong.
| Util.waitForLatch(mouseClickLatch, 10, "Timeout waiting for mouse click"); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
would it be possible to add another test, where the cursor is moved across an emoji character one pixel at a time, making sure that insertionIndex monotonically increases?
as a variant, move across a text string with multiple emojis?
There was a problem hiding this comment.
Sure. I will try to add these test cases.
I checked above scenario in Monkey tester and I'm also seeing same issue. But in a simple test app which I created separately, it looks to be working. I will do some more testing and see what is the exact issue. |
I wonder what could be the difference? text size? |
Text size is one difference. So looks like the the combination of different text and emojis are giving different results. I will check these and resume work on this PR after the test sprint. |
|
Fixed the above mentioned issue in calculating insertion index by adding insertion index calculation logic to |
andy-goryachev-oracle
left a comment
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
minor changes requested
| if (Character.isHighSurrogate(txt.charAt(relIdx)) && !leading) { | ||
| insertionIndex = charIndex + 2; | ||
| insertionIndex = charIndex; | ||
| String txt = new String(getText()); |
There was a problem hiding this comment.
possible NPE: looks like getText() might return null, in which case this line will throw an NPE.
| insertionIndex = charIndex; | ||
| String txt = new String(getText()); | ||
| if (!leading) { | ||
| if (txt != null) { |
There was a problem hiding this comment.
This check should be done earlier for getText()
There was a problem hiding this comment.
Added null check for getText()
| if (txt != null) { | ||
| int next; | ||
| BreakIterator charIterator = BreakIterator.getCharacterInstance(); | ||
| synchronized(charIterator) { |
There was a problem hiding this comment.
question: why do we need to synchronize on the instance here? BreakIterator.get*Instance() creates a new instance each time, so synchronization is probably unnecessary.
There was a problem hiding this comment.
Correct it is not necessary. Updated code to remove synchronization
|
Does not seem to work correctly still - the insertion index in TextFlow should not point in between surrogate pair of characters that comprise an emoji. Basically, it should be exactly the same as with Text.hitInfo():
|
This condition is working for me as |
andy-goryachev-oracle
left a comment
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Testing looks good so far.
There is one question whether it's possible to always initialize HitInfo.insertionIndex and remove the secondary computation from HitInfo.getInsertionIndex()
I am not clear under which conditions it is not possible to initialize, and whether subsequent HitInfo.getInsertionIndex() would produce a meaningful result.
Perhaps in a separate RFE.
| @Override | ||
| public Hit getHitInfo(float x, float y) { | ||
| int charIndex = -1; | ||
| int insertionIndex = -1; |
There was a problem hiding this comment.
[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?
There was a problem hiding this comment.
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.
I think it should be possible to initialize insertionIndex always in |
|
Just by looking at the code, the code paths that leave If we can initialize insertionIndex to a positive value, then the buggy code in HitInfo.getInsertionIndex() will never get executed, and we can remove it later (in JDK-8302511). |
Updated the code to initialize |
| if (insertionIndex == -1) { | ||
| insertionIndex = charIndex; | ||
| if (!leading) { | ||
| insertionIndex += 1; |
There was a problem hiding this comment.
I would have initialized insertionIndex directly in all the blocks instead of adding conditional logic, but this code does seem to produce the desired result.
For clarify, would it be possible to avoid this additional conditional logic?
Also, do existing unit tests cover the newly modified paths?
There was a problem hiding this comment.
Initializing the insertion index directly with character index looks better. In this way we can avoid additional conditional logic as well. Changed code to do so.
Added additional test case. This tests the condition present in line 430.
I couldn't create the scenario to hit line 473. If you have any suggestions please let me know.
While testing I found that testTextFlowInsertionIndexUsingMultipleEmojis() was failing intermittently when all the tests are run. Hence added small delay.
There was a problem hiding this comment.
I couldn't create the scenario to hit line 473. If you have any suggestions please let me know.
I can't seem to trigger this code path (I wonder if it's possible to hit it at all). Anyway, the code there looks correct to me, even if this condition would never happen.
There was a problem hiding this comment.
Assuming line 473 is the one still there today, it looks to me as if that would be reached if you had the caret on an empty line that isn't the last line.
There was a problem hiding this comment.
Regarding
"If we can initialize insertionIndex to a positive value, then the buggy code in HitInfo.getInsertionIndex()
will never get executed, and we can remove it later (in JDK-8302511)."
it seems to me that this new code has been pretty much copied from the supposedly buggy code there ..
I'm not sure that code is actually buggy (comments over in that bug), rather that the instance was constructed
with bad data.
But maybe this code here also needs to make sure it won't cause AIOB
There was a problem hiding this comment.
The exception in 8302511 was likely caused by a prior exception due to null text, which corrupted insertionIndex value in HitInfo.
The goal of this change is to always compute insertionIndex (an I believe we do it correctly this time).
There was a problem hiding this comment.
No, it wasn't due to null text. That is provably impossible.
The exception from JDK-8302511 is as follows (copied from the bug report).
Exception in thread "JavaFX Application Thread" java.lang.IllegalArgumentException: offset out of bounds
at java.base/sun.text.RuleBasedBreakIterator.checkOffset(RuleBasedBreakIterator.java:730)
at java.base/sun.text.RuleBasedBreakIterator.following(RuleBasedBreakIterator.java:744)
at javafx.graphics/javafx.scene.text.HitInfo.getInsertionIndex(HitInfo.java:84)
at javafx.graphics/javafx.scene.text.HitInfo.toString(HitInfo.java:100)
Look at the code in HitInfo.java referenced in JDK-8302511, the code at line 84 can only be reached if text != null
..
if (text != null) {
// Skip complex character clusters / ligatures.
int next;
synchronized(charIterator) {
charIterator.setText(text);
next = charIterator.following(insertionIndex); // this is line 84
}
...
There was a problem hiding this comment.
you are right, it cannot be explained by a null text.
so if i understand you correctly, the question is about validity of insertionIndex += 1 on lines 463 and 469.
463: should it be set to text.length() instead?
469: since text==null, could insertionIndex = charIndex + 1 be correct?
And also, is it possible at all that text == null within PrismTextLayout?
There was a problem hiding this comment.
No, because that's after the call to BreakIterator.following(int)
It is mainly about the assignment on line 456
insertionIndex = charIndex;
since that is what gets passed to BreakIterator.following(int)
and charIndex was calculated by
charIndex = run.getStart() + run.getOffsetAtX(x, trailing);
so it goes back to that calculation.
Perhaps a simple validation that charIndex is not out of bounds is all that is needed.
There was a problem hiding this comment.
Added check to make sure that insertion index passed to BreakIterator.following(int) is not out of bound.
Added this check after line 456 insertionIndex = charIndex;, otherwise insertionIndex remains as -1 and Hitinfo::getInsertionIndex will calculate wrong insertionIndex.
Please check the change.
|
Karthik, we are very close! Also, I noticed that the tests do not cover a case of line wrapping - would it be possible to add a similar test where the text string is wrapped, so we can verify that monotonic test works across the wrap point please? |
|
Sure. I will check if I can add new test case with wrapped text. |
|
Added new test to validate character index and insertion index in wrapped text. @andy-goryachev-oracle , above failure is caused because test is asserting before the mouse event handler could retrive hitinfo values. So added small delays in all the tests to make sure that the text is rendered and test becomes stable before getting the hitinfo values and assertion. Please let me know if you are able to run all the tests. |
|
Almost good! It looks like there might be some timing issue still lingering in one test: the very first run failed with Subsequent runs were ok. |
|
This looks like timing issue only. I believe we have to increase the delays little bit more. |
openjdk
bot
removed
the
merge-conflict
Removed red heart from the text sequence.
Added above suggested method in Util and tests looks to be stable now. Currently waiting for 10 pulses as suggested. Added parameterized method to specify the number of pulses to be waited as well. Also removed the unnecessary latches as I was already using |
andy-goryachev-oracle
left a comment
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
verified on macOS, Windows 11. looks good!
|
@karthikpandelu This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 24 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@andy-goryachev-oracle Do you think this needs a second reviewer, or are you satisfied that a single review is sufficient? |
|
I don't mind someone else take a look at this, @kevinrushforth . |
|
@prrace Would you be willing to take a look? /reviewers 2 |
|
@kevinrushforth |
|
Btw, I took a look at the new Can you file a new (P4) testbug as a follow-up issue to use it replace the two existing uses of |
|
what do you people think of the number of pulses parameter? is 10 enough? is there a reasonable upper limit that's needed for any processing to settle? another question is whether it could be made static - i.e. not require a Scene. Perhaps we could add a pulse listener to all visible Scenes? Would that be sufficient, since we only need a signal from one to indicate the pulse has been performed, and there is another and another (... up to 10 or N) pulses that basically guarantees that things are settled. And if the layout has not settled by that time, we have a bigger issue (can we detect this? I've seen continuous layout loops in one of my programs) |
I think 10 should be a reasonable number of pulses for our tests. Most graphs settle down within 3 layout passes.
I doubt this is worth it. This is a test utility, and if one of the very few tests that have multiple scenes needed to call it, it wouldn't be hard to have the test call it for each scene (and would give the test more control).
Right. This is tangential to this PR, but at some point we might want to resurrect the discussion on PR #433 to see if we can improve the current situation. |
Created JDK-8308608 |
|
The new code also exhibits a strange behavior in the RichTextArea (based ob TextFlow): I am unable to position the cursor at the end of ☝🏿☝🏿☝🏿 emoji sequence with the mouse (unless I click on the next empty line). |
|
|
||
| private void addMultipleEmojis() { | ||
| Util.runAndWait(() -> { | ||
| text = new Text("😊😇💙🦋🏁🔥"); |
There was a problem hiding this comment.
Both new code (the last commit) and the old code (previous commit) fail this test if the text string is the following sequence:
☝🏿☝🏿☝🏿
\u261d\ud83c\udfff\u261d\ud83c\udfff\u261d\ud83c\udfff
I've tried it with jdk19 and jdk20.0.1 (the latter contains a BreakInterator change JDK-8291660 that alters its handling of grapheme clusters.
Exception:
java.lang.AssertionError: expected:<0> but was:<1>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.junit.Assert.assertEquals(Assert.java:633)
at test.robot.javafx.scene.TextFlowSurrogatePairInsertionIndexTest.testTextFlowInsertionIndexUsingMultipleEmojis(TextFlowSurrogatePairInsertionIndexTest.java:216)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:578)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
at org.junit.vintage.engine.execution.RunnerExecutor.execute(RunnerExecutor.java:42)
at org.junit.vintage.engine.VintageTestEngine.executeAllChildren(VintageTestEngine.java:80)
at org.junit.vintage.engine.VintageTestEngine.execute(VintageTestEngine.java:72)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:95)
at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:91)
at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:60)
at org.eclipse.jdt.internal.junit5.runner.JUnit5TestReference.run(JUnit5TestReference.java:98)
at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
There was a problem hiding this comment.
This case is same as the one which we saw with red heart as the emoji sequence mentioned here is also a U+FE0F Variation Selector-16 character. Since it exhibits different behavior in different platforms we decided to remove it.
I compared the insertion index calculated for these emojis with the insertion index calculated for red heart emoji. The insertion index calculated looks to be correct for the above given emojis. It is considered as 2 characters if color is also present else it is considered as single character.
For me it is giving correct caret position as well in the Monkey tester.
I found strange behavior when I tried to add heart emojis (with and without color) along with the CLUSTERS variable present in Monkey Tester so that it gets displayed when Rich Text option is selected.
When CLUSTERS = "❤️❤❤️", emojis are displayed properly.
When CLUSTERS = "☝🏿☝🏿☝🏿🤦🏼♂️❤️❤❤️";, following sequence is displayed.
This looks like a separate issue. Please let me know your thoughts on this.
There was a problem hiding this comment.
- you are right, multiple symbols and a failed test are due to lack of proper handling of grapheme clusters in fx. I don't think we have a ticket, so created JDK-8309565 for that
- your latest fix uncovered a problem in my RichTextArea code, so thank you!
The new code passes all my tests in the MonkeyTester and RichTextArea.
There was a problem hiding this comment.
Than you @andy-goryachev-oracle for the detailed review and feedback.
|
FYI, I've added "Enter Text" button to Text/TextFlow pages in the Monkey Tester it might be easier to test various emojis and text strings with it. |
|
Thanks @andy-goryachev-oracle. I will check above mentioned issue. |
|
/integrate |
|
Going to push as commit c20f6d0.
Your commit was automatically rebased without conflicts. |
|
@karthikpandelu Pushed as commit c20f6d0. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
5 participants
Since surrogate pairs are internally considered as 2 characters and text field is null in
HitInfowhengetInsertionIndexis invoked fromTextFlow, wrong insertion index was returned.Updated code to calculate insertion index in
getHitInfomethod ofPrismTextLayoutclass whenhitTestof trailing side of surrogate pair is requested. Since text runs are processed in this method already, calculating the insertion index in this method looks better than calculating ingetInsertionIndexofHitInfomethod.The latter approach also requires the text to be sent to
HitInfoas parameter from thehitTestmethod ofTextFlow. If the number ofTextnodes inTextFloware very large, processing all theTextnodes on eachhitTestmethod invocation might cause performance issue. Hence implemented first approach.Added system test to validate the fix.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1091/head:pull/1091$ git checkout pull/1091Update a local copy of the PR:
$ git checkout pull/1091$ git pull https://git.openjdk.org/jfx.git pull/1091/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1091View PR using the GUI difftool:
$ git pr show -t 1091Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1091.diff
Webrev
Link to Webrev Comment