From 989a2edaf246128e0aa33faa7d2c180d66161964 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Feb 2026 15:13:47 +0000 Subject: [PATCH 1/3] Initial plan From 6585dad08a1ee616df9148a88c6ff050f94101ce Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Feb 2026 16:04:15 +0000 Subject: [PATCH 2/3] Add checkbox model binding integration test Co-authored-by: gunndabad <2041280+gunndabad@users.noreply.github.com> --- .../TagHelpers/CheckboxesContext.cs | 2 +- .../TagHelpers/CheckboxesItemTagHelper.cs | 14 +++++-- .../TagHelpers/RadiosContext.cs | 2 +- .../Checkboxes.cshtml | 9 +++++ .../TagHelperModelBindingTests/Tests.cs | 37 +++++++++++++++++++ 5 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Checkboxes.cshtml diff --git a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesContext.cs b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesContext.cs index ef62a6ed..2b9f5f14 100644 --- a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesContext.cs +++ b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesContext.cs @@ -149,7 +149,7 @@ public override void SetErrorMessage( TemplateString? html, string tagName) { - if (Fieldset is not null) + if (Fieldset is not null && !_fieldsetIsOpen) { throw new InvalidOperationException($"<{tagName}> must be inside <{FieldsetTagName}>."); } diff --git a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs index a04798b5..70f54572 100644 --- a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs +++ b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs @@ -181,9 +181,17 @@ bool DoesModelMatchItemValue() if (modelExpression.Metadata.IsEnumerableType) { - var value = ViewContext!.ModelState.TryGetValue(modelExpression.Name, out var entry) ? - entry.RawValue : - model; + // Try to get the value from ModelState first, but fall back to model if ModelState doesn't have a valid enumerable value + var value = model; + + if (ViewContext!.ModelState.TryGetValue(modelExpression.Name, out var entry) && entry.RawValue != null) + { + // Only use RawValue if it looks like it's an enumerable collection + if (entry.RawValue is IEnumerable && !(entry.RawValue is string)) + { + value = entry.RawValue; + } + } var values = (value as IEnumerable)?.Cast(); return values?.Any(v => v?.ToString() == Value) is true; diff --git a/src/GovUk.Frontend.AspNetCore/TagHelpers/RadiosContext.cs b/src/GovUk.Frontend.AspNetCore/TagHelpers/RadiosContext.cs index 99d6e38d..0d9371b6 100644 --- a/src/GovUk.Frontend.AspNetCore/TagHelpers/RadiosContext.cs +++ b/src/GovUk.Frontend.AspNetCore/TagHelpers/RadiosContext.cs @@ -149,7 +149,7 @@ public override void SetErrorMessage( TemplateString? html, string tagName) { - if (Fieldset is not null) + if (Fieldset is not null && !_fieldsetIsOpen) { throw new InvalidOperationException($"<{tagName}> must be inside <{FieldsetTagName}>."); } diff --git a/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Checkboxes.cshtml b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Checkboxes.cshtml new file mode 100644 index 00000000..89715053 --- /dev/null +++ b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Checkboxes.cshtml @@ -0,0 +1,9 @@ +@model GovUk.Frontend.AspNetCore.IntegrationTests.TagHelperModelBindingTests.CheckboxesTestsModel + + + + + Option 1 + Option 2 + + diff --git a/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs index dfe95e92..0b3f105f 100644 --- a/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs +++ b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs @@ -347,6 +347,30 @@ public async Task SelectOverridden_RendersCorrectly() var errorMessage = page.Locator(".govuk-error-message").First; Assert.Equal("Error: Overridden error message", await errorMessage.TextContentAsync()); } + + [Fact] + public async Task Checkboxes_RendersCorrectly() + { + var page = await fixture.Browser!.NewPageAsync(); + await page.GotoAsync($"{ServerFixture.BaseUrl}/ModelBindingTests/Checkboxes"); + + var checkbox1 = page.Locator("input[value='option1']").First; + Assert.Equal("Options", await checkbox1.GetAttributeAsync("name")); + Assert.Equal("Options", await checkbox1.GetAttributeAsync("id")); + Assert.False(await checkbox1.IsCheckedAsync()); + + var checkbox2 = page.Locator("input[value='option2']").First; + Assert.Equal("Options", await checkbox2.GetAttributeAsync("name")); + Assert.Equal("Options-2", await checkbox2.GetAttributeAsync("id")); + // This is the critical assertion that should pass with the fix + Assert.True(await checkbox2.IsCheckedAsync()); + + var hint = page.Locator(".govuk-hint").First; + Assert.Equal("ModelMetadata description", await hint.InnerTextAsync()); + + var errorMessage = page.Locator(".govuk-error-message").First; + Assert.Equal("Error: Model error message", await errorMessage.TextContentAsync()); + } } [Route("/[controller]/[action]")] @@ -456,6 +480,13 @@ public IActionResult SelectOverridden() ModelState.AddModelError(nameof(SelectTestsModel.Option), "Model error message"); return View(new SelectTestsModel { Option = "option2" }); } + + [HttpGet] + public IActionResult Checkboxes() + { + ModelState.AddModelError(nameof(CheckboxesTestsModel.Options), "Model error message"); + return View(new CheckboxesTestsModel { Options = ["option2"] }); + } } public record TextInputTestsModel @@ -499,3 +530,9 @@ public record SelectTestsModel [Display(Name = "ModelMetadata display name", Description = "ModelMetadata description")] public string? Option { get; set; } } + +public record CheckboxesTestsModel +{ + [Display(Name = "ModelMetadata display name", Description = "ModelMetadata description")] + public List? Options { get; set; } +} From 46cf1242696e27b17a356320257f31afab018f6f Mon Sep 17 00:00:00 2001 From: James Gunn Date: Sat, 7 Feb 2026 09:21:37 +0000 Subject: [PATCH 3/3] Add test for overridden attributes --- .../CheckboxesErrorMessageTagHelper.cs | 2 +- .../TagHelpers/CheckboxesItemTagHelper.cs | 14 ++---- .../TagHelpers/CheckboxesTagHelper.cs | 4 +- .../CheckboxesOverridden.cshtml | 13 +++++ .../TagHelperModelBindingTests/Tests.cs | 50 ++++++++++++++++++- 5 files changed, 68 insertions(+), 15 deletions(-) create mode 100644 tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/CheckboxesOverridden.cshtml diff --git a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesErrorMessageTagHelper.cs b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesErrorMessageTagHelper.cs index fc033290..a58751ac 100644 --- a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesErrorMessageTagHelper.cs +++ b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesErrorMessageTagHelper.cs @@ -33,7 +33,7 @@ private protected override void SetErrorMessage(TagHelperContent? content, TagHe var attributes = new AttributeCollection(output.Attributes); checkboxesContext.SetErrorMessage( - VisuallyHiddenText, + VisuallyHiddenText is not null ? new TemplateString(VisuallyHiddenText) : null, attributes, content?.ToTemplateString(), output.TagName); diff --git a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs index 70f54572..a04798b5 100644 --- a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs +++ b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesItemTagHelper.cs @@ -181,17 +181,9 @@ bool DoesModelMatchItemValue() if (modelExpression.Metadata.IsEnumerableType) { - // Try to get the value from ModelState first, but fall back to model if ModelState doesn't have a valid enumerable value - var value = model; - - if (ViewContext!.ModelState.TryGetValue(modelExpression.Name, out var entry) && entry.RawValue != null) - { - // Only use RawValue if it looks like it's an enumerable collection - if (entry.RawValue is IEnumerable && !(entry.RawValue is string)) - { - value = entry.RawValue; - } - } + var value = ViewContext!.ModelState.TryGetValue(modelExpression.Name, out var entry) ? + entry.RawValue : + model; var values = (value as IEnumerable)?.Cast(); return values?.Any(v => v?.ToString() == Value) is true; diff --git a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesTagHelper.cs b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesTagHelper.cs index bb2cc24e..4b63d392 100644 --- a/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesTagHelper.cs +++ b/src/GovUk.Frontend.AspNetCore/TagHelpers/CheckboxesTagHelper.cs @@ -107,7 +107,9 @@ internal CheckboxesTagHelper(IComponentGenerator componentGenerator, IModelHelpe /// public override void Init(TagHelperContext context) { - context.SetContextItem(new CheckboxesContext(Name, For)); + var checkboxesContext = new CheckboxesContext(Name, For); + context.SetContextItem(checkboxesContext); + context.SetContextItem(checkboxesContext); } /// diff --git a/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/CheckboxesOverridden.cshtml b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/CheckboxesOverridden.cshtml new file mode 100644 index 00000000..5bcd163c --- /dev/null +++ b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/CheckboxesOverridden.cshtml @@ -0,0 +1,13 @@ +@model GovUk.Frontend.AspNetCore.IntegrationTests.TagHelperModelBindingTests.CheckboxesTestsModel + + + + + Overridden legend + + Overridden hint + Overridden error message + Option 1 + Option 2 + + diff --git a/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs index 0b3f105f..f6ee238c 100644 --- a/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs +++ b/tests/GovUk.Frontend.AspNetCore.IntegrationTests/TagHelperModelBindingTests/Tests.cs @@ -362,7 +362,6 @@ public async Task Checkboxes_RendersCorrectly() var checkbox2 = page.Locator("input[value='option2']").First; Assert.Equal("Options", await checkbox2.GetAttributeAsync("name")); Assert.Equal("Options-2", await checkbox2.GetAttributeAsync("id")); - // This is the critical assertion that should pass with the fix Assert.True(await checkbox2.IsCheckedAsync()); var hint = page.Locator(".govuk-hint").First; @@ -371,6 +370,29 @@ public async Task Checkboxes_RendersCorrectly() var errorMessage = page.Locator(".govuk-error-message").First; Assert.Equal("Error: Model error message", await errorMessage.TextContentAsync()); } + + [Fact] + public async Task CheckboxesOverridden_RendersCorrectly() + { + var page = await fixture.Browser!.NewPageAsync(); + await page.GotoAsync($"{ServerFixture.BaseUrl}/ModelBindingTests/CheckboxesOverridden"); + + var checkbox1 = page.Locator("input[value='option1']").First; + Assert.Equal("OverriddenName", await checkbox1.GetAttributeAsync("name")); + Assert.Equal("OverriddenIdPrefix", await checkbox1.GetAttributeAsync("id")); + Assert.True(await checkbox1.IsCheckedAsync()); + + var checkbox2 = page.Locator("input[value='option2']").First; + Assert.Equal("OverriddenName", await checkbox2.GetAttributeAsync("name")); + Assert.Equal("OverriddenIdPrefix-2", await checkbox2.GetAttributeAsync("id")); + Assert.False(await checkbox2.IsCheckedAsync()); + + var hint = page.Locator(".govuk-hint").First; + Assert.Equal("Overridden hint", await hint.InnerTextAsync()); + + var errorMessage = page.Locator(".govuk-error-message").First; + Assert.Equal("Error: Overridden error message", await errorMessage.TextContentAsync()); + } } [Route("/[controller]/[action]")] @@ -379,6 +401,7 @@ public class ModelBindingTestsController : Controller [HttpGet] public IActionResult TextInput() { + ModelState.SetModelValue(nameof(TextInputTestsModel.Text), "Model value", "Model value"); ModelState.AddModelError(nameof(TextInputTestsModel.Text), "Model error message"); return View(new TextInputTestsModel { Text = "Model value" }); } @@ -386,6 +409,7 @@ public IActionResult TextInput() [HttpGet] public IActionResult TextInputOverridden() { + ModelState.SetModelValue(nameof(TextInputTestsModel.Text), "Model value", "Model value"); ModelState.AddModelError(nameof(TextInputTestsModel.Text), "Model error message"); return View(new TextInputTestsModel { Text = "Model value" }); } @@ -393,6 +417,7 @@ public IActionResult TextInputOverridden() [HttpGet] public IActionResult PasswordInput() { + ModelState.SetModelValue(nameof(PasswordInputTestsModel.Password), "Model value", "Model value"); ModelState.AddModelError(nameof(PasswordInputTestsModel.Password), "Model error message"); return View(new PasswordInputTestsModel { Password = "Model value" }); } @@ -400,6 +425,7 @@ public IActionResult PasswordInput() [HttpGet] public IActionResult PasswordInputOverridden() { + ModelState.SetModelValue(nameof(PasswordInputTestsModel.Password), "Model value", "Model value"); ModelState.AddModelError(nameof(PasswordInputTestsModel.Password), "Model error message"); return View(new PasswordInputTestsModel { Password = "Model value" }); } @@ -407,6 +433,7 @@ public IActionResult PasswordInputOverridden() [HttpGet] public IActionResult DateInput() { + ModelState.SetModelValue(nameof(DateInputTestsModel.Date), new DateOnly(2024, 3, 15), "15,3,2024"); ModelState.AddModelError(nameof(DateInputTestsModel.Date), "Model error message"); return View(new DateInputTestsModel { Date = new DateOnly(2024, 3, 15) }); } @@ -414,6 +441,7 @@ public IActionResult DateInput() [HttpGet] public IActionResult DateInputOverridden() { + ModelState.SetModelValue(nameof(DateInputTestsModel.Date), new DateOnly(2024, 3, 15), "15,3,2024"); ModelState.AddModelError(nameof(DateInputTestsModel.Date), "Model error message"); return View(new DateInputTestsModel { Date = new DateOnly(2024, 3, 15) }); } @@ -421,6 +449,7 @@ public IActionResult DateInputOverridden() [HttpGet] public IActionResult DateInputOverriddenItems() { + ModelState.SetModelValue(nameof(DateInputTestsModel.Date), new DateOnly(2024, 3, 15), "15,3,2024"); ModelState.AddModelError(nameof(DateInputTestsModel.Date), "Model error message"); return View(new DateInputTestsModel { Date = new DateOnly(2024, 3, 15) }); } @@ -428,6 +457,7 @@ public IActionResult DateInputOverriddenItems() [HttpGet] public IActionResult Textarea() { + ModelState.SetModelValue(nameof(TextareaTestsModel.Text), "Model value", "Model value"); ModelState.AddModelError(nameof(TextareaTestsModel.Text), "Model error message"); return View(new TextareaTestsModel { Text = "Model value" }); } @@ -435,6 +465,7 @@ public IActionResult Textarea() [HttpGet] public IActionResult TextareaOverridden() { + ModelState.SetModelValue(nameof(TextareaTestsModel.Text), "Model value", "Model value"); ModelState.AddModelError(nameof(TextareaTestsModel.Text), "Model error message"); return View(new TextareaTestsModel { Text = "Model value" }); } @@ -442,6 +473,7 @@ public IActionResult TextareaOverridden() [HttpGet] public IActionResult CharacterCount() { + ModelState.SetModelValue(nameof(CharacterCountTestsModel.Text), "Model value", "Model value"); ModelState.AddModelError(nameof(CharacterCountTestsModel.Text), "Model error message"); return View(new CharacterCountTestsModel { Text = "Model value" }); } @@ -449,6 +481,7 @@ public IActionResult CharacterCount() [HttpGet] public IActionResult CharacterCountOverridden() { + ModelState.SetModelValue(nameof(CharacterCountTestsModel.Text), "Model value", "Model value"); ModelState.AddModelError(nameof(CharacterCountTestsModel.Text), "Model error message"); return View(new CharacterCountTestsModel { Text = "Model value" }); } @@ -456,6 +489,7 @@ public IActionResult CharacterCountOverridden() [HttpGet] public IActionResult FileUpload() { + ModelState.SetModelValue(nameof(FileUploadTestsModel.File), "", ""); ModelState.AddModelError(nameof(FileUploadTestsModel.File), "Model error message"); return View(new FileUploadTestsModel()); } @@ -463,6 +497,7 @@ public IActionResult FileUpload() [HttpGet] public IActionResult FileUploadOverridden() { + ModelState.SetModelValue(nameof(FileUploadTestsModel.File), "", ""); ModelState.AddModelError(nameof(FileUploadTestsModel.File), "Model error message"); return View(new FileUploadTestsModel()); } @@ -470,6 +505,7 @@ public IActionResult FileUploadOverridden() [HttpGet] public IActionResult Select() { + ModelState.SetModelValue(nameof(SelectTestsModel.Option), "option2", "option2"); ModelState.AddModelError(nameof(SelectTestsModel.Option), "Model error message"); return View(new SelectTestsModel { Option = "option2" }); } @@ -477,6 +513,7 @@ public IActionResult Select() [HttpGet] public IActionResult SelectOverridden() { + ModelState.SetModelValue(nameof(SelectTestsModel.Option), "option2", "option2"); ModelState.AddModelError(nameof(SelectTestsModel.Option), "Model error message"); return View(new SelectTestsModel { Option = "option2" }); } @@ -484,6 +521,15 @@ public IActionResult SelectOverridden() [HttpGet] public IActionResult Checkboxes() { + ModelState.SetModelValue(nameof(CheckboxesTestsModel.Options), new[] { "option2" }, null); + ModelState.AddModelError(nameof(CheckboxesTestsModel.Options), "Model error message"); + return View(new CheckboxesTestsModel { Options = ["option2"] }); + } + + [HttpGet] + public IActionResult CheckboxesOverridden() + { + ModelState.SetModelValue(nameof(CheckboxesTestsModel.Options), new[] { "option2" }, null); ModelState.AddModelError(nameof(CheckboxesTestsModel.Options), "Model error message"); return View(new CheckboxesTestsModel { Options = ["option2"] }); } @@ -522,7 +568,7 @@ public record CharacterCountTestsModel public record FileUploadTestsModel { [Display(Name = "ModelMetadata display name", Description = "ModelMetadata description")] - public Microsoft.AspNetCore.Http.IFormFile? File { get; set; } + public IFormFile? File { get; set; } } public record SelectTestsModel