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

The order of 'IsEnabled' and 'Command' should not matter #2758

Open
mattleibow opened this Issue May 17, 2018 · 11 comments

Comments

10 participants
@mattleibow
Copy link
Contributor

mattleibow commented May 17, 2018

Description

I have two buttons:

<Button
    Text="Button1"
    Command="{Binding TheCommand}"
    IsEnabled="False" />

<Button
    Text="Button2"
    IsEnabled="False"
    Command="{Binding TheCommand}" />

For some reason, the IsEnabled property is overwritten in Button2 - this enables the button incorrectly.

Button1 is disabled this way, but when I trigger TheCommand.ChangeCanExecute(), the button enables.

Expected Behavior

Both buttons stay disabled.

The matrix of this should be an && operation:

TheCommand.CanExecute() IsEnabled Final State
True True Enabled
True False Disabled
False True Disabled
False False Disabled

Actual Behavior

The button with the command applied later overwrites the enabled state.

Basic Information

  • Version with issue: 2.5.x and 3.0
  • Last known good version: unsure
  • IDE: VS2017
  • Platform Target Frameworks: all

@pauldipietro pauldipietro added this to New in Triage May 17, 2018

@mattleibow

This comment has been minimized.

Copy link
Contributor Author

mattleibow commented May 17, 2018

I just checked with native UWP, and the matrix is the platform does - and the order doesn't matter.

@pauldipietro pauldipietro moved this from New to Ready For Work in Triage May 17, 2018

@hartez

This comment has been minimized.

Copy link
Member

hartez commented May 17, 2018

@pauldipietro I don't think is is a XAML issue. IsEnabledProperty and Command.CanExecute are each changing IsEnabled completely independent of one another. You could fail the matrix above with pure C#.

@mattleibow

This comment has been minimized.

Copy link
Contributor Author

mattleibow commented May 18, 2018

I was looking at this and I think the real issue is the fact that the IsEnabled property is being overwritten entirely.
What we can do is make use of the coercing feature and actually use that. When IsEnabled is set, we store the actual value and then run a coerce on it. In the Button, we implement a coerce function to return the Command.CanExecute value. The resulting IsEnabled should be the propertyValue && infusedValue.

class VisualElement {
    BindableProperty IsEnabledProperty = BindableProperty.Create(
        ...,
        coerceValue: OnIsEnabledCoerce);

    static object OnIsEnabledCoerce(BindableObject bindable, object value) {
        var element = (VisualElement)bindable;
        if (element == null)
            return false;
        if ((bool)value) {
            return element.IsEnabledCore;
        } else {
            return false;
        }
    }

    protected virtual bool IsEnabledCore { get { return true; } }
}

class Button {
    protected override bool IsEnabledCore {
        get { return base.IsEnabledCore && CanExecute; }
    }
    bool CanExecute {
        get { return _canExecute; }
        set {
            if (_canExecute != value) {
                _canExecute = value;
                ReCoerceValue(IsEnabledProperty);
            }
        }
    }
    void CommandCanExecuteChanged(object sender, EventArgs eventArgs) {
        CanExecute = Command?.CanExecute(CommandParameter) ?? true;
    }
}
@mattleibow

This comment has been minimized.

Copy link
Contributor Author

mattleibow commented May 18, 2018

I tried to do something, but the binding code is just a bit too complex for me at the moment. I could get things to work mostly OK if I stored the RealValue, the CoercedValue and a IsCoerced flag, but things got wonky because I don't quite understand the setting logic too well.

Hopefully this helps someone, somewhere.

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

StephaneDelcroix commented May 18, 2018

unrelated to xaml, the same can be achieved in code

@TruffelNL

This comment has been minimized.

Copy link

TruffelNL commented Sep 8, 2018

It used to matter what order you have. Currently it looks like it's broken all the way. IsEnabled works until I add Command. As soon as I do that, it doesn't look at IsEnabled anymore.

@andreinitescu

This comment has been minimized.

Copy link
Contributor

andreinitescu commented Sep 9, 2018

This problem is many years old, I think at least 3.

@StephaneDelcroix Shouldn't this be marked as a bug and not as an enhancement?

@andreinitescu

This comment has been minimized.

Copy link
Contributor

andreinitescu commented Sep 9, 2018

From all the XAML frameworks, this problem is only in Xamarin Forms

@samhouts samhouts removed this from Ready For Work in Triage Oct 4, 2018

@samhouts samhouts added this to Under consideration in Enhancements Feb 5, 2019

@samhouts samhouts moved this from Under consideration to Needs Specification in Enhancements Feb 6, 2019

@jonkas2211

This comment has been minimized.

Copy link

jonkas2211 commented Feb 26, 2019

In Xamarin.Forms v3.4.0.1008975 it completly ignores the order. IsEnabled is always true, when using a Command!

grafik

grafik

grafik

When is this likely to be fixed?
The only workaround is to set the CanExecute of the ICommand right know.

@dansiegel

This comment has been minimized.

Copy link
Contributor

dansiegel commented Mar 5, 2019

perhaps leave it to me to ruin the party here... but this would introduce a MAJOR breaking change in behavior which means you break people without them knowing it... now "Maybe" its a worthwhile break (though I heavily lean towards no)... but can you honestly name a scenario in which you have two buttons attached to the same command where one should be disabled and the other shouldn't... and you're either NOT using a Parameter or using the same Command Parameter?

The entire point of Can Execute is that you're evaluating the parameter null or otherwise to determine if it can/should execute...

@jbachelor

This comment has been minimized.

Copy link

jbachelor commented Mar 8, 2019

This certainly seems like a bug to me... If it cannot be fixed, I would at least propose some sort of warning be added to the documentation of the IsEnabled property and the Button class. I've created a small project to reproduce the issue, as well as provided a few branches to demonstrate ways to avoid the issue by using CanExecute rather than IsEnabled: https://github.com/jbachelor/XamarinFormsIsEnabledIssue

@mattleibow mattleibow added this to New in Triage via automation Mar 10, 2019

@mattleibow mattleibow removed this from Needs Specification in Enhancements Mar 10, 2019

@mattleibow mattleibow removed the t/bug 🐛 label Mar 10, 2019

@mattleibow mattleibow removed this from New in Triage Mar 10, 2019

@mattleibow mattleibow added this to New in Triage via automation Mar 10, 2019

@samhouts samhouts added breaking and removed t/enhancement labels Mar 11, 2019

@samhouts samhouts moved this from New to Needs Estimate in Triage Mar 14, 2019

@samhouts samhouts added the e/5 🕔 label Mar 22, 2019

@samhouts samhouts moved this from Needs Estimate to Ready For Work in Triage Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.