dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.14k stars 1.74k forks source link

IsEnabled=False in RefreshView is inherited by child controls. (Was not the case with Xamarin) #22699

Open inimirpaz opened 4 months ago

inimirpaz commented 4 months ago

Description

Child controls of a RefreshView (supposedly) wrongly inherit the IsEnable when set to false. This is an issue when migrating from Xamarin and your intent is to, for condition X, inhibit the pull-to-refresh.

Repro contains both the MAUI and Xamarin solutions.

Steps to Reproduce

No response

Link to public reproduction project repository

https://github.com/inimirpaz/maui_issues/tree/refrview_isenabled_issue

Version with bug

8.0.40 SR5

Is this a regression from previous behavior?

Yes, this used to work in Xamarin.Forms

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

github-actions[bot] commented 4 months ago

Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

Open similar issues:

Note: You can give me feedback by thumbs upping or thumbs downing this comment.

ninachen03 commented 4 months ago

Verified this issue with Visual Studio 17.11.0 Preview 1.0 (8.0.40 & 8.0.3). Can repro it. by the way, Xamarin forms works fine.

mattleibow commented 4 months ago

This is an interesting one because IsEnabled is supposed to disable all the child controls. How would you disable all the controls if you wanted to?

I also see the IsEnabled is tied to Command.CanExecute and I think that is wrong. The command is for the refresh ability, the IsEnabled is for the normal enabled/disabled system.

This is a "regression" or a "difference" from Forms, but technically forms was very wrong. But there is still a bug in maui.

johannes-keinestam commented 4 months ago

Running into the same issue. We have a base ContentPage class containing a StackLayout in a ScrollView in a RefreshView. Not all subclasses actually used the pull-to-refresh functionality, so by default the RefreshView has IsEnabled set to false. Felt quite reasonable to have that as a wrapper which could be enabled or disabled on demand, instead of having to fiddle with the layout hierarchy on every Page you needed it.

Furthermore, it was neat to be able to temporarily disable pull-to-refresh e.g. while some data was reloaded in the background. Previously this worked fine, but now it means a lot of visual changes (e.g. buttons getting grayed out).

If IsEnabled should indeed cascade to child views for RefreshView, would it not makes sense to introduce an explicit property to disable pull-to-refresh on a RefreshView then?

inimirpaz commented 4 months ago

If IsEnabled should indeed cascade to child views for RefreshView, would it not makes sense to introduce an explicit property to disable pull-to-refresh on a RefreshView then?

@mattleibow I am 100% on board with what @johannes-keinestam 's saying here. If IsEnabled is supposed to disable child controls, there must be another way to inhibit the pull-to-refresh alone.

mattleibow commented 4 months ago

would it not makes sense to introduce an explicit property to disable pull-to-refresh on a RefreshView then?

I forgot to type out the conversation in my brain. Yes, this is what should be happening. It is a bit weird that forms used the same property to do entirely different things.

johannes-keinestam commented 4 months ago

I ended up creating my own RefreshView subtype which introduces a IsRefreshEnabled property for now, in case that helps anyone. Seems to work as expected from my brief testing at least (implemented on iOS and Android).

Sorta wonky solution with modifying the static RefreshViewHandler.Mapper but wanted to keep the workaround to a single file and couldn't figure a neater way of doing it.

RefreshView.cs ```c# /// /// A variant of which allows disabling the pull-to-refresh /// functionality without disabling any children. /// public class RefreshView : Microsoft.Maui.Controls.RefreshView { /// /// Bindable property for . /// private static readonly BindableProperty IsRefreshEnabledProperty = BindableProperty.Create(nameof(IsRefreshEnabledProperty), typeof(bool), typeof(RefreshView), defaultValue: true); /// /// Reflects whether the user can pull down to refresh. /// public bool IsRefreshEnabled { get => (bool)GetValue(IsRefreshEnabledProperty); set => SetValue(IsRefreshEnabledProperty, value); } /// /// Sets up the default mapper to work with this variant. /// static RefreshView() { // Make sure that setting VisualElement.IsEnabled = true does not override IsRefreshEnabled = false. RefreshViewHandler.Mapper.ReplaceMapping(nameof(IsEnabled), (IRefreshViewHandler handler, IRefreshView view) => { if (view is RefreshView refreshView) { OnRefreshEnabledChanged(handler, refreshView); } else { RefreshViewHandler.MapIsEnabled(handler, view); } }); RefreshViewHandler.Mapper.Add(nameof(IsRefreshEnabled), (handler, view) => { if (view is RefreshView refreshView) { OnRefreshEnabledChanged(handler, refreshView); } }); } /// /// Called when or has changed. /// Updates the enabled state of the platform view accordingly. /// /// The platform-specific handler of the virtual view whose property has changed. /// The virtual view whose property has changed. private static void OnRefreshEnabledChanged(IRefreshViewHandler handler, RefreshView view) { var allowRefresh = view.IsRefreshEnabled && view.IsEnabled; #if IOS handler.PlatformView.UpdateIsEnabled(allowRefresh); #elif ANDROID handler.PlatformView.Enabled = allowRefresh; #endif } } ```
Xideta commented 4 months ago

