RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v4]

[Phil Race]

On Fri, 2 Jun 2023 19:41:04 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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
>>                     }
>> 
>> ...
>
> 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?

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.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1214832843