Skip to content

fix(blazor): preserve ValidationSummary fixed class when AdditionalAttributes contains class#66531

Open
EduardF1 wants to merge 17 commits into
dotnet:mainfrom
EduardF1:feat/validationsummary-cssclass
Open

fix(blazor): preserve ValidationSummary fixed class when AdditionalAttributes contains class#66531
EduardF1 wants to merge 17 commits into
dotnet:mainfrom
EduardF1:feat/validationsummary-cssclass

Conversation

@EduardF1
Copy link
Copy Markdown
Contributor

@EduardF1 EduardF1 commented Apr 29, 2026

Summary

Fixes #43860 — users can now pass CSS utility classes to <ValidationSummary> via attribute splatting without losing the built-in validation-errors class.

Before (broken)

azor <ValidationSummary class=""pt-2"" />
Rendered: <ul class=""pt-2""> — the fixed validation-errors class was overwritten by AdditionalAttributes.

After (fixed)

azor <ValidationSummary class=""pt-2"" />
Rendered: <ul class=""pt-2 validation-errors""> — both classes are combined.

Approach

Uses the same internal pattern as InputRadio (no new public API):

csharp builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttributeIfNotNullOrEmpty(2, ""class"", AttributeUtilities.CombineClassNames(AdditionalAttributes, ""validation-errors""));

CombineClassNames merges the splatted class attribute (if any) with the component's fixed class string. The fixed class always appears last, matching the InputRadio convention.

Test coverage

E2E test (FormsTest.ValidationSummaryCombinesAdditionalAttributesClassWithDefault) verifies:

  1. Default renders class=""validation-errors""
  2. class=""pt-2"" combines to class=""pt-2 validation-errors""
  3. Non-class attributes (data-testid) pass through correctly

@EduardF1 EduardF1 requested a review from a team as a code owner April 29, 2026 22:14
@github-actions github-actions Bot added the area-blazor Includes: Blazor, Razor Components label Apr 29, 2026
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Thanks for your PR, @EduardF1. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@EduardF1
Copy link
Copy Markdown
Contributor Author

@dotnet-policy-service agree

EduardF1 and others added 4 commits May 3, 2026 18:16
… condition

The ul element is only rendered after Blazor processes validation on form
submit. Calling FindElement synchronously after submitButton.Click() could
throw NoSuchElementException before the re-render completes.

Moving FindElement inside the Browser.Equal lambda ensures the element
lookup is retried until the ValidationSummary ul is present in the DOM.
EduardF1 and others added 2 commits May 5, 2026 01:30
The previous commits replaced MountTypicalValidationComponent() with
Browser.MountTestComponent<ValidationSummaryWithCssClassComponent>() in
~19 unrelated form tests. The new minimal component only renders a
Name field, so every test that looks up `age`, `height`, `description`,
`ticket-class`, `cities`, etc. lost its element under test and CI went
red on Ubuntu/Helix.

This restores the original test fixture for all unrelated tests, drops
the redundant ValidationSummaryWithCssClassComponent.razor (and its
Index.razor entry), and adds the CssClass coverage directly to
TypicalValidationComponent.razor where the dedicated
ValidationSummaryAppliesCssClass test already lives.
@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 5, 2026

Pushed 9f64eb9 to scope the test changes correctly.

The previous revision replaced MountTypicalValidationComponent() with Browser.MountTestComponent<ValidationSummaryWithCssClassComponent>() in 19 form tests, but the new fixture only renders a Name field — every test that looks up age, height, description, ticket-class, cities etc. lost its element under test, which is what was turning the Helix and components-e2e queues red.

This commit:

  • Restores MountTypicalValidationComponent() for all 19 unrelated tests.
  • Drops ValidationSummaryWithCssClassComponent.razor (and its Index.razor entry) — there's already a dedicated ValidationSummaryAppliesCssClass test, no need for a parallel fixture.
  • Adds the <ValidationSummary CssClass="custom-summary-css-class" /> block directly into TypicalValidationComponent.razor so the dedicated test has the all-errors-custom-class container it expects.

Net diff is +22/−60: the production code change (BindRenderTree + CssClass parameter) is unchanged, only the E2E test wiring is corrected.

…ionSummary

