Allow hiding ValidationSummary content when there are no errors

[Bartmax]

<div asp-validation-summary="ValidationSummary.ModelOnly" class="alert alert-danger alert-dismissible" role="alert">
    <button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">&times;</span></button>
    <h4>Oops! Looks like there were some errors.</h4>
</div>

If I use the validation summary tag helper with custom code, this get renders even when there's no error. I think validation summary should support (or always behave) like don't render anything if there's no error something like. <div as-validation-summary-visible="Always|OnError">

[DamianEdwards]

Can you explain this a little more? What do you mean by "custom code"? Are you saying the <h4> is always being rendered, even when there is no validation error currently?

[Bartmax]

Exactly, and the button too.

I currently have :

        @if (ViewData?.ModelState[""]?.Errors?.Any() == true)
        {
        <div asp-validation-summary="ValidationSummary.ModelOnly" class="alert alert-danger alert-dismissible" role="alert">
            <button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">&times;</span></button>
            <h4>Oops! Looks like there were some errors.</h4>
        </div>
        }

to achieve the desired output.

I'm on beta3 btw.

[dpaquette]

@Bartmax I definitely see the value in this. I have had similar problems before when trying to add additional content or styling to the validation summary div.

The problem with the conditional rendering you are using now is that it doesn't work with client side validation. In some cases, model level errors will not be displayed on the client side because the div is not there.

I think this might be something that is better handled on the client side. If there is a class or attribute added to the validation summary then we can easily handle the show/hide via CSS. This would likely require a change to both the unobtrusive validation JS and the server side GenerateValidationSummary code.

[dougbu]

@dpaquette

If there is a class or attribute added to the validation summary then we can easily handle the show/hide via CSS

This is supported today: The GenerateValidationSummary() code adds one of the validation-summary-errors or validation-summary-valid classes depending on the status when the page is generated. Beg, borrow, or steal JavaScript code that does whatever you like for <div> elements with those classes.

[Bartmax]

@dougbu validation-summary-errors or validation-summary-valid are only used after the form submission. So when you enter the page for first time (get request) you don't get any of those classes on div.
Knowing the existence of them I can get to get the desired behavior without c# code but with css.

note that validation-summary class is custom app class, not framework one

css:

.validation-summary{
    display:none;
}
.validation-summary.validation-summary-errors{
    display:block;
}

cshtml:

<div asp-validation-summary="ValidationSummary.ModelOnly" class="validation-summary alert alert-danger alert-dismissible" role="alert">
    <button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">&times;</span></button>
    <h4>Oops!</h4>
</div>

I'm not sure if there's anything to do here, I leave it just in case. Feel free to close it.

[dougbu]

I'm not sure if there's anything to do here

There is something we could do though it may cause more trouble than it's worth. My "supported today" point was specifically about existing CSS classes.

We could add an optional attribute to the <div asp-validation-summary...> helper that disables rendering of the <div> or <div> body when the model is valid. One potential problem is interactions between that and Unobtrusive validation, which updates the <div> if client-side validation errors occur.

[CezarCretu]

The way this currently works (beta7) seems unintuitive to me: using ValidationSummary.ModelOnly, the validation-summary-errors class is still generated even if there aren't any non-property validation errors to display but the form is otherwise not valid. This all means that the approach outlined by @Bartmax would result in displaying a div with a generic message of no use when there are property errors handled in other ways.

While I understand that asp-validation-summary="ValidationSummary..." acts more like a filter for the type of errors that are displayed, I think that having an additional attribute that controls the rendering (not necessarily disabling it, it could be an additional class) based on the same enum would be useful.

[Bartmax]

Maybe we need a validation-summary-initial class with a default display:none, for the 'missing' state.

There are 3 states, initial, valid and error. We currently have classes for valid and error. Do you think it add value to have a validation-summary-initial class ?

[DamianEdwards]

Probably easier to just always apply a class of validation-summary, and then stack the validation-summary-valid and validation-summary-error when they apply. That way, you can do the following to get the desired behavior:

.validation-summary { display:none; }
.validation-summary.validation-summary-valid, .validation-summary.validation-summary-error { display: block; }

[DamianEdwards]

@Eilon please see my comment above. We should make this simple change for the next release if possible, please re-triage.

[Bartmax]

that will work 👍 just wanted to note, with validation-summary-valid you don't want to display the div.

.validation-summary { display:none; }
.validation-summary.validation-summary-error { display: block; }

[DamianEdwards]

@Bartmax yeah that 😄

[CezarCretu]

@DamianEdwards while that's useful, that doesn't address my problem: the validation summary element will still be invalid (and have the validation-summary-error class) if used with ValidationSummary.ModelOnly in the case where there are no model-only messages to display, but the form is still invalid.

The scenario I'm thinking of is that I may have some property errors that I have handled otherwise, but in case I can't, for whatever reason, determine what properties the errors relate to, then I would like to show the validation summary (and only then).

