Inputs: Hide Clear button when readonly

[danielchalmers]

Description

This PR changes the behavior of standalone inputs which are both Clearable and ReadOnly. In this case, the clear button will be hidden which is the expected behavior.

However, if an input (i.e. MudInput or MudTextfield) is used within another component (i.e. MudTextfield is used in all pickers then this logic doesn't apply for good reason, meaning that the clear button is shown as long as Clearable is set without regard to the ReadOnly state. For more details see the discussion below.

Closes #4275 and closes #4277

How Has This Been Tested?

unit

Type of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (fix or improvement to the website or code docs)

Checklist

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[danielchalmers]

@ScarletKuro Do you know why the tests are failing? I'm missing something here

[ScarletKuro]

@ScarletKuro Do you know why the tests are failing? I'm missing something here

No idea. It seems like you broke something, since not only the new tests fail but some existing ones as well like SelectClearableTest and TimePicker_Should_Clear.

[danielchalmers]

@ScarletKuro The usage of "readonly" is super inconsistent. Sometimes it means not-editable, sometimes not. Take a look at a default MudSelect with Clearable=true and you'll see that the input has a readonly attribute. Read only doesn't always mean read only. It has different meanings. @henon Any ideas on how to escape this hole?

[henon]

Actually, I don't see a problem here. Unlike MudAutocomplete, MudSelect never allowed to edit the input text. It just looks like a text input, but is none. We can change this in the new MudComboBox

[danielchalmers]

Actually, I don't see a problem here. Unlike MudAutocomplete, MudSelect never allowed to edit the input text. It just looks like a text input, but is none. We can change this in the new MudComboBox

@henon I'm actually having this issue with several controls. Could you take a look at the failing tests

[henon]

I'm actually having this issue with several controls.

We'll have to look at them on a case-by-case basis.

Could you take a look at the failing tests

OK

[henon]

@danielchalmers I think the problem with the tests is this:

Whether a date picker is read-only is a different thing than whether its input is read-only. Like we discussed above, the input of a component can be read-only even if the parent component is not. This causes problems when the parent component expects a clear button to be shown even though it set its input to read-only.

I would use the cascading value SubscribeToParentForm to determine if we want to hide the clear button in an input when it is read-only. In other words, let's do this side-effect of the ReadOnly parameter only for stand-alone MudInputs and not for embedded ones.

[henon]

@danielchalmers I am pushing a fix that works.

[henon]

This fixes some tests but I don't know what is the problem with MudMask and the TimePicker.

[ScarletKuro]

hope we will get rid of that State suffix

[henon]

I am pretty sure that this new test is just not correct. Setting the text doesn'T update the mask's text, probably because the mask pattern is not initialized or whatever. Try utilizing one of the working tests and add the clear button checks to it.

[danielchalmers]

That's probably correct. I'm not familiar with the mask and just applied the test from autocomplete

[danielchalmers]

@ScarletKuro @henon Any insight or commits you guys can make would be really helpful because I'm not as familiar with the long history of the components and this would let me work on CSS/layout plans I have for several components. I didn't expect this small tweak to be so involved

[henon]

@danielchalmers I will help you but it is very time consuming to get this working as it is even hard for me to get back into the complicated logic of these components. I also thought I might fix it in an hour then had to give up for the day after two.

[ScarletKuro]

@ScarletKuro @henon Any insight or commits you guys can make would be really helpful because I'm not as familiar with the long history of the components and this would let me work on CSS/layout plans I have for several components. I didn't expect this small tweak to be so involved

Typically, I don’t usually get involved with input components. The experts here are @mckaragoz and @Mr-Technician, who have worked on the disabled and read-only states, with @henon, who has been part of the project from the start.

even hard for me to get back into the complicated logic of these components

Yes, looking at all this, I really don’t like what I’m seeing. I might even suggest that we abandon it for now and take some time to rethink things.

I recommend writing down all the input components and going through them one by one to describe how each component should behave when it is in the disabled or read-only state. Should users be able to write in the input? Are clearable buttons visible, etc.? Then we can analyze what is currently implemented and compare with how it behaves now, and come up with ideas on how to standardize this.

I think we could create an internal interface, IEditable, that would include ReadOnly, Disabled, ParentReadOnly, ParentDisabled, etc. (we might even split it into two interfaces if not all components require a parent). Would help us avoid duplication everywhere:

private bool GetClearable()
{
    if (SubscribeToParentForm)
        return Clearable && !GetReadOnlyState() && !GetDisabledState();
    return Clearable && !GetDisabledState();
}

protected bool GetReadOnlyState() => ReadOnly || ParentReadOnly;

protected bool GetDisabledState() => Disabled || ParentDisabled;

We could also use extension methods, and potentially have variations of them if some inputs require special cases.

[codecov]

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.17%. Comparing base (928d946) to head (7aba4ce).
Report is 15 commits behind head on dev.

Files with missing lines	Patch %	Lines
.../MudBlazor/Components/Input/MudRangeInput.razor.cs	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev   #10070      +/-   ##
==========================================
+ Coverage   91.14%   91.17%   +0.03%     
==========================================
  Files         411      411              
  Lines       12482    12481       -1     
  Branches     2422     2429       +7     
==========================================
+ Hits        11377    11380       +3     
+ Misses        560      557       -3     
+ Partials      545      544       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[henon]

I think I got everything to work though. The desire to avoid duplication was what made us derive all inputs from MudBaseInput. This led to the brittle base class syndrom we are suffering from now. To tell the truth, I think it is much better to have a little duplication instead of shared functionality which breaks everything if you only look at it sideways.

I recommend writing down all the input components

I agree, and we will do this for sure. Edit: I read this as I recommend rewriting all the input components ... and that was what I wanted to agree to ;)

[henon]

While working on this I discovered that MudInput on its own doesn't even have any test cases! I am adding the first one now for the readonly / clearable functionality

[henon]

OK, I even manged to pull the ShowClearButton() logic up into MudBaseInput to avoid duplication. It was the same in MudTextField and MudInput. Except for Mask, which is a special case.

Edit: looking at it again, that probably isn't worth it. The last commit could be reverted, the tests would still work.

[henon]

@danielchalmers you can take it from here.