The previous attempt added a second <ValidationSummary CssClass=...> alongside
the existing one inside TypicalValidationComponent.razor. Every form test that
counts .validation-message elements globally (FindElements(By.ClassName(...))
in CreateValidationMessagesAccessor) then saw each error twice, which is what
turned the Helix Subset 1 + components-e2e queues red after build 1409664
(InputDateInteractsWithEditContext_*, InputSelectInteractsWithEditContext,
InputTextInteractsWithEditContext, etc., all asserting Expected: ["..."]
vs Actual: ["...", "..."]).

Restores ValidationSummaryWithCssClassComponent.razor as a focused fixture
(name input + both summaries), registers it in Index.razor, removes the
extra <ValidationSummary CssClass=...> from TypicalValidationComponent.razor,
and points only ValidationSummaryAppliesCssClass at the new component.

Net: 19+ unrelated form tests stop seeing duplicates; the dedicated CssClass
test still asserts both 'validation-errors' and 'validation-errors
custom-summary-css-class'.
@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 6, 2026

Pushed 72cbda6 which should resolve the Helix Subset 1 + components-e2e failures from build 1409664/1409666.

Root cause was in 9f64eb9 from yesterday: I'd added a second <ValidationSummary CssClass="custom-summary-css-class" /> to TypicalValidationComponent.razor so the dedicated test had its container, but CreateValidationMessagesAccessor (in both FormsTest.cs and FormsInputDateTest.cs) selects .validation-message elements across the whole appElement:

appElement.FindElements(By.ClassName("validation-message")).Select(x => x.Text)...

So every error rendered by both <ValidationSummary>s got reported twice. That's exactly what the failures showed, e.g. InputDateInteractsWithEditContext_MonthInput:

Expected: ["The VisitMonth field must be a year and month."]
Actual:   ["The VisitMonth field must be a year and month.", "The VisitMonth field must be a year and month."]

19+ unrelated form tests across FormsTest, ServerFormsTest, and FormsInputDateTest all hit the same pattern.

The new revision restores ValidationSummaryWithCssClassComponent.razor (40-line focused fixture: a name input + both summaries), registers it in Index.razor, drops the extra <ValidationSummary CssClass=...> block from TypicalValidationComponent.razor, and points only ValidationSummaryAppliesCssClass at the new component. Net diff is +42/−4 plus one new file.

For Helix Subset 1's Interop.FunctionalTests.HttpClientHttp2InteropTests.ServerReset_BeforeRequestBody_ClientBodyThrows(scheme: "http") failure (build 1409664, expected HttpProtocolException, got TimeoutException after 30s) — that one's a Kestrel HTTP/2 interop test with no surface to my BindRenderTree/ValidationSummary change. Reads as a flaky timing-sensitive infrastructure failure unrelated to this PR; should clear on the next run.

@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 7, 2026

The two failing checks on the latest commit (72cbda6) are unrelated to this PR:

  • aspnetcore-components-e2e (build 1410968): 2061 passed, 1 failed. The failing test is ServerVirtualizationTest.AnchorMode_End_AppendAfterLeavingBottom_DoesNotReengage(variableHeight: False, useItemsProvider: False) at src/Components/test/E2ETest/Tests/VirtualizationTest.cs:3093, which exercises virtualization scroll-anchor behavior and is unrelated to ValidationSummary CssClass.
  • aspnetcore-template-tests-pr (build 1410969): the Templates.Tests--net11.0 Helix work item dead-lettered (Helix infrastructure failure, not a real test result).

Could a maintainer kick off another run when convenient? Both look like transient infra/flake on unrelated tests.

@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 7, 2026

Branch is now updated to current main (merge commit 8b920ec), which retriggers CI on the latest tree, so the prior unrelated ServerVirtualizationTest flake and Helix dead-letter should sort out cleanly on the new run.

@javiercn, @MackinnonBuck, @ilonatommy: when one of you has a moment, this is a small one-property addition to ValidationSummary (CssClass) matching the existing pattern already used on EditForm, InputBase and friends. Happy to address any feedback.

@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 8, 2026

