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

Custom Data Attribute Validation is broken on Tab in 5.0.6 #1229

Closed
Mr-Technician opened this issue Mar 18, 2021 · 7 comments
Closed

Custom Data Attribute Validation is broken on Tab in 5.0.6 #1229

Mr-Technician opened this issue Mar 18, 2021 · 7 comments
Labels
bug Something does not work as intended/expected fixed Fix has been merged
Milestone

Comments

@Mr-Technician
Copy link
Member

Mr-Technician commented Mar 18, 2021

Describe the bug
When typing in a <MudTextField> within an <EditForm>, which is bound to a model field that is decorated with a custom validation attribute, tabbing to the next control will trigger validation of that attribute. In 5.0.6, the ValidationContext of said attribute will return null:

protected override ValidationResult IsValid(object value, ValidationContext validationContext)
        {
            var propertyTestedInfo = validationContext.ObjectType.GetProperty(""); //error will show up here. The validationContext is null after the control loses focus

            return ValidationResult.Success;
        }
public class Test
    {
        [Test]
        public string SomeString1 { get; set; }
        [Test]
        public string SomeString2 { get; set; }
    }
<EditForm Model="test">
        <DataAnnotationsValidator></DataAnnotationsValidator>
        <MudTextField @bind-Value="test.SomeString1" For="@(() => test.SomeString1)" Label="1"></MudTextField>
        <MudTextField @bind-Value="test.SomeString2" For="@(() => test.SomeString2)" Label="2"></MudTextField>
        <MudButton ButtonType="ButtonType.Submit" Variant="Variant.Filled">Submit</MudButton>
    </EditForm>

The issue doesn't occur in 5.0.5.

Expected behavior
Validation should run as expected and the ValidationContext parameter should not be null.

TryMudBlazor
Type something into one of the boxes and press tab. You should see an error in the console.
https://try.mudblazor.com/snippet/GawPEdvszHioVvlw

I have also included a link to a repository running 5.0.5, where the error does not occur.
https://github.com/ThatFlashCat/MudBlazorValidationError5.0.5

@Mr-Technician Mr-Technician added the bug Something does not work as intended/expected label Mar 18, 2021
@henon
Copy link
Collaborator

henon commented Mar 18, 2021

@GerkinDev can you look at this please? Maybe your last fix fixed this too?

@GerkinDev
Copy link
Contributor

GerkinDev commented Mar 18, 2021

The bug is indeed introduced by my PR #1198 with those lines, triggering validation here with implicit null context. This is a regression since validation was previously executed by the EditForm itself, using its Model property to properly initialize a ValidationContext.
This should be fixed by those lines of PR #1212 .

I'm working on test case to ensure it works as expected.

Note: I would suggest to extend a bit abilities of MudForm to have a Model property and other props to be used in place of EditForm, since it would keep track of the correct EditContext for validation. See this hacky workaround to support with/without EditForm.

@henon
Copy link
Collaborator

henon commented Mar 18, 2021

The philosophy behind MudForm was always validation by the input element and not the model as a contrast to EditForm which has model based validation. I am not sure if we should mix the two. The "For" params are supposed to be used with EditForm. While it is great that they work without EditForm now too I don't see the value of using MudForm when using "For" params because you can directly use EditForm. But maybe I am missing something?

GerkinDev referenced this issue in GerkinDev/MudBlazor Mar 18, 2021
@GerkinDev
Copy link
Contributor

GerkinDev commented Mar 18, 2021

Test added here: GerkinDev@f63dcf0

Please cherry-pick this commit, that only adds the test case for this issue (or ask me to PR).

About the responsability of EditForm & MudForm, I can't tell, you're the bosses ! But this part of #1212 adds use of ValidationAttribute even out of the context of an EditForm. AFAICT, they are behaving as expected in MudForm as long as validation attributes are not using other values from the model (just like CompareAttribute) as commented here. Maybe this part should be roll back then

henon referenced this issue Mar 18, 2021
Related to Garderoben/MudBlazor#1229

(cherry picked from commit f63dcf0)
@henon
Copy link
Collaborator

henon commented Mar 18, 2021

Cherrypicked your test. Thanks for it.

I thought a bit about it and I think the correct way to inject a validation context for model attributes is indeed to use EditForm. It can probably be combined with MudForm (inside or outside) without problems.

@GerkinDev
Copy link
Contributor

GerkinDev commented Mar 18, 2021

I thought a bit about it and I think the correct way to inject a validation context for model attributes is indeed to use EditForm. It can probably be combined with MudForm (inside or outside) without problems.

Yeah, I would have think of this too. The thing is, it would cause 2 nested form tags unless extra code for tree exploration up to detect if there's already a form, and I don't even know if this is possible. That's not a real functional problem I guess, but it would break the semantics of HTML.

Citing HTML spec:

Content model:
Flow content, but with no form element descendants.

@henon henon added the fixed Fix has been merged label Mar 18, 2021
@henon henon added this to the 5.0.7 milestone Mar 18, 2021
@henon henon closed this as completed Mar 18, 2021
@Mr-Technician
Copy link
Member Author

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected fixed Fix has been merged
Projects
None yet
Development

No branches or pull requests

3 participants