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

[regression/8.0.0-preview.1.7762] TwoWay Binding does not work when value is changed from the source ViewModel #16849

Closed
APopatanasov opened this issue Aug 18, 2023 · 14 comments · Fixed by #17110
Assignees
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 i/regression This issue described a confirmed regression on a currently supported version p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with partner Issue or Request from a partner team platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Milestone

Comments

@APopatanasov
Copy link

APopatanasov commented Aug 18, 2023

Description

I have Entry and Text property with TwoWay Binding. When I change the property in the ViewModel the UI of the Entry is not updated. Even though the example is with Entry I think there is a major issue with the TwoWay Binding because I am observing this issue in multiple controls.

I've tested the my code against the latest changes in net 8 and the issue persists.

Steps to Reproduce

  1. Run the project.
  2. Change the text in the Entry.
  3. Click the Button that sets the Text of the Entry to empty in the ViewModel.
  4. Notice how the text in the Entry itself is not changed.

Link to public reproduction project repository

https://github.com/telerik/ms-samples/tree/main/Maui/MauiBindingIssue

Version with bug

8.0.0-preview.7.8842

Is this a regression from previous behavior?

Yes, this used to work in .NET MAUI

Last version that worked well

8.0.0-preview.6.8686

Affected platforms

iOS, Android, Windows, macOS

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

@APopatanasov APopatanasov added the t/bug Something isn't working label Aug 18, 2023
@jsuarezruiz jsuarezruiz added area-xaml XAML, CSS, Triggers, Behaviors partner Issue or Request from a partner team i/regression This issue described a confirmed regression on a currently supported version labels Aug 18, 2023
@jsuarezruiz
Copy link
Contributor

Have done some tests on main branch and cannot reproduce the issue:

issue-16849-ios
issue-16849-droid

@ppluijten
Copy link

We have a similar issue: #16859
The difference for our case is that the clearing initially works, but after editing the text in the Entry the binding seems to break.

@samhouts samhouts added this to the .NET 8 GA milestone Aug 18, 2023
@samhouts samhouts changed the title TwoWay Binding does not work when value is changed from the source ViewModel [regression/8.0.0-preview.1.7762] TwoWay Binding does not work when value is changed from the source ViewModel Aug 18, 2023
@samhouts
Copy link
Member

I believe this was fixed by #16410

@APopatanasov
Copy link
Author

APopatanasov commented Aug 18, 2023

@samhouts I've tested my sample against both the main and net8 branch and the mentioned PR doesn't fix the issue.

@PureWeen
Copy link
Member

I'm able to reproduce this on preview7 @StephaneDelcroix

I realize that our code should be calling SetValue with a more correct specificity but it looks like the new code no longer takes into account the following settings inside BO https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/BindableObject.cs#L490-L491

Is that by design? By default, before your change, TwoWay bindings didn't clear when calling SetValue, only OneWay bindings did.

We have this issue to fix all of our setters #16654.

@mikeparker104 mikeparker104 added the partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with label Aug 24, 2023
@XamlTest XamlTest added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Aug 28, 2023
@XamlTest
Copy link

Verified this on Visual Studio Enterprise 17.8.0 Preview 1.0. Repro on Windows 11 .NET 8, Android 13.0-API33 and iOS 16.4 with below Project:
MauiBindingIssue.zip
ClearEntry

@samhouts samhouts added the p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint label Aug 28, 2023
@StephaneDelcroix
Copy link
Contributor

@PureWeen we no longer clear the bindings in any case (iirc)

@StephaneDelcroix
Copy link
Contributor

if the value is set with the correct specificity (fromHandler), everything's fine

[Fact]
		//https://github.com/dotnet/maui/issues/16849
		public void ValueFromHandlerDoesntClearWayBinding()
		{
			var vm = new MockViewModel();
			var entry = new Entry { BindingContext = vm};
			entry.SetBinding(Entry.TextProperty, "Text", BindingMode.TwoWay);
			entry.SetValueFromRenderer(Entry.TextProperty, "foo");
			Assert.Equal("foo", vm.Text);
			vm.Text = "bar";
			Assert.Equal("bar", entry.Text);
			vm.Text = string.Empty;
			Assert.Equal(string.Empty, entry.Text);
		}

@samhouts
Copy link
Member

samhouts commented Aug 29, 2023

Excellent. This means that this will be resolved when all of the tasks listed in #16654 are complete, which we will target for RC2.

I will close this one as a duplicate for now.

Thanks!

@samhouts
Copy link
Member

Duplicate of #16654

@samhouts samhouts marked this as a duplicate of #16654 Aug 29, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in MAUI SDK Ongoing Aug 29, 2023
@PureWeen PureWeen reopened this Aug 29, 2023
@PureWeen
Copy link
Member

Chatted with @StephaneDelcroix

There's still an issue with TwoWay bindings that needs to be resolved. We'll hopefully get that fixed for RC1 but if not we have all these coming along #16654

which will fix our controls

@samhouts samhouts marked this as not a duplicate of #16654 Aug 29, 2023
@StephaneDelcroix
Copy link
Contributor

there are 2 way to fix this, any one of the 2 would be enough, but both are required:

  • Set the value from the handlers with SetValueFromRenderer (or SetValue with a FromHnadler specificity), that's what's done in [Controls] Move ITextInput and IFontElement to InputView #17036
  • change the behaviour of 2WayBindings so they always apply, even after a SetValue. This is counterintuitive, but it's the observed behaviour in other xaml platforms

@APopatanasov
Copy link
Author

@PureWeen When you make a decision which option to choose please, take into account that the Entry scenario is one of many where the Binding is broken.

We have multiple composite Templated controls (not native) where the Binding is not working. Let's take for example the ComboBox. This is a control with selection that has SelectedItem property. When the selection is performed through the UI we need to set the property from our code which is no longer working.

Setting such properties with SetValueFromRenderer or SetValue with FromHandler specificity might not be so intuitive.

@StephaneDelcroix
Copy link
Contributor

@APopatanasov there's no option, we need to fix both

StephaneDelcroix added a commit that referenced this issue Aug 30, 2023
don't ask me why, that's how it is

- fixes #16849
PureWeen pushed a commit that referenced this issue Aug 30, 2023
don't ask me why, that's how it is

- fixes #16849
rmarinho added a commit that referenced this issue Aug 31, 2023
### Description of Change

don't ask me why, that's how it is

### Issues Fixed

- fixes #16849
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 i/regression This issue described a confirmed regression on a currently supported version p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with partner Issue or Request from a partner team platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants