Ensure "text boxes" preserve cursor location #15099

mattleibow · 2023-05-16T02:12:47Z

Description of Change

This PR adds some tests to make sure all the platforms are consistent with regards to how the cursor position changes (or not) when text is changed.

It is just 2 small changes:

make sure the cursor is not updated for the same text
moved the duplicate tests from editor/entry into a single base test for all things - editor, entry and searchbar

There is a code change for iOS and Windows to only update selection start/length if the text has changed:

if nothing has changed, don't move the cursor
if the text is the same, then we may still be initializing a binding process
doing less is always better for performance

mattleibow · 2023-05-17T06:27:55Z

src/Controls/tests/DeviceTests/Elements/TextInput/TextInputTests.cs

+		[Theory]
+		[InlineData(2)]
+		public async Task CursorPositionInitializesCorrectlyWithUpdateCursorPositionLast(int initialPosition)
+		{
+			var entry = new TView
+			{
+				Text = "This is TEXT!",
+				CursorPosition = initialPosition
+			};
+
+			var value = await GetValueAsync<int, THandler>(entry, handler =>
+			{
+				handler.UpdateValue(nameof(ITextInput.CursorPosition));
+				return GetPlatformCursorPosition(handler);
+			});
+
+			Assert.Equal(initialPosition, value);
+		}
+
+		[Theory]
+		[InlineData(2)]
+		public async Task CursorPositionInitializesCorrectlyWithUpdateTextLast(int initialPosition)
+		{
+			var entry = new TView
+			{
+				Text = "This is TEXT!",
+				CursorPosition = initialPosition
+			};
+
+			var value = await GetValueAsync<int, THandler>(entry, handler =>
+			{
+				handler.UpdateValue(nameof(ITextInput.Text));
+				return GetPlatformCursorPosition(handler);
+			});
+
+			Assert.Equal(initialPosition, value);
+		}


These 2 tests cause failures depending on the order of properties in the mapper table. Currently if Text is first, then it is passing, but if it happens to have Text after the CursorPosition (which will be the case in my other PRs because I am fixing the mappers) then it fails.

mattleibow · 2023-05-17T06:32:21Z

src/Controls/src/Core/Platform/Windows/Extensions/TextBoxExtensions.cs


-			platformControl.Select(cursorPosition, 0);
+				platformControl.Select(cursorPosition, 0);


This is the change for Windows.

mattleibow · 2023-05-17T06:32:33Z

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs

 				textView.Text = newText;

-			textView.SetTextRange(cursorPosition, 0);
+				textView.SetTextRange(cursorPosition, 0);


This is the change for iOS.

rmarinho · 2023-05-17T09:19:45Z

@mattleibow can you see if it helps with this issue. #11512 ?

mattleibow · 2023-05-17T13:57:27Z

@rmarinho this does not seem to make any difference. I think your conclusions on the other issue are correct, so maybe we need to do less of something so the code that runs in the events actually stick.

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs

rmarinho · 2023-05-31T00:03:38Z

/rebase

This reverts commit 328c6ba. # Conflicts: # src/Controls/tests/DeviceTests/Elements/TextInput/TextInputTests.cs

This reverts commit d62b12a.

This reverts commit 6065a41.

rachelkang

UPDATE: After some offline discussion, we learned this PR isn't exactly meant to modify the behavior I annotated below, although it is still useful context to have.

I tested some of the different scenarios on Windows and Android via keyboard tab / shift+tab navigation. Here's what I'm seeing on this PR branch at this point in time:

Scenario	Behavior
1. Entry/Editor blank with text updated after	Cursor position remains wherever it was left last. The platform "remembers" wherever it is, even if the cursor is in the middle of a word.
2a. Entry/Editor with pre-filled text	Cursor position remains at the beginning of the input field.
2b. 2a with text updated after	Same as 1
3a. Entry/Editor with pre-filled text via binding	Same as 2a
3b. 3a with text updated after	Same as 1

Interestingly, the behavior on Windows and Android was exactly the same for all of the scenarios. Shift+Tab didn't appear to work on Android emulator for me though (unclear why, as I do think it was working before and/or on device, but anyhow, that's a separate issue).

This behavior is also exactly the same on main, so it doesn't seem that keyboard focus was affected by the work you've done so far on this PR.

Based on what I'm seeing here, my thinking is that we should update so that for scenarios 2a and 3a, cursor position is at the END of the text in input field.

rachelkang

LGTM on Windows!

need to test on iOS still, and left a couple comments

