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

TokenizingTextBox Text property cannot be set manually from code behind in Release mode #4749

Closed
4 of 14 tasks
Dieveral opened this issue Sep 1, 2022 · 10 comments · Fixed by #4786
Closed
4 of 14 tasks
Assignees
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 controls 🎛️
Milestone

Comments

@Dieveral
Copy link

Dieveral commented Sep 1, 2022

Describe the bug

In Release mode TokenizingTextBox Text property cannot be set from code behind or with x:Bind. In Debug mode everything works as expected.

Regression

No response

Reproducible in sample app?

  • This bug can be reproduced in the sample app.

Steps to reproduce

Link to sample app:
https://github.com/Dieveral/TokenizingTextBox_TextBug

Steps:
1. Start application in Debug mood
2. Write text in field
3. Leave field without transforming text to TokenItem
4. Press 'Clear' button.
Text disappears.
5. Exit application
6. Start application in Release mood
7. Repeat steps 2-4
Text remains.

Expected behavior

You can change Text property of TokenizingTextBox from code behind and with binding system in Release mood, as in Debug mood.
In sample app: Text must disappear after pressing 'Clear' button in Release mood.

Screenshots

TokenizingTextBox
TokenizingTextBox_BackEnd

Windows Build Number

  • Windows 10 1809 (Build 17763)
  • Windows 10 1903 (Build 18362)
  • Windows 10 1909 (Build 18363)
  • Windows 10 2004 (Build 19041)
  • Windows 10 20H2 (Build 19042)
  • Windows 10 21H1 (Build 19043)
  • Windows 11 21H2 (Build 22000)
  • Other (specify)

Other Windows Build number

Windows 10 21H2 (Build 19044.1889)

App minimum and target SDK version

  • Windows 10, version 1809 (Build 17763)
  • Windows 10, version 1903 (Build 18362)
  • Windows 10, version 1909 (Build 18363)
  • Windows 10, version 2004 (Build 19041)
  • Other (specify)

Other SDK version

No response

Visual Studio Version

2019

Visual Studio Build Number

16.11.17

Device form factor

Desktop

Nuget packages

Microsoft.NETCore.UniversalWindowsPlatform v6.2.12
Microsoft.Toolkit.Uwp.UI.Controls v7.1.12

Additional context

No response

Help us help you

No.

@Dieveral Dieveral added the bug 🐛 An unexpected issue that highlights incorrect behavior label Sep 1, 2022
@ghost ghost added the needs triage 🔍 label Sep 1, 2022
@ghost
Copy link

ghost commented Sep 1, 2022

Hello Dieveral, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@michael-hawker michael-hawker added controls 🎛️ WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3. and removed needs triage 🔍 labels Sep 7, 2022
@michael-hawker michael-hawker added this to the 7.1.3 milestone Sep 7, 2022
@michael-hawker
Copy link
Member

@LalithaNadimpalli this could be a good one for you to pick up next. Maybe see if you can reproduce in a new UWP project first? At least we can see if it's a general issue or specific to WinUI 3?

@Dieveral what Windows App SDK version are you using in your app? (@Arlodotexe we probably want to add that to our issue form template too. (For both WinUI and WindowsAppSDK - do they support conditional questions based on other answers yet?))

@LalithaNadimpalli LalithaNadimpalli self-assigned this Sep 7, 2022
@LalithaNadimpalli
Copy link
Contributor

LalithaNadimpalli commented Sep 7, 2022

I am reproducing the app in UWP using TokenizingTextBox code(XAML and .cs) but I see the issue user pointing to when I run the simple sample app provided

@michael-hawker
Copy link
Member

Thanks @LalithaNadimpalli, want to attach your reproduction project here as well?

I built this version of the control, so let me know if you have any questions in digging into what might be happening here when the text isn't being set.

@Dieveral
Copy link
Author

Dieveral commented Sep 8, 2022