Seems to be the same issue as #19091, but the solution posed here is much nicer.

zagoskin commented 4 months ago

I ended up creating my own RefreshView subtype which introduces a IsRefreshEnabled property for now, in case that helps anyone. Seems to work as expected from my brief testing at least (implemented on iOS and Android). ...

@johannes-keinestam Sorry, a bit of a MAUI noob here. How would I go about using your custom RefreshView? I was wrapping the BlazorWebView with the original RefreshView previously, and using an injected service I would be able to track its state before. But know I seem to get some errors when using your new version.

johannes-keinestam commented 4 months ago

@zagoskin I'm not very familiar with BlazorWebView, but I'd imagine replacing the reference to Microsoft.Maui.Controls.RefreshView with this new RefreshView class would still allow any state tracking you did previously since this class just extends the Maui variant. Except for the new IsRefreshEnabled property, that is. You could listen for changes to that state for example by attaching to the PropertyChanged event, either in C#:

myRefreshViewReference.PropertyChanged += (_, args) =>
{
    if (args.PropertyName == nameof(RefreshView.IsRefreshEnabled))
    {
        // Do your processing here.
    }
}

or in XAML:

<views:RefreshView
    ...
    PropertyChanged="OnPropertyChanged"/>
mattleibow commented 4 months ago

I wonder if you could replace this with an attached property. Then you can still use the normal view, but in the mapper you can check the attached property for a value.

dzebas commented 4 weeks ago

@johannes-keinestam I'm trying to implement your example as looks like Maui developers won't do anything and latest Maui.Controls (8.0.90) completely broke RefreshView now, e.g. if IsEnabled = false then all controls gets disabled (was not the case in 8.0.82).

I had to change BindableProperty to public in you code otherwise I get compile errors. But when I run my app it doesn't trigger OnRefreshEnabledChanged when value for IsRefreshEnabled changes. It does trigger when you first open the page though. If I change InEditMode in my view model to true it should trigger this event.

Below is my XAML, what I'm doing wrong? Thanks in advance!

<jim2:RefreshView
        IsRefreshEnabled="{Binding InEditMode, Converter={StaticResource inverter}}"
        IsRefreshing="{Binding IsHeaderRefreshing}"
        Command="{Binding RefreshCommand}">

Edit: found the issue, in the example binding is incorrect, should be as follows:

public static readonly BindableProperty IsRefreshEnabledProperty = BindableProperty.Create(nameof(IsRefreshEnabled), typeof(bool), typeof(RefreshView), defaultValue: true);

alex3696 commented 3 weeks ago

+1 I think my problem is related to this bug. ScrollView doesn't scroll when canExecute in RefreshView Command returns false.

for example, in View model: public Command CmdTest { get; } = new Command(() => { }, () => false); and in View:

<RefreshView
    Command="{Binding CmdTest}"
    IsRefreshing="{Binding IsRefreshing}" >
    <ScrollView>
        <VerticalStackLayout>
            <Border HeightRequest="200" BackgroundColor="Gray" Padding="4"/>
            <Border HeightRequest="200" BackgroundColor="Gray" Padding="4"/>
            <Border HeightRequest="200" BackgroundColor="Gray" Padding="4"/>
            <Border HeightRequest="200" BackgroundColor="Gray" Padding="4"/>
            <Border HeightRequest="200" BackgroundColor="Gray" Padding="4"/>
            <Border HeightRequest="200" BackgroundColor="Gray" Padding="4"/>
        </VerticalStackLayout>
    </ScrollView>
</RefreshView>

tested: scrolling works in xamarin (Windows, Android) but does not work in Maui.