src/Controls/tests/DeviceTests/ControlsHandlerTestBase.cs

src/Controls/tests/DeviceTests/Elements/TextInput/TextInputTests.cs

Co-authored-by: Rachel Kang <rachelkang@microsoft.com>

hartez

Looks good, couple of minor change requests.

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs

rachelkang · 2023-07-06T15:58:58Z

UPDATE: After some offline discussion, we learned this PR isn't exactly meant to modify the behavior I annotated below, although it is still useful context to have.

I tested some of the different scenarios on Windows and Android via keyboard tab / shift+tab navigation. Here's what I'm seeing on this PR branch at this point in time:

Scenario Behavior

Entry/Editor blank with text updated after Cursor position remains wherever it was left last. The platform "remembers" wherever it is, even if the cursor is in the middle of a word.
2a. Entry/Editor with pre-filled text Cursor position remains at the beginning of the input field.
2b. 2a with text updated after Same as 1
3a. Entry/Editor with pre-filled text via binding Same as 2a
3b. 3a with text updated after Same as 1
Interestingly, the behavior on Windows and Android was exactly the same for all of the scenarios. Shift+Tab didn't appear to work on Android emulator for me though (unclear why, as I do think it was working before and/or on device, but anyhow, that's a separate issue).

This behavior is also exactly the same on main, so it doesn't seem that keyboard focus was affected by the work you've done so far on this PR.

Based on what I'm seeing here, my thinking is that we should update so that for scenarios 2a and 3a, cursor position is at the END of the text in input field.

Updating table to record my findings on macios:

Scenario	WinUI/Android	Mac	iOS
1. Entry/Editor blank with text updated after	Cursor position remains wherever it was left last. The platform "remembers" wherever it is, even if the cursor is in the middle of a word.	same as WinUI/Android, but Keyboard tab nav doesn't work unless VO is on or keyboard navigation setting is enabled, and tabbing away from an Editor doesn't seem feasible? (creates a tab space in the actual input)	With VO+double tap, cursor always jumps to the end? Double tapping toggles between cursor beginning and end position. With keyboard, position is always at the end and doesn't remember changes for Entry, but position starts at the beginning and then remembers changes for Editor
2a. Entry/Editor with pre-filled text	Cursor position remains at the beginning of the input field.	same as WinUI/Android	same as 1
2b. 2a with text updated after	Same as 1	Same as 1	Same as 1
3a. Entry/Editor with pre-filled text via binding	Same as 2a	not tested	not tested
3b. 3a with text updated after	Same as 1	not tested	not tested

rachelkang

Tested on mac, iOS as well and LGTM!