@michael-hawker I am using TokenizingTextBox control in UWP project.
Target: Universal Windows
Target version: Windows 10, version 2004 (Build 19041)
Minimum version: Windows 10, version 1809 (Build 17763)

@michael-hawker michael-hawker removed the WinUI 💠 Related to WinUI 3 Version or when paired with External can mean requires fix in WinUI 2/3. label Sep 8, 2022
@michael-hawker
Copy link
Member

Synced with Laltiha and we think we found the root cause.

We discovered that it looks like the Text property appears to update and is set correctly (you can see this same behavior directly in the sample app as well as the clear tokens button should clear the text as well):

image

Above the Current Edit is bound to the Text property. Then when you click clear:

image

You can see the Current Edit properly reflects that the Text is now empty.

Still not sure why this is actually working in Debug vs. Release. But basically we think it's the AutoSuggestBox component that's part of the template that's not properly getting updated to reflect the proper state of the Text value.

You can actually see this if you try and set the default text value to the control as well, like:

<controls:TokenizingTextBox Text="Some Text Here"/>

image

The Text property reflects that, but not the initial state of the AutoSuggestBox.


We'll want to do three things:

  1. Add 2 new test cases that show these two scenarios for setting the initial text and clearing and seeing the text of the inner AutoSuggestBox cleared here: https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/main/UnitTests/UnitTests.UWP/UI/Controls/Test_TokenizingTextBox_General.cs

  2. Update the initialization to get set the AutoSuggestBox text to the value of the token text here:

if (Content is ITokenStringContainer str && str.IsLast)
{
// Workaround for https://github.com/microsoft/microsoft-ui-xaml/issues/2568
if (Owner.QueryIcon is FontIconSource fis &&
fis.ReadLocalValue(FontIconSource.FontSizeProperty) == DependencyProperty.UnsetValue)
{
// This can be expensive, could we optimize?
// Also, this is changing the FontSize on the IconSource (which could be shared?)
fis.FontSize = Owner.TryFindResource("TokenizingTextBoxIconFontSize") as double? ?? 16;
}
var iconBinding = new Binding()
{
Source = Owner,
Path = new PropertyPath(nameof(Owner.QueryIcon)),
RelativeSource = new RelativeSource() { Mode = RelativeSourceMode.TemplatedParent }
};
var iconSourceElement = new IconSourceElement();
iconSourceElement.SetBinding(IconSourceElement.IconSourceProperty, iconBinding);
_autoSuggestBox.QueryIcon = iconSourceElement;
}

  1. Update the TextPropertyChanged event to call a method on the last token item to tell it to reflect the changed text:

private static void TextPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
if (d is TokenizingTextBox ttb && ttb._currentTextEdit != null)
{
ttb._currentTextEdit.Text = e.NewValue as string;
}
}

Example of getting the last token item from focus event here:

(ContainerFromIndex(Items.Count - 1) as TokenizingTextBoxItem).Focus(FocusState.Programmatic);

That should hopefully resolve all these scenarios for this component.

Potentially in the future we could see if we could do this better with some sort of binding like we setup for the QueryIcon, but not sure what ramifications that has on the other tokenizing logic, so for now this seems like the most straight-forward fix without impacting a lot of the other parts of the control.

@LalithaNadimpalli LalithaNadimpalli removed their assignment Sep 22, 2022
@michael-hawker michael-hawker self-assigned this Oct 6, 2022
@michael-hawker
Copy link
Member

michael-hawker commented Oct 11, 2022

Split up a test case and added two new ones:

image

Above is debug and we can see clearly the scenario not working for clearing the text.

And when run in Release, we can see...the issue with setting the text initially as well (this works when in Debug for whatever reason):

image

Huh, the initial text scenario is working in the test... hmm. Will dig into it more as I work on fixing the other side.

(Could have something to do with initialization timing when loading directly vs. how the sample app processes?)

@michael-hawker
Copy link
Member

Sorted out the test:

image