With the current behavior, the summary is still shown when the form is invalid, even if I explicitly ask for ModelOnly errors and even if there are no errors with key="", meaning it will only render an empty div (since the errors have been filtered out by ValidationSummary.ModelOnly) with the validation-summary-error class..

Do you think this should be a separate issue or is it a scenario that will not be covered by asp-validation-summary?

[dougbu]

@CezarCretu one workaround would be to special-case validation-summary-error :not(:empty) instead of the entire class.

Am I correct what you'd prefer is for <div asp-validation-summary="ValidationSummary.ModelOnly" ...></div> to generate nothing unless model-level errors exist? The code currently special-cases ModelOnly when the model is valid server-side (because client code only validates properties). So what I think you're asking wouldn't be much of a stretch.

[CezarCretu]

Yeah, either generate nothing (which would relate to the original discussion) or not add the validation-summary-error class.

It's not hard to get around, but ValidationSummary.ModelOnly is kind of misleading, since it doesn't do exactly what the name says it would, or maybe my expectations are skewed.

Your :not(:empty) workaround wouldn't work though, since inner html does get generated:

<ul>
   <li style="display:none"></li>
</ul>

[Eilon]

Clearing milestone so we'll see it in triage.

[Eilon]

Need to add a CSS class to the outer <div> so that the rendering can be customized.

[dougbu]

@danroth27 seems odd to have an assigned issue in the Backlog milestone. Should it actually be in RC2? If not, I'll just ignore it 🚶🚶🚶

[dougbu]

Looking back at this, I believe we will have a couple of fairly quick fixes for this issue once #5209 is fixed. I would appreciate opinions on the following from everyone who has commented above. @Bartmax @DamianEdwards @Eilon and the rest: don't hold back.

Fixing #5209 at least means the <div validation-summary="..."> tag helper won't generate a <div> with the validation-summary-errors class and no displayed errors (as @CezarCretu mentioned).

The remaining work here is to somehow hide the <div> element itself when there are no server-side validation errors and client-side validation cannot add errors. First a bit of background: The tag helper is effectively calling IHtmlHelper.ValidationSummary(...) and that lower-level code will return null in this case. (ValidationSummary() normally returns a TagBuilder with attributes to add to the <div> and a <li> to place in its body.) The tag helper then renders the <div> unaltered except to remove the asp-validation-summary attribute. This is also the "get request" condition @Bartmax describes above.

Hiding the <div> either means not rendering it at all when ValidationSummary() returns null or always adding a class that is styled as display:none. The first makes the tag helper a bit more like the HTML helper but would be a behaviour change from 1.0.0 as well as make the tag helper's behaviour less straightforward. (Today the <environment> tag helper is the only one that sometimes renders nothing. But that's what it's for.) The second option is less of a behaviour change but doesn't clean anything up for existing applications -- they'd lack the styling for the proposed validation-summary class.

If we go for the validation-summary class addition, will also need to decide whether the HTML helper should use it as well. I lean toward "yes" on this secondary question. I also lean toward this overall approach but am less definite there.

Again, thoughts appreciated...

[dougbu]

Fixed like -> lack typo.

[dougbu]

As @Eilon said, assigning this to any Doug and moving it into 1.1.

[dougbu]

Offline decision was to have any <div asp-validation-summary="..."></div> render nothing in the same cases where @Html.ValidationSummary(...) returns HtmlString.Empty. No need for the proposed validation-summary CSS class since rendered <div>s will always have either the validation-summary-error or validation-summary-valid class.

[Bartmax]

@dougbu

Option 1: (it may have unintended side-effects)
Doesn't the clientside javascript expect that div to exist to append the errors?
Even if it doesn't, some people may have custom javascript that depend on that div.

Option 2: (no breaking changes)
Current application will not be affected with this change which means no breaking changes (afaik a big bonus for microsoft) and they already had to deal with the visibility issues.

I will go with option2, adding the style to the default templates

[Bartmax]

CSS class since rendered <div>s will always have either the validation-summary-error or validation-summary-valid class.

you normally want to hide summary-valid class, so css style is still needed ?
maybe im not following

[dougbu]

@Bartmax backing up a bit, note everything already handles @Html.ValidationSummary(...) not being used in a <form> or returning nothing (HtmlString.Empty) in many cases. Option 1 simply makes the tag helper do the same as the HTML helper.

Doesn't the clientside javascript expect that div to exist to append the errors?

No, it searches using a CSS selector ([data-valmsg-summary=true]) within the <form> and handles the already-frequent case where the element doesn't exist.

you normally want to hide summary-valid class, so css style is still needed ?

CSS classes will still exist. But, going with option 1, there's no need for an additional validation-summary class. Style validation-summary-error (or validation-summary-valid) as you wish.

[dougbu]

0a4c06e