Normalize start Text pointer to support Text Cursor Indicator

[harshit7962]

Description

Using accessibility option of text cursor indicator in RichTextBox sometimes lead to crashing applications due to Invariant assert here. Adding Normalize() call in changed location moves the end to the start if this invariant assert fails and hence essentially corrects the cursor position.

Customer Impact

Accessibility fix. Allowing the usage of text cursor indicator with RichTextBox.

Regression

None

Testing

Local Build Pass
Sample Application Testing
Test Pass

Risk

Low

Microsoft Reviewers: Open in CodeFlow

[miloush]

I see change on lines 1813-15. So you get the bounding rectangle, then potentially change it and then return the previous rectangle values? Doesn't sound like the intended API experience.

I also don't see how changing the order of Normalize and GetBoundingRectangle in this method changes the invariant after exiting the method. Even if the adapter called back for a clone, it would be after Normalize before the change, so putting Normalize at the beginning of Clone should have not fixed it.

Can you share some of the investigation on what breaks the invariant or when?

[h3xds1nz]

I agree with miloush. I'm a bit puzzled. Sharing the investigation would be great to understand this properly.

So you get the bounding rectangle, then potentially change it and then return the previous rectangle values? Doesn't sound like the intended API experience.

And this exactly concerns me.

[harshit7962]

So the GetBoudingRectangles eventually leads the call to NormalizePosition method of TextPointerBase, where based on the LogicalDirection, the start pointer deviates from its expected value.

Flow:

• TextRangeAdaptor -> ITextRangeProvider.GetBoundingRectangles
• This calls GetBoundingRectangles of TextAdaptor
• which calls MoveToInsertionPosition of TextRangeAdaptor from here
• This calls the MoveToInsertionPosition of TextPointer which again calls MoveToInsertionPosition of TextPointerBase
• This is where NormalizePosition is called and our invariance breaks.

[harshit7962]

Building upon the above comment, basically the idea of the change is that GetBoundingRectangles in TextRangeAdaptor uses a start textpointer and an end textpointer, during the calculation of which it changes the start textpointer causing the invariance. After the calculation is done with, we Normalize the start pointer back to what the original value was.

How do we certain that the start is going back to its original value post Normalize:
The MoveToInsertionPosition which causes the change in GetBoundingRectangles, is also called in Normalize with LogicalDirection as reverse of what it was earlier.

cc:/ @miloush, @h3xds1nz

[h3xds1nz]

@harshit7962 sorry, somehow managed to miss this.

From a brief overview it makes sense with your explanation but I'm still not sure whether that's the correct solution; imho it shouldn't happen in the first place. Do we have a repro? I'd just like to step through.

[harshit7962]

Thanks for willing to take a look into it, for the repro, use the following xaml

<RichTextBox> 
    <FlowDocument> 
        <Paragraph> 
            <Bold>a</Bold>
            <Bold>b</Bold> 
        </Paragraph> 
   </FlowDocument> 
</RichTextBox> 

Turn on the text cursor indicator from settings. Settings -> Accessibility -> Text Cursor
Launch the application and move the indicator between a and b, it should crash the application.

[harshit7962]

@h3xds1nz, @miloush - If there are no more concerns, we are planning on moving forward with this.

[h3xds1nz]

@harshit7962 Apologies, didn't have the bandwidth yet, I'll take a look today/tomorrow max. to no longer delay this.

[miloush]

checking now

[miloush]

@harshit7962 I can repro on .NET 9 but not on .NET Framework, is that expected?

[h3xds1nz]

@miloush #10281 (review)

[harshit7962]

@miloush so in the framework code, the Clone method does the normalization as we tried in #10281 (As pointed out by @h3xds1nz). But as was discussed earlier, this probably is not the best solution and hence we tried the current change.

But yeah, that change is not available since .NET core, and hence you would be able to repro it in any of the core versions.

[h3xds1nz]

@harshit7962 Alright, I can see that calling Normalize (which I'd just move into the TextRangeAdaptor ctor by the way, therefore the assert can just be in DEBUG, so we know something is wrong there - if we're looking for a workaround, so its essentially same as the original "fix" but we just always perform normalization on construction) workarounds the issue but doesn't solve it.

However I'm not keen on spending more time on stuff I don't particularly understand right now (never worked with the internal document nodes - I have no idea why the end contains a Run (within TextTreeTextElementNode) and the difference between start's TextTreeTextNode which doesn't contain TextElement when this assert crumbles). My knowledge around FlowDocument is limited in general. Would require following the node assigment/search process closely.

Hopefully miloush knows more.

[miloush]

OK I managed to have a look.

This is the Normalize() method:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/TextRangeAdaptor.cs

Lines 1489 to 1499 in d411234

	private void Normalize()
	{
	MoveToInsertionPosition(_start, _start.LogicalDirection);
	MoveToInsertionPosition(_end, _end.LogicalDirection);
	// If start passes end, move the entire range to the start position.
	if (_start.CompareTo(_end) > 0)
	{
	_end.MoveToPosition(_start);
	}
	}

MoveToInsertionPosition() is the bit that manipulates the start/end separately and can potentially break the invariant. Therefore, after that is done, you need to make sure that end >= start.

Now in the context of this scenario, the only point where MoveToInsertionPosition() is called outside of Normalize is in GetBoundingRectangles(), before the actual rectangles are calculated:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/TextAdaptor.cs

Lines 115 to 116 in d411234

	TextRangeAdaptor.MoveToInsertionPosition(start, LogicalDirection.Forward);
	TextRangeAdaptor.MoveToInsertionPosition(end, LogicalDirection.Backward);

So I suggest we enforce the invariant after these two lines that can break it, just like in Normalize():

if (start.CompareTo(end) > 0)
{
    end.MoveToPosition(start);
}

[harshit7962]

@miloush thanks for the suggestion. A quick couple of things though

1. shouldn't it be start.MoveToPosition(end) given that (in this case) its the start pointer that has deviated from what was provided by the parent call. Also, this is what we were kind of achieving via the Normalize call.
2. I am a bit concerned about the Rect[] being returned by GetBoudingRectangles given that we change the TextPointer kind of before the actual calculations/logic. So to put it more clearly, earlier in TextRangeAdaptor we used to get the rects based on whateved the _start and _end was computed withing the function logic and now it will alter the start/end and then return the rects. This looks like a possible behavioral change.

Tbh I am still trying to reason the 2nd point out completely, so had just mentioned it here to get your inputs.

[miloush]

The issue we had with the solution in this PR is that you calculated the rectangles first and then you moved around the pointers, so the caller would have rectangles that do not correspond to the pointers they have. It is quite important aspect of the situation that the caller and callee are different classes, the ownership of the pointers is critical to the bug.

It is a bit evil that the method modifiers the pointers it receives. The solution is to establish a convention. Either the GetBoundingRectangles owns the pointers and is allowed to do what it wants with them, in which case it should be mentioned in the comments and if the caller has requirements on them such as upholding an invariant, it needs to deal with them being changed or call the method with a copy, or, we say that the caller owns the pointers and the method cannot touch them. In such case, the method should make a copy for its own use if it needs to change them.

You are correct, the GetBoundingRectangles method itself does not need the invariant to hold, it doesn't know the two positions are part of a range. It is a better solution and separation of concerns to keep the ownership with the caller. Therefore a better solution than I suggested would be to add

start = start.CreatePointer();
end = end.CreatePointer();

before

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/TextAdaptor.cs

Lines 115 to 116 in d411234

	TextRangeAdaptor.MoveToInsertionPosition(start, LogicalDirection.Forward);
	TextRangeAdaptor.MoveToInsertionPosition(end, LogicalDirection.Backward);

(if you want you can optimize it a bit because in some cases you are already creating copies above)

This also voids your first question.