(There are some behavior quirks, but no new quirks. We should definitely investigate more in the future, but that's outside the scope of what this PR aims to do).

Left a nitpick comment on a var name, but approving as all else LGTM!

rachelkang · 2023-07-06T16:04:42Z

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs

-				textField.Text = newText;
+			{
+				// Re-calculate the cursor offset position if the text was modified by a Converter.
+				// but if the text is being set by code, let's just move the cursor to the end.


We should reevaluate this behavior in the future as it's inconsistent with Windows and Android. And we should check to see if it's consistent with iOS platform behavior, as it seems like it might not be. Could you log an issue to capture this, or update existing related issue?

rachelkang · 2023-07-06T16:07:51Z

src/Controls/tests/DeviceTests/ControlsHandlerTestBase.cs

+			where TType : IView, new()
+			where THandler : class, IPlatformViewHandler, IElementHandler, new()
+		{
+			Func<THandler, Task> boop = (handler) =>


nitpick: can we rename boop to be more descriptive :p

Eilon added the area-controls-entry Entry label May 16, 2023

ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 16, 2023

mattleibow force-pushed the dev/ios-text branch from edd8537 to 8ca70af Compare May 16, 2023 04:15

mattleibow changed the title ~~Ensure iOS "text boxes" preserve cursor location~~ Ensure "text boxes" preserve cursor location May 16, 2023

mattleibow commented May 17, 2023

View reviewed changes

mattleibow requested a review from PureWeen May 17, 2023 06:34

This was referenced May 17, 2023

Update mappers instead of replacing field values #13836

Merged

Move most controls to use ModifyMapping (and variants) #15058

Closed

PureWeen reviewed May 24, 2023

View reviewed changes

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs Outdated Show resolved Hide resolved

mattleibow added 14 commits May 31, 2023 00:04

Ensure iOS "text boxes" preserve cursor location

26bcea2

Fix the Windows tests

b38a0d6

Attach the controls to the UI for focus tests

8dc936b

Adding some more "text box" tests

4a2f911

todo

5e870ad

tests

7cdc2f8

Revert "todo"

e3a4747

This reverts commit 328c6ba. # Conflicts: # src/Controls/tests/DeviceTests/Elements/TextInput/TextInputTests.cs

this

70bc6c0

move

68dc760

Revert "this"

c93b474

This reverts commit d62b12a.

moar tests

9829831

Revert "Revert "todo""

7e4605e

This reverts commit 6065a41.

all working on windows

3257130

Merge the methods

4192bd5

github-actions bot force-pushed the dev/ios-text branch from d797992 to 4192bd5 Compare May 31, 2023 00:04

mattleibow mentioned this pull request Jun 6, 2023

A11y_.NETCore_RecipesAppMyRecipesEdit_EditRecipe_Screen Reader: Talkback focus is not moving correctly when we try to edit in "Ingredients" and "Recipe". #15226

Closed

Merge remote-tracking branch 'origin/main' into dev/ios-text

a2730d1

rachelkang self-requested a review June 8, 2023 15:02

rachelkang requested changes Jun 8, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/ios-text

21ce0d3

rachelkang requested changes Jun 27, 2023

View reviewed changes

mattleibow and others added 3 commits July 6, 2023 00:49

Update src/Controls/tests/DeviceTests/ControlsHandlerTestBase.cs

260255f

Co-authored-by: Rachel Kang <rachelkang@microsoft.com>

fix the variable names

2e578a8

fix

1868019

hartez suggested changes Jul 5, 2023

View reviewed changes

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs Outdated Show resolved Hide resolved

src/Controls/src/Core/Platform/iOS/Extensions/TextExtensions.cs Outdated Show resolved Hide resolved

mattleibow added 2 commits July 5, 2023 23:15

Merge remote-tracking branch 'origin/main' into dev/ios-text

822f3e5

feedback

71b850f

mattleibow requested review from rachelkang, hartez and PureWeen July 6, 2023 08:11

rachelkang approved these changes Jul 6, 2023

View reviewed changes

hartez enabled auto-merge (squash) July 6, 2023 18:30

hartez approved these changes Jul 6, 2023

View reviewed changes

hartez merged commit 0504ca2 into main Jul 6, 2023

hartez deleted the dev/ios-text branch July 6, 2023 18:30

mattleibow mentioned this pull request Jul 10, 2023

Use AppendToMapping for Element and VisualElement #14948

Closed

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024

samhouts added the fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842! label Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure "text boxes" preserve cursor location #15099

Ensure "text boxes" preserve cursor location #15099

mattleibow commented May 16, 2023 •

edited

Loading

mattleibow May 17, 2023

mattleibow May 17, 2023

mattleibow May 17, 2023

rmarinho commented May 17, 2023

mattleibow commented May 17, 2023

rmarinho commented May 31, 2023

rachelkang left a comment •

edited

Loading

rachelkang left a comment

hartez left a comment

rachelkang commented Jul 6, 2023 •

edited

Loading

rachelkang left a comment •

edited

Loading

rachelkang Jul 6, 2023

rachelkang Jul 6, 2023


		platformControl.Select(cursorPosition, 0);
		platformControl.Select(cursorPosition, 0);

Ensure "text boxes" preserve cursor location #15099

Ensure "text boxes" preserve cursor location #15099

Conversation

mattleibow commented May 16, 2023 • edited Loading

Description of Change

mattleibow May 17, 2023

Choose a reason for hiding this comment

mattleibow May 17, 2023

Choose a reason for hiding this comment

mattleibow May 17, 2023

Choose a reason for hiding this comment

rmarinho commented May 17, 2023

mattleibow commented May 17, 2023

rmarinho commented May 31, 2023

rachelkang left a comment • edited Loading

Choose a reason for hiding this comment

rachelkang left a comment

Choose a reason for hiding this comment

hartez left a comment

Choose a reason for hiding this comment

rachelkang commented Jul 6, 2023 • edited Loading

rachelkang left a comment • edited Loading

Choose a reason for hiding this comment

rachelkang Jul 6, 2023

Choose a reason for hiding this comment

rachelkang Jul 6, 2023

Choose a reason for hiding this comment

mattleibow commented May 16, 2023 •

edited

Loading

rachelkang left a comment •

edited

Loading

rachelkang commented Jul 6, 2023 •

edited

Loading

rachelkang left a comment •

edited

Loading