There was a rogue binding which should have been meaningless, and wasn't working as expected, but also somehow causing the test to pass even though the correct behavior wasn't working when tested in the sample app.

Removed the binding and now the test fails as expected. Going to work to adding the proposed fixes in now and then can submit a PR.

Did notice an issue when hitting the clear button in the textbox itself after typing on an existing token, but hope that'll be resolved as well, that'd be harder to test in the current setup.

@michael-hawker
Copy link
Member

Close to fixing behavior, trying to test some other scenarios, and may have introduced a new bug. May have to try writing some UI tests here, looks like the first character received is being eaten now maybe.

  1. Add tokens to box
  2. Select first one with keyboard
  3. Type key, should go in textbox and start new dropdown for autosuggest, but currently not (though setting it in the Text field...)

@michael-hawker
Copy link
Member

Figured out the issue, was only setting the initial text for the last box. Should be resolved now. Not sure how to go about testing that yet, may need to beef up tests there in 8.0 world.

michael-hawker added a commit to michael-hawker/UWPCommunityToolkit that referenced this issue Oct 17, 2022
…nityToolkit#4749

Think we maybe used to bind to the text directly, but there's no Text property directly on the TokenizingTextBoxItem, so this binding is meaningless. It doesn't effect the behavior of the textbox in practical usage, but somehow was doing something which was masking the problem for us to be able to detect within a test case.
Removing it to reduce confusion.

The text sync between the parent TokenizingTextBox collection of items (and the current edit) and the item is maintained through code-behind with text changing events. Though work is being done to resolve issues in this sync process. See issue CommunityToolkit#4749
michael-hawker added a commit to michael-hawker/UWPCommunityToolkit that referenced this issue Oct 17, 2022
Hook up initialization and change detection of parent Text property to update corresponding inner text of TokenizingTextBoxItem
Failing tests from previous commit now pass.
@ghost ghost added the In-PR 🚀 label Oct 17, 2022
michael-hawker added a commit to michael-hawker/UWPCommunityToolkit that referenced this issue Oct 18, 2022
…nityToolkit#4749

Think we maybe used to bind to the text directly, but there's no Text property directly on the TokenizingTextBoxItem, so this binding is meaningless. It doesn't effect the behavior of the textbox in practical usage, but somehow was doing something which was masking the problem for us to be able to detect within a test case.
Removing it to reduce confusion.

The text sync between the parent TokenizingTextBox collection of items (and the current edit) and the item is maintained through code-behind with text changing events. Though work is being done to resolve issues in this sync process. See issue CommunityToolkit#4749
michael-hawker added a commit to michael-hawker/UWPCommunityToolkit that referenced this issue Oct 18, 2022
Hook up initialization and change detection of parent Text property to update corresponding inner text of TokenizingTextBoxItem
Failing tests from previous commit now pass.
michael-hawker added a commit to michael-hawker/UWPCommunityToolkit that referenced this issue Oct 18, 2022
…nityToolkit#4749

Think we maybe used to bind to the text directly, but there's no Text property directly on the TokenizingTextBoxItem, so this binding is meaningless. It doesn't effect the behavior of the textbox in practical usage, but somehow was doing something which was masking the problem for us to be able to detect within a test case.
Removing it to reduce confusion.

The text sync between the parent TokenizingTextBox collection of items (and the current edit) and the item is maintained through code-behind with text changing events. Though work is being done to resolve issues in this sync process. See issue CommunityToolkit#4749
michael-hawker added a commit to michael-hawker/UWPCommunityToolkit that referenced this issue Oct 18, 2022
Hook up initialization and change detection of parent Text property to update corresponding inner text of TokenizingTextBoxItem
Failing tests from previous commit now pass.
Repository owner moved this from In Progress to Done in Windows Community Toolkit Oct 18, 2022
@ghost ghost added Completed 🔥 and removed In-PR 🚀 labels Oct 18, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 controls 🎛️
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants