Skip to content
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.

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

Fixed Keyboard Scrolling in editors with Center or End VerticalTextAlignment #25827

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

NirmalKumarYuvaraj
Copy link
Contributor

@NirmalKumarYuvaraj NirmalKumarYuvaraj commented Nov 13, 2024

Issue Details

Sometimes, the editor cursor moves below the keyboard frame, causing it to render incorrectly. Once the editor content exceeds the editor's size, the cursor aligns properly above the keyboard frame.

Root Cause

The content offset of UITextView is set without consideration of keyboard frame. Causing the content to remain center even when the keyboard is open. As a result, the cursor goes below the keyboard frame.

Description of Change

The content offset of UITextView should be recalculated based on the keyboard frame value . We should check if the cursor's bottom position goes below the keyboard; we have to update the y position of the content offset based on the difference between the cursor's bottom position and the keyboard frame's top position. 

Validated the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Issues Fixed

Fixes #24977

Output

Before After
Before.mov
After.mov

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 13, 2024
@jsuarezruiz jsuarezruiz added area-keyboard Keyboard, soft keyboard area-controls-editor Editor labels Nov 13, 2024
@NirmalKumarYuvaraj NirmalKumarYuvaraj marked this pull request as ready for review November 14, 2024 09:25
@NirmalKumarYuvaraj NirmalKumarYuvaraj requested a review from a team as a code owner November 14, 2024 09:25
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/azp run

@dotnet dotnet deleted a comment from jsuarezruiz Nov 18, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 18, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added this to the .NET 9 SR1.1 milestone Nov 18, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen modified the milestones: .NET 9 SR1.1, .NET 9 SR2 Nov 20, 2024
@tj-devel709
Copy link
Member

Looks like this changed the behavior of the placeholder a tad. Looks like it is moved down a little and is being cut off a little bit on the top

Original is on the left, the diff is in the middle, and the new behavior from this PR is on the right

image

Comment on lines 164 to +174
{
var contentHeight = ContentSize.Height;
var contentHeight = ContentSize.Height - TextContainerInset.Top - TextContainerInset.Bottom;
var availableSpace = Bounds.Height - contentHeight * ZoomScale;
if (availableSpace <= 0)
return;
ContentOffset = VerticalTextAlignment switch

TextContainerInset = VerticalTextAlignment switch
{
Maui.TextAlignment.Center => new CGPoint(0, -Math.Max(1, availableSpace / 2)),
Maui.TextAlignment.End => new CGPoint(0, -Math.Max(1, availableSpace)),
_ => ContentOffset,
Maui.TextAlignment.Center => new UIEdgeInsets((nfloat)Math.Max(1, availableSpace / 2), TextContainerInset.Left, TextContainerInset.Bottom, TextContainerInset.Right),
Maui.TextAlignment.End => new UIEdgeInsets((nfloat)Math.Max(1, availableSpace), TextContainerInset.Left, TextContainerInset.Bottom, TextContainerInset.Right),
_ => TextContainerInset,
Copy link
Member

Choose a reason for hiding this comment

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

So it looks like the failing tests aren't just from the placeholder, but the spacing is just not correct and not accounting for the bottom of the TextContainerInset correctly. The following gets us closer and seems like the middle adjustment is a rounding issue perhaps. Can you look into this?

		void ShouldCenterVertically()
		{
			// ContentSize.Height rounds to the nearest whole number so to keep the measurements consistent we round the adjustment as well
			var contentHeight = ContentSize.Height - Math.Round(TextContainerInset.Top, MidpointRounding.AwayFromZero);;
			var availableSpace = Bounds.Height - contentHeight * ZoomScale;
			if (availableSpace <= 0)
				return;

			TextContainerInset = VerticalTextAlignment switch
			{
				Maui.TextAlignment.Center => new UIEdgeInsets((nfloat)Math.Max(1, availableSpace / 2), TextContainerInset.Left, TextContainerInset.Bottom, TextContainerInset.Right),
				Maui.TextAlignment.End => new UIEdgeInsets((nfloat)Math.Max(1, availableSpace), TextContainerInset.Left, TextContainerInset.Bottom, TextContainerInset.Right),
				_ => TextContainerInset,
			};
		}

Copy link
Member

Choose a reason for hiding this comment

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

This is the new diff from the code above

diff5

Copy link
Contributor

Choose a reason for hiding this comment

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

There are more tests that would require a similar change
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsuarezruiz , EditorPlaceholderPosition is the test case related to this PR. The other test cases are unrelated, as this PR contains code changes for UITextView, and the remaining test cases do not involve an editor in their scenarios. To confirm, I ran both test cases locally, and they passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tj-devel709 , The rounding approach for middle alignment does not resolve the issue. I attempted to recalculate the position when the text alignment is set to center. Using this approach, we obtained the correct value, which is 66.25. This value is required for the test case to pass; however, the test still fails, even though the top inset value is set to 66.25.

void ShouldCenterVertically()
{
	// ContentSize.Height rounds to the nearest whole number so to keep the measurements consistent we round the adjustment as well
	var contentHeight = ContentSize.Height - Math.Round(TextContainerInset.Top, MidpointRounding.AwayFromZero);
	var availableSpace = Bounds.Height - contentHeight * ZoomScale;
	if (availableSpace <= 0)
		return;

	TextContainerInset = VerticalTextAlignment switch
	{
		Maui.TextAlignment.Center => new UIEdgeInsets((nfloat)Math.Max(1,(Bounds.Height - (ContentSize.Height - (TextContainerInset.Top + TextContainerInset.Bottom)) * ZoomScale) / 2), TextContainerInset.Left, TextContainerInset.Bottom, TextContainerInset.Right),
		Maui.TextAlignment.End => new UIEdgeInsets((nfloat)Math.Max(1, availableSpace), TextContainerInset.Left, TextContainerInset.Bottom, TextContainerInset.Right),
		_ => TextContainerInset,
	};
}

New diff from the above code

Copy link
Member

Choose a reason for hiding this comment

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

Huh.. that looks pretty close so I'm not sure what is off here. It looks like maybe its just off by a fraction of a pixel or something like that. @PureWeen, what do you think? Good enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

@PureWeen, can you please share your thoughts on this?

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

Failing tests:
image

@NirmalKumarYuvaraj NirmalKumarYuvaraj added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
@PureWeen PureWeen self-assigned this Dec 6, 2024
@PureWeen PureWeen modified the milestones: .NET 9 SR2, .NET 9 SR3 Dec 6, 2024
@PureWeen
Copy link
Member

/rebase

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Thoughts on this difference?

If I comment out the code setting the TextContainerInset you get the typical iOS bounce when scrolling

Commenting out code that sets TextContainerInset

WithBounce.mov

This PR

Wobbly.mov

@tj-devel709
Copy link
Member

Thoughts on this difference?

If I comment out the code setting the TextContainerInset you get the typical iOS bounce when scrolling

Commenting out code that sets TextContainerInset

WithBounce.mov

This PR

Wobbly.mov

Yeah that is a weird behavior... Not sure what to immediately make of it other than the first behavior seems like the expected

@NirmalKumarYuvaraj NirmalKumarYuvaraj changed the title Fixed Keyboard Scrolling in editors with Center or End VerticalTextAlignment is off Fixed Keyboard Scrolling in editors with Center or End VerticalTextAlignment Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-editor Editor area-keyboard Keyboard, soft keyboard community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

Keyboard Scrolling in editors with Center or End VerticalTextAlignment is off
6 participants