Rebased on main once more. The failures on the prior run (8b920ec) were the same flaky unrelated tests: ServerVirtualizationTest.AnchorMode_End_AppendAfterLeavingBottom_DoesNotReengage in components-e2e, a Helix Subset 1 HTTP/2 interop timeout, and a Templates.Tests dead-letter, none touching the BindRenderTree / ValidationSummary CssClass surface. Could a maintainer trigger another run on the new commit, or let me know if there is a quarantine list I should reference? cc @javiercn @MackinnonBuck @ilonatommy

@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 8, 2026

Latest run on 8b920ec (build 1413865) failed Helix Subset 1 on the same two unrelated work items, identical to PR #66532's run that finished a few minutes earlier:

  • Interop.FunctionalTests--net11.0: HttpClientHttp2InteropTests.ServerReset_BeforeRequestBody_ClientBodyThrows(scheme: "http") [FAIL] — HTTP/2 interop, no overlap with the ValidationSummary surface.
  • Microsoft.AspNetCore.DataProtection.Extensions.Tests--net11.0: DataProtectionProviderTests.System_UsesProvidedCertificateNotFromStore [FAIL] with Interop+AppleCrypto+AppleCommonCryptoCryptographicException: The specified keychain could not be found. The queue is ubuntu.2404.amd64.open, so an Apple keychain failure is clearly environmental.

components-e2e is now green on the latest commit (the e2e fixture rework in 72cbda6 was the right fix), so this PR is on the same two flakes as the unrelated #66532. cc @javiercn @MackinnonBuck @ilonatommy — could one of you trigger another /azp run or point me at the quarantine entry to add for these on net11.0?

@oroztocil
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new CssClass parameter to the Blazor ValidationSummary component so callers can append custom CSS classes to the rendered <ul>’s default validation-errors class, and adds E2E coverage via a new BasicTestApp test component and test.

Changes:

  • Add CssClass parameter to ValidationSummary and combine it with AdditionalAttributes["class"] and the default validation-errors class during rendering.
  • Update public API baselines for the new parameter.
  • Add a new BasicTestApp test component and an E2E test validating the rendered class attribute.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Components/Web/src/PublicAPI.Unshipped.txt Adds the new public API entries for ValidationSummary.CssClass.
src/Components/Web/src/Forms/ValidationSummary.cs Introduces the CssClass parameter and updates class attribute rendering logic.
src/Components/test/testassets/BasicTestApp/Index.razor Adds a selector entry for the new test component.
src/Components/test/testassets/BasicTestApp/FormsTest/ValidationSummaryWithCssClassComponent.razor New test component used by E2E test coverage.
src/Components/test/E2ETest/Tests/FormsTest.cs Adds E2E test asserting ValidationSummary renders combined CSS classes.

Comment on lines +1 to +2
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Razor files should not have a license header.

Copy link
Copy Markdown
Member

@oroztocil oroztocil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @EduardF1! Thanks for the contribution.

I would suggest adding a test covering the interaction between CssClass and AdditionalAttributes["class"]. Either add a unit test in Microsoft.AspNetCore.Components.Web.Tests covering the combinations, or extend the E2E test fixture to render a third <ValidationSummary class="extra" CssClass="custom" /> and assert the resulting class="extra validation-errors custom".

Comment on lines +1 to +2
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Razor files should not have a license header.

@EduardF1
Copy link
Copy Markdown
Contributor Author

Update: the Daily Test Quarantine Management bot filed #66637 at 10:41Z tracking the exact System_UsesProvidedCertificateNotFromStore failure that hit this PR's run (build 1414758). The bot's investigation confirms 2 failures of this test on main itself in the last 30 days (builds 1414721 + 1413909), root-causes it as a Helix macOS keychain environment issue, and recommends quarantining.

Could a maintainer either land the quarantine via #66637 so this PR's CI clears, or given the infra-not-code root cause, consider merging despite that failing leg? cc @javiercn @MackinnonBuck @ilonatommy

(The quarantine bot's integrity filter blocked it from auto-linking this PR + #66532 to the issue, so #66637 isn't yet visibly tied to either.)

@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 10, 2026

Follow-up: still CI-clean as of this evening. All Helix legs SUCCESS, Build Analysis SUCCESS, only blocker is review. cc @javiercn @MackinnonBuck @ilonatommy whenever one of you has a moment.

@EduardF1
Copy link
Copy Markdown
Contributor Author

Still CI-clean as of 2026-05-12 (Helix + Build Analysis all green). The only remaining blocker is a review pass — would really appreciate a look when anyone has a moment. cc @javiercn @MackinnonBuck @ilonatommy

@Youssef1313
Copy link
Copy Markdown
Member

Hi @EduardF1. I see @oroztocil added already some review comments 3 days ago: #66531 (review). Were they addressed please?

Comment on lines +26 to +30
/// <summary>
/// Gets or sets an optional CSS class string appended to the <c>validation-errors</c>
/// CSS class on the rendered <c>ul</c> element.
/// </summary>
[Parameter] public string? CssClass { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oroztocil This seems like a new public API. Was it previously discussed/approved somewhere?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Youssef1313 After reviewing this PR, I noticed there was another PR (#66364) for the same issue which was posted few days earlier and fixes the problem in a more minimal manner. While I appreciate both contributions, we should have a discussion which way we want to take before we merge either.

@EduardF1
Copy link
Copy Markdown
Contributor Author

@Youssef1313 @oroztocil — thanks for the review and for flagging #66364. Both issues from the review are addressed in the latest commit:

  1. License header removed from ValidationSummaryWithCssClassComponent.razor — the // lines have been deleted (they would have rendered as DOM text nodes in Razor).
  2. Interaction test addedValidationSummaryAppliesCssClass now includes a third assertion covering the class="extra" CssClass="custom" combination, confirming the rendered output is "extra validation-errors custom".

On the two-PR situation: happy to defer to whichever approach the team prefers. That said, here is why I think this PR's approach is more robust than the minimal fix in #66364, in case it helps the decision:

#66364 fixes the bug where attribute-splatted class silently clobbers validation-errors. That is a genuine improvement. But users still have to know that class is the magic attribute to splat and that it will be merged rather than override.

This PR adds a dedicated CssClass parameter, which:

  • Is self-documenting and IDE-discoverable: developers see CssClass in IntelliSense alongside Model and AdditionalAttributes, with a clear XML-doc summary.
  • Eliminates attribute-splatting for the common case: <ValidationSummary CssClass="my-class" /> is cleaner than <ValidationSummary class="my-class" /> when all you want is to append a class, and it makes intent explicit.
  • Is consistent with other Blazor components: InputBase<T> and related components expose CssClass or equivalent named parameters for class customization rather than relying on users knowing the splatting contract.
  • Still supports combining with splatted class: as the new test demonstrates, both can be used simultaneously when needed.

The minimal fix in #66364 is strictly additive to this approach — the same CombineClassNames logic is in both. If the team would rather land #66364 first and revisit the CssClass parameter separately, that is completely reasonable. Either way, I will keep this PR updated.

@EduardF1
Copy link
Copy Markdown
Contributor Author

EduardF1 commented May 15, 2026

@Youssef1313 @oroztocil, following up on my May 13 reply. Both review points are addressed in the latest commit: the license header is removed from the .razor file, and an interaction test covering CssClass + AdditionalAttributes["class"] combined is in place. CI is still all-green (aspnetcore-ci SUCCESS, Build Analysis SUCCESS). Ready for re-review whenever either of you has a moment. Thank you!

@EduardF1
Copy link
Copy Markdown
Contributor Author

Hi @Youssef1313, friendly week-21 nudge. The Copilot review from 2026-05-10 was positive and the main aspnetcore-ci plus Build Analysis legs are green. The only red leg is aspnetcore-quarantined-pr (macOS), which looks like the keychain flake tracked in #66637. I do not have permission to re-trigger Azure Pipelines myself. Could you kick /azp run aspnetcore-quarantined-pr when you have a moment so we can clear the BLOCKED status? Thanks for your help.

@EduardF1
Copy link
Copy Markdown
Contributor Author

Thanks for the review @oroztocil and @Youssef1313.

The two review points are addressed in the most recent commits on this branch:

  • e940078 removes the // license header from ValidationSummaryWithCssClassComponent.razor (Razor files should not carry the C# license header) and adds a <ValidationSummary class="extra" CssClass="custom" /> instance to the fixture.
  • 5789637 adds the E2E assertion Browser.Equal("extra validation-errors custom", ...) covering the class + CssClass interaction in FormsTest.cs.

On the duplicate (#66364): the two PRs differ in scope. This one adds a dedicated CssClass parameter with E2E coverage; #66364 changes the existing class-merge logic with a unit test. Happy to defer to the area owners on which approach you prefer.

Could one of you re-run the CI via /azp run when you get a chance? The current failing aspnetcore-quarantined-pr check looks unrelated to the diff, and I do not have the perms to trigger /azp from my end. Happy to address any further feedback.

@ilonatommy
Copy link
Copy Markdown
Member

@oroztocil, I am not sure if we want to make a precedent here and add a CssClass public parameter. The fix could be purely internal, I agree with the point mentioned here: #43860 (comment) with one correction. NavLink doesn't have CssClass as public parameter, it's protected only. It makes this PR a truly first component to fix it this way.
I would suggest we should take a step back and finish the discussion on the original issue to agree on the implementation, then come back to making a PR. I can see 2 ways:

Copy link
Copy Markdown
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More discussion needed.

Address @ilonatommy's review feedback: instead of adding a new public
CssClass parameter (which would set a precedent — no other component
exposes this publicly), use the same internal pattern as InputRadio:

  builder.AddMultipleAttributes(AdditionalAttributes);
  builder.AddAttributeIfNotNullOrEmpty("class",
      AttributeUtilities.CombineClassNames(AdditionalAttributes, "validation-errors"));

This ensures:
- The fixed 'validation-errors' class is always present
- Users can add extra classes via AdditionalAttributes (class="pt-2")
- Both are combined correctly ("pt-2 validation-errors")
- No new public API surface

E2E test updated to verify:
1. Default renders with only 'validation-errors'
2. class="pt-2" via splatting combines to 'pt-2 validation-errors'
3. Other splatted attributes (data-testid) pass through correctly
@EduardF1
Copy link
Copy Markdown
Contributor Author

@ilonatommy Thank you for the review feedback — you're absolutely right that adding a public CssClass parameter would set an unwanted precedent.

I've pushed a revision that switches to the internal-only approach (option 1 from your comment), matching the InputRadio pattern:

csharp builder.OpenElement(0, "ul"); builder.AddMultipleAttributes(1, AdditionalAttributes); builder.AddAttributeIfNotNullOrEmpty(2, "class", AttributeUtilities.CombineClassNames(AdditionalAttributes, "validation-errors"));

What this does:

  • The fixed validation-errors class is always preserved (never overridden by splatting)
  • Users can add extra utility classes via normal attribute splatting: <ValidationSummary class="pt-2" /> → renders class="pt-2 validation-errors"
  • No new public API surface — removed CssClass from the component and the PublicAPI baseline

E2E test coverage:

  1. Default <ValidationSummary />class="validation-errors"
  2. <ValidationSummary class="pt-2" />class="pt-2 validation-errors"
  3. <ValidationSummary data-testid="my-summary" /> → non-class attributes pass through correctly

This directly addresses the original issue (#43860) without introducing any new parameters. Let me know if there's anything else you'd like adjusted.

@EduardF1 EduardF1 changed the title feat(blazor): add CssClass parameter to ValidationSummary component fix(blazor): preserve ValidationSummary fixed class when AdditionalAttributes contains class May 25, 2026
@javiercn
Copy link
Copy Markdown
Member

Just saw this PR. TL;DR I haven't looked at it, however I vaguely remember something in this area that we did. If I recall correctly Steve had some concerns at the time on something like this. We should dig this from the issue/pr history to make sure we account for it.

@EduardF1
Copy link
Copy Markdown
Contributor Author

Hi @oroztocil @ilonatommy, thanks for the feedback.

I see that #66364 addresses the same issue with a more minimal approach. I'm happy to defer to whichever direction the team prefers — if #66364 is the preferred path, feel free to close this one.

If you'd rather proceed with this PR, I can:

  1. Remove the license header from the Razor test file (already noted)
  2. Add an additional test case for the combination scenario (\class="extra" CssClass="custom"\ → \"extra validation-errors custom")

Just let me know which way to go and I'll adapt accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pass css class in <ValidationSummary /> to the generated <ul>

6 participants