From c405883c9044e0bf1bc896d58bddde1f4b1dea25 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Mon, 14 Sep 2026 16:32:52 +0100 Subject: [PATCH 01/16] Give the search widget a search target and an instant option The widget could only search the whole site or a path prefix, and always navigated to a results page. It now carries two independent properties: - searchIn - the whole site, a section of it, or the page the widget sits on. "This page" resolves to the current request path, so a submit with no JavaScript still lands on a search of just that page. - instant - marks the form for progressive enhancement. The markup is unchanged either way; the marker attributes and the script only appear when it is on. Widgets already placed carry no searchIn, so an existing scope still reads as a section and an empty one as the whole site. No migration. Refs #426 AB#303114 --- .../ContentPages/WidgetRegistry.cs | 2 +- .../Views/ContentPage/Edit.cshtml | 23 +++++ .../ContentPages/Widgets/_Search.cshtml | 39 +++++++- .../Shared/ContentPages/_EditWidget.cshtml | 86 +++++++++++++----- .../SearchWidgetRenderContractTests.cs | 90 +++++++++++++++++++ .../ContentPages/WidgetEditorContractTests.cs | 34 +++++++ .../ContentPages/WidgetPropsBuilderTests.cs | 34 +++++++ .../ContentPages/WidgetRegistryTests.cs | 30 +++++++ 8 files changed, 311 insertions(+), 27 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs b/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs index 1e6d5e963..158841b26 100644 --- a/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs +++ b/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs @@ -15,7 +15,7 @@ public static class WidgetRegistry new("card", "Card", ContributesToNav: false, """{"title":"","body":"","href":""}"""), new("summarylist", "Summary list", ContributesToNav: false, """{"rows":[]}"""), new("published", "Published callout", ContributesToNav: false, """{"text":""}"""), - new("search", "Search", ContributesToNav: false, """{"label":"Search","placeholder":"","action":"/search","buttonText":"Search","scope":""}"""), + new("search", "Search", ContributesToNav: false, """{"label":"Search","placeholder":"","action":"/search","buttonText":"Search","scope":"","searchIn":"site","instant":"false","noResultsText":"No results found"}"""), new("results", "Search results", ContributesToNav: false, """{"scope":"","emptyText":"No results found."}"""), new("pagenav", "Page navigation", ContributesToNav: false, """{"mode":"headings","childrenParentPath":"","showSearch":false,"searchPath":"","searchLabel":"Search"}""") ]; diff --git a/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml b/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml index c9dac7d4b..0a560c2d7 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml @@ -418,6 +418,29 @@ })(); +@* Live toggling for the Search widget editor: the scope path only applies to a section of the + site, and the no-results copy only to instant search. *@ + + @* URL-change confirmation. When the segment field differs from the value the page loaded with, the outer form's submit is cancelled, the current title / subtitle / page name / segment are copied into the modal's hidden inputs, and the govuk-confirm-modal is opened. Title-only diff --git a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_Search.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_Search.cshtml index f4120e3af..ec3248a5a 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_Search.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_Search.cshtml @@ -6,10 +6,39 @@ var action = Model.GetString("action") ?? string.Empty; var buttonText = Model.GetString("buttonText") ?? "Search"; // Optional scope: narrows results to pages under a path prefix (matches /search's ?scope=). - var scope = (Model.GetString("scope") ?? string.Empty).Trim().Trim('/'); - var inputId = string.IsNullOrEmpty(Model.Anchor) ? "search" : $"search-{Model.Anchor}"; + var scopeProp = (Model.GetString("scope") ?? string.Empty).Trim().Trim('/'); + + // Where the widget searches: the whole site, one section of it, or the page it sits on. + var searchIn = (Model.GetString("searchIn") ?? string.Empty).Trim().ToLowerInvariant(); + // Widgets placed before this prop existed carry only a scope, so a scope they already set still + // means a section and an empty one still means the whole site. Nothing needs migrating. + if (searchIn.Length == 0) searchIn = scopeProp.Length > 0 ? "path" : "site"; + // An unrecognised value searches the whole site rather than rendering a box that does nothing. + if (searchIn != "path" && searchIn != "page") searchIn = "site"; + + // The scope that travels with the form. "This page" resolves to the page being viewed, which is + // what lets a no-JS submit still land on a search of just this page. + var currentPath = (ViewContext.HttpContext.Request.Path.Value ?? string.Empty).Trim('/'); + var scope = searchIn switch + { + "path" => scopeProp, + "page" => currentPath, + _ => string.Empty + }; + + // Instant search is an enhancement over the form below, never a replacement for it: the markup + // is identical either way, and the script only binds when the marker attribute is present. + var instant = Model.GetBool("instant") ?? false; + var noResultsText = Model.GetString("noResultsText") ?? "No results found"; + var inputId = string.IsNullOrEmpty(Model.Anchor) ? "search" : $"search-{Model.Anchor}"; } - + } @if (Model.ShowAggregateToggle) { diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Analytics/SearchAnalyticsSurfaceSourceTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Analytics/SearchAnalyticsSurfaceSourceTests.cs index ec540a0a7..edaf25d93 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Analytics/SearchAnalyticsSurfaceSourceTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Analytics/SearchAnalyticsSurfaceSourceTests.cs @@ -35,7 +35,7 @@ public void TheSurfaceParameterIsBoundInOnePlace() // missing it. var bindings = Regex.Matches(Source, @"NpgsqlParameter\(""surfaces"""); - Assert.Equal(1, bindings.Count); + Assert.Single(bindings); } private static string ReadQueryService() From 6d9e22b09b74f580bc847cb9e43666896e2481ec Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Tue, 15 Sep 2026 10:12:18 +0100 Subject: [PATCH 10/16] Name the search surfaces so the instant pair reads as a pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Instant search" and "Single-page search" read as two unrelated features, so someone who used the instant search on a page ticked "Instant search" and filtered their own searches out of the dashboard. They are now "Instant — the whole site or a section" and "Instant — this page only". The single-page section also warns when the surface filter excludes it, rather than reporting that nothing was searched for. An empty table because of a filter and an empty table because nobody searched are different facts and a reader could not tell them apart. Refs #426 AB#303114 --- .../Views/Admin/Search/OnPage.cshtml | 20 +++++++++++++++++-- .../Admin/Search/_TimeWindowFilterForm.cshtml | 15 +++++++++----- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/OnPage.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/OnPage.cshtml index b004fa118..ffb9d5761 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/OnPage.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/OnPage.cshtml @@ -1,5 +1,7 @@ @using System.Globalization +@using DfE.CheckPerformanceData.Application.Analytics @model DfE.CheckPerformanceData.Web.Controllers.ViewModels.OnPageSearchViewModel +@inject ISearchSurfaceFilter SurfaceFilter @{ var isDrillIn = !string.IsNullOrWhiteSpace(Model.HostPath); ViewData["Title"] = isDrillIn ? "Single-page search: " + Model.HostPath : "Single-page search"; @@ -62,7 +64,21 @@ else ShowBucketSize = false, }) -@if (Model.TotalCount == 0) +@* Every row on this page is an on-page search, so a surface filter that excludes them + empties the table. Saying "nothing was searched for" in that case would be a lie, and the + reader has no way to tell the difference. *@ +@if (!SurfaceFilter.Surfaces.Contains(SearchSurfaces.InstantPage)) +{ +
+ + + Warning + The search surface filter currently excludes on-page searches, so this section has + nothing to show. Tick Instant — this page only above to see it. + +
+} +else if (Model.TotalCount == 0) {

@(isDrillIn @@ -70,7 +86,7 @@ else : "No on-page searches recorded in this window.")

} -else +else if (SurfaceFilter.Surfaces.Contains(SearchSurfaces.InstantPage)) {

Showing @firstRow to @lastRow of @Model.TotalCount. diff --git a/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/_TimeWindowFilterForm.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/_TimeWindowFilterForm.cshtml index 2bac0b715..8fffbe20e 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/_TimeWindowFilterForm.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Admin/Search/_TimeWindowFilterForm.cshtml @@ -113,15 +113,20 @@

Search surface
- Leave all ticked to count every search. A submitted search is one someone pressed - the button for; an instant search is a typeahead they settled on. + Leave all ticked to count every search. Two of these are instant search — the + typeahead — and they are recorded separately because searching a whole section + of the site and searching the page you are reading are different acts.
@foreach (var (value, label) in new[] { - (SearchSurfaces.Site, "Submitted search"), - (SearchSurfaces.Instant, "Instant search"), - (SearchSurfaces.InstantPage, "Single-page search"), + @* Named so the family is visible. The first labelling called these + "Instant search" and "Single-page search", which reads as two unrelated + features — so someone who used the instant search on a page ticked + "Instant search" and their own searches were filtered out. *@ + (SearchSurfaces.Site, "Submitted — someone pressed the button"), + (SearchSurfaces.Instant, "Instant — the whole site or a section"), + (SearchSurfaces.InstantPage, "Instant — this page only"), }) { var id = "surface-" + value; From f190090b362a3633d2581df0a74ac202fba77aa0 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Tue, 15 Sep 2026 10:22:28 +0100 Subject: [PATCH 11/16] Cover abandoning a search by leaving the page The report for that case is sent with sendBeacon while the page unloads, so route interception is torn down before it can see it and the test has to read the row back from the dashboard instead. Verified by hand first against a real navigation: the row lands with the right host page and no selection. The single-page section test also asserted its page appeared in the paged list, which only held while that list was short - it addresses the terms drill-in by path now, and checks the list renders rows separately. Refs #426 AB#303114 --- .../Admin/OnPageSearchSectionTests.cs | 19 +++++--- .../InstantSearchReportingE2ETests.cs | 45 +++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs index 4c1aafbe4..a35e726a7 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs @@ -78,21 +78,26 @@ await CmsSeedHelpers.UpdateWidgetAsync(Fixture.SeedClient, id, "0.1", "heading", var adminCookie = await AuthHelpers.ImpersonateAsAdminAsync(Fixture); AttachCookieToContext(adminCookie); + // The terms drill-in is addressed by path, so this does not depend on where the + // page happens to land in a paged list that every other test is also writing to. + var drillIn = $"{Fixture.BaseUrl}/admin/Search/OnPage?path={Uri.EscapeDataString(hostPath)}&range=24h"; + // The sink drains on a timer. for (var attempt = 0; attempt < 30; attempt++) { - await Page.GotoAsync($"{Fixture.BaseUrl}/admin/Search/OnPage?range=24h"); - if (await Page.Locator($"a:has-text('{hostPath}')").CountAsync() > 0) break; + await Page.GotoAsync(drillIn); + if ((await Page.Locator("body").InnerTextAsync()).Contains(term, StringComparison.Ordinal)) break; await Page.WaitForTimeoutAsync(500); } - var pageLink = Page.Locator($"a:has-text('{hostPath}')").First; - await Expect(pageLink).ToBeVisibleAsync(); - - // Drill in: the terms searched on that page. - await pageLink.ClickAsync(); await Expect(Page.Locator("h1")).ToContainTextAsync("Single-page search"); await Expect(Page.Locator("#sa-onpage-table")).ToContainTextAsync(term); + + // And the list view renders pages with rows in it, each linking to its own drill-in. + await Page.GotoAsync($"{Fixture.BaseUrl}/admin/Search/OnPage?range=24h"); + await Expect(Page.Locator("#sa-onpage-table")).ToBeVisibleAsync(); + Assert.True(await Page.Locator("#sa-onpage-table tbody tr").CountAsync() > 0); + Assert.True(await Page.Locator("#sa-onpage-table tbody a[href*='OnPage?path=']").CountAsync() > 0); } finally { diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs index de5b371d8..2470f44ff 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs @@ -176,6 +176,51 @@ public async Task AQueryAbandonedForADifferentOne_IsReportedWithNoSelection() } } + // ============================================================ + // 2b. Abandoning by navigating away, without blurring the box or picking anything. The + // most ordinary way someone gives up on a search. + // + // Asserted through the dashboard rather than at the network boundary: this report is + // sent with sendBeacon while the page unloads, and route interception is torn down + // with the page before it can see it. The claim worth testing is that the row arrives + // anyway, which is what the admin surface answers. + // ============================================================ + [Fact] + public async Task AQueryAbandonedByLeavingThePage_IsStillReported() + { + var term = "cypdnav" + Guid.NewGuid().ToString("N")[..10].ToLowerInvariant(); + var url = await SeedPageAsync("page"); + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + + await Input.ClickAsync(); + await Input.FillAsync(term); + await Expect(Page.Locator(".autocomplete__menu")).ToContainTextAsync("No matches"); + + // Leave, without blurring the box or choosing anything. + await Page.GotoAsync($"{Fixture.BaseUrl}/guidance"); + + try + { + var adminCookie = await AuthHelpers.ImpersonateAsAdminAsync(Fixture); + AttachCookieToContext(adminCookie); + + var drillIn = $"{Fixture.BaseUrl}/admin/Search/OnPage?path={Uri.EscapeDataString(url)}&range=24h"; + for (var attempt = 0; attempt < 30; attempt++) + { + await Page.GotoAsync(drillIn); + var body = await Page.Locator("body").InnerTextAsync(); + if (body.Contains(term, StringComparison.Ordinal)) return; + await Page.WaitForTimeoutAsync(500); + } + + Assert.Fail($"Abandoned query '{term}' never reached the dashboard for {url}."); + } + finally + { + await AuthHelpers.ImpersonateAsEditorAsync(Fixture); + } + } + // ============================================================ // 3. What was shown, and what was chosen, both reach the report. // ============================================================ From fbb4196592f7e7e15b9d0b99926abe982751303c Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Tue, 15 Sep 2026 11:27:59 +0100 Subject: [PATCH 12/16] Take the page-contents nav down to H4, and show its nesting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nav listed H2 and H3 only, so every H4 on a page was unreachable from it. H4 now nests under the most recent H3. Levels fall back rather than being dropped: an H4 with no H3 above it attaches to the current H2, and a heading with nothing above it at all becomes top-level. Authors skip levels — several of the guidance pages go straight from their title to H3s — and dropping those headings would leave sections the nav cannot reach, which is the job it exists to do. A new H2 resets the branch so a later section's H4 cannot attach to the previous section's last H3. The list markup recurses now instead of spelling out two levels, so the depth is the nav builder's decision rather than the view's. moj-side-navigation gives every nested list zero margin and padding, so until now the nesting existed only in the markup: an H3 sat at the same left edge, in the same size, as the H2 it belonged to. Each level is indented one step and set a size smaller. This changes how the contents nav looks on pages that already use it — previously flat lists will now read as the hierarchies they always were. AB#303114 --- .../ContentPages/ContentNavBuilder.cs | 44 +++++-- .../Views/Shared/ContentPages/_SideNav.cshtml | 36 +----- .../Shared/ContentPages/_SideNavList.cshtml | 31 +++++ .../wwwroot/css/site.css | 16 +++ .../ContentPages/ContentNavBuilderTests.cs | 109 +++++++++++++++++- 5 files changed, 190 insertions(+), 46 deletions(-) create mode 100644 src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNavList.cshtml diff --git a/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs b/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs index 8e8d20398..feb63b38a 100644 --- a/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs +++ b/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs @@ -1,14 +1,23 @@ namespace DfE.CheckPerformanceData.Application.ContentPages; // Builds a page's left-hand nav by walking the content tree (depth-first, document order) for -// Heading widgets. H2 → top-level item; H3 → nested under the most recent H2 (an H3 before any H2 -// falls back to top-level). Other widgets and other heading levels (H1, H4–H6) do not contribute. +// Heading widgets. H2 → top-level item; H3 → nested under the most recent H2; H4 → nested under +// the most recent H3. +// +// Authors skip levels, so each level falls back to the nearest one that exists rather than being +// dropped: an H4 with no H3 above it attaches to the current H2, and a heading with nothing above +// it at all becomes top-level. Dropping it instead would leave a section of the page that the +// nav cannot reach, which is the whole job of this list. +// +// H1 is the page title and would duplicate the heading above the nav; H5 and H6 are below the +// depth a contents list stays readable at. Neither contributes. public static class ContentNavBuilder { public static IReadOnlyList Build(IReadOnlyList tree) { var top = new List(); MutableItem? currentH2 = null; + MutableItem? currentH3 = null; foreach (var heading in Walk(tree)) { @@ -16,17 +25,30 @@ public static IReadOnlyList Build(IReadOnlyList tre if (text is null || heading.Anchor is null) continue; var item = new MutableItem(text, $"#{heading.Anchor}"); - if (level == 2) + switch (level) { - top.Add(item); - currentH2 = item; - } - else if (level == 3) - { - if (currentH2 is null) top.Add(item); - else currentH2.Children.Add(item); + case 2: + top.Add(item); + currentH2 = item; + // A new section starts a fresh branch: without this reset an H4 under the + // new H2 would attach to the previous section's last H3. + currentH3 = null; + break; + + case 3: + if (currentH2 is null) top.Add(item); + else currentH2.Children.Add(item); + currentH3 = item; + break; + + case 4: + if (currentH3 is not null) currentH3.Children.Add(item); + else if (currentH2 is not null) currentH2.Children.Add(item); + else top.Add(item); + break; + + // Other levels do not appear in the nav. } - // Other levels do not appear in the nav. } return top.Select(Freeze).ToList(); diff --git a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNav.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNav.cshtml index fdf825b0a..92d65d800 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNav.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNav.cshtml @@ -1,40 +1,14 @@ @using DfE.CheckPerformanceData.Application.ContentPages @model IReadOnlyList @* - Page-side navigation used by the PageNav widget in both "headings" (auto H2/H3 of the - current page) and "children" (a sibling/child page list) modes. Marks the entry whose - Href matches the current request path as active, which triggers the blue left bar via - moj-side-navigation__item--active. + Page-side navigation used by the PageNav widget in both "headings" (auto H2/H3/H4 of the + current page) and "children" (a sibling/child page list) modes. The list itself is rendered + by _SideNavList, which recurses, so nesting depth is decided by the nav builder rather than + by how many levels this markup happens to spell out. *@ -@{ - var currentPath = Context.Request.Path.Value ?? string.Empty; - bool IsActive(string href) => - !string.IsNullOrEmpty(href) && !href.StartsWith('#') - && string.Equals(href.TrimEnd('/'), currentPath.TrimEnd('/'), StringComparison.OrdinalIgnoreCase); -} @if (Model.Count > 0) { } diff --git a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNavList.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNavList.cshtml new file mode 100644 index 000000000..168858a28 --- /dev/null +++ b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_SideNavList.cshtml @@ -0,0 +1,31 @@ +@using DfE.CheckPerformanceData.Application.ContentPages +@model IReadOnlyList +@* + One level of the page-side navigation, rendered recursively so the markup does not need + changing each time the nav learns about another heading level. Marks the entry whose Href + matches the current request path as active, which triggers the blue left bar via + moj-side-navigation__item--active; in-page anchors never match, so a headings nav has no + active item. +*@ +@{ + var currentPath = Context.Request.Path.Value ?? string.Empty; + bool IsActive(string href) => + !string.IsNullOrEmpty(href) && !href.StartsWith('#') + && string.Equals(href.TrimEnd('/'), currentPath.TrimEnd('/'), StringComparison.OrdinalIgnoreCase); +} +@if (Model.Count > 0) +{ +
    + @foreach (var item in Model) + { + var itemActive = IsActive(item.Href); +
  • + @item.Text + @if (item.Children.Count > 0) + { + @await Html.PartialAsync("ContentPages/_SideNavList", item.Children) + } +
    • + } +
+} diff --git a/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css b/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css index c32f5bc88..150b9c8dd 100644 --- a/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css +++ b/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css @@ -1771,6 +1771,22 @@ body:has(.cpb-breadcrumbs) main.govuk-main-wrapper { padding-top: 0 !important; padding-top: 30px; } +/* Page-contents nav: show the nesting. + moj-side-navigation sets margin and padding to zero on every list at every depth, so a + nested item sits at exactly the same left edge as its parent and the hierarchy exists only + in the markup. One step of indent per level, and a smaller face as it goes deeper, so a + reader can see which section a sub-heading belongs to. The sizes are the GDS scale's 16px + and 14px steps down from the 19px the nav uses at the top level. */ +.moj-side-navigation__list .moj-side-navigation__list { + padding-left: 15px; +} +.moj-side-navigation__list .moj-side-navigation__list a { + font-size: 16px; +} +.moj-side-navigation__list .moj-side-navigation__list .moj-side-navigation__list a { + font-size: 14px; +} + /* Landing-page data-check sections: visible rule directly under the heading. */ .cypmd-landing-section { margin-bottom: 40px; } .cypmd-landing-section__rule { margin-top: 5px; margin-bottom: 20px; } diff --git a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs index abe2394a8..a39fe0e0d 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs @@ -4,8 +4,9 @@ namespace DfE.CheckPerformanceData.Application.UnitTests.ContentPages; // The left-hand nav is built automatically by walking the content tree for Heading widgets, in -// document order: H2 → top-level item, H3 → nested under the most recent H2. Editors never hand -// maintain it. Headings nested inside regions are found just the same. +// document order: H2 → top-level item, H3 → nested under the most recent H2, H4 → nested under +// the most recent H3. Editors never hand maintain it. Headings nested inside regions are found +// just the same. public class ContentNavBuilderTests { private static WidgetNode Heading(int level, string text, string anchor) => new() @@ -78,20 +79,120 @@ public void HeadingsInsideNestedRegions_AreFound_InDocumentOrder() } [Fact] - public void NonHeadingWidgets_AndNonH2H3Levels_AreIgnored() + public void NonHeadingWidgets_AndLevelsOutsideH2ToH4_AreIgnored() { + // H1 is the page title and would duplicate the heading above the nav; H5 and H6 are + // below the depth a contents list stays readable at. IReadOnlyList tree = [ Heading(1, "Page title", "page-title"), new WidgetNode { Type = "divider" }, Heading(2, "Real section", "real-section"), - Heading(4, "Too deep", "too-deep") + Heading(5, "Too deep", "too-deep"), + Heading(6, "Deeper still", "deeper-still") ]; var nav = ContentNavBuilder.Build(tree); Assert.Single(nav); Assert.Equal("real-section", Trim(nav[0].Href)); + Assert.Empty(nav[0].Children); + } + + [Fact] + public void H4s_NestUnderThePrecedingH3() + { + IReadOnlyList tree = + [ + Heading(2, "Removing a pupil", "removing-a-pupil"), + Heading(3, "Reasons for removal", "reasons-for-removal"), + Heading(4, "Admitted following permanent exclusion", "admitted-exclusion"), + Heading(4, "Admitted from abroad", "admitted-abroad"), + Heading(3, "Evidence", "evidence") + ]; + + var nav = ContentNavBuilder.Build(tree); + + var section = Assert.Single(nav); + Assert.Equal(2, section.Children.Count); + + var reasons = section.Children[0]; + Assert.Equal("reasons-for-removal", Trim(reasons.Href)); + Assert.Equal( + ["admitted-exclusion", "admitted-abroad"], + reasons.Children.Select(c => Trim(c.Href))); + + // The following H3 starts a fresh branch rather than collecting the previous H4s. + Assert.Empty(section.Children[1].Children); + } + + [Fact] + public void AnH4WithNoPrecedingH3_NestsUnderTheH2() + { + // Authors skip levels. Dropping the heading would leave a section the nav cannot reach, + // so it attaches at the nearest level that exists. + IReadOnlyList tree = + [ + Heading(2, "Removing a pupil", "removing-a-pupil"), + Heading(4, "Admitted following permanent exclusion", "admitted-exclusion") + ]; + + var nav = ContentNavBuilder.Build(tree); + + var section = Assert.Single(nav); + Assert.Equal("admitted-exclusion", Trim(Assert.Single(section.Children).Href)); + } + + [Fact] + public void AnH4BeforeAnyH2OrH3_IsTopLevel() + { + IReadOnlyList tree = + [ + Heading(4, "Orphan", "orphan"), + Heading(2, "Real section", "real-section") + ]; + + var nav = ContentNavBuilder.Build(tree); + + Assert.Equal(["orphan", "real-section"], nav.Select(n => Trim(n.Href))); + } + + [Fact] + public void AnH3AfterAnH4_ReturnsToTheH3Level() + { + // The walk has to track the current H3 and reset it, or a later H3 would end up nested + // inside the previous H3's children. + IReadOnlyList tree = + [ + Heading(2, "Section", "section"), + Heading(3, "First", "first"), + Heading(4, "Detail", "detail"), + Heading(3, "Second", "second") + ]; + + var nav = ContentNavBuilder.Build(tree); + + var section = Assert.Single(nav); + Assert.Equal(["first", "second"], section.Children.Select(c => Trim(c.Href))); + } + + [Fact] + public void ANewH2_ResetsTheH3Branch() + { + IReadOnlyList tree = + [ + Heading(2, "First section", "first-section"), + Heading(3, "Sub", "sub"), + Heading(2, "Second section", "second-section"), + Heading(4, "Detail", "detail") + ]; + + var nav = ContentNavBuilder.Build(tree); + + Assert.Equal(2, nav.Count); + // The H4 belongs to the second section, not to the first section's H3. + Assert.Empty(nav[0].Children[0].Children); + Assert.Equal("detail", Trim(Assert.Single(nav[1].Children).Href)); } private static string Trim(string href) => href.TrimStart('#'); From 00363e4e256f353567734238a3f2f977ee0e7ff4 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Tue, 15 Sep 2026 11:58:15 +0100 Subject: [PATCH 13/16] Let the author choose which heading levels the nav lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page-nav widget now carries a tick box per level, H1 to H6, with H2 and H3 ticked. The chosen levels form the hierarchy in the order they are chosen rather than by their absolute numbers, so a page that uses H2s and H4s but no H3s can tick just those two and get the H4s nested directly under the H2s. An unchosen level is ignored without breaking the chain around it. The nav is built from the page's own tree in the widget now, not read from the list the controller precomputes, because the levels are the widget's choice and two nav widgets on one page may legitimately want different ones. The precomputed list stays as the fallback for the editor preview, which renders a widget with no page around it. A widget placed before the tick boxes existed declares none of them, which means the default pair — not the empty list that unticking everything gives. Telling those apart is why the level reader checks whether the props are present rather than just reading each as a boolean. Each box carries a hidden companion posting "false". Without it an unticked box posts nothing, and the props builder keeps the registry default for a field it never sees, so unticking a level that defaults to on did not turn it off. Found by driving the editor form rather than posting props directly, and covered by a test that does the same. AB#303114 --- .../ContentPages/ContentNavBuilder.cs | 84 +++++---- .../ContentPages/NavLevelSelection.cs | 28 +++ .../ContentPages/WidgetRegistry.cs | 2 +- .../Views/ContentPage/Edit.cshtml | 2 + .../Views/Page/Content.cshtml | 3 + .../ContentPages/Widgets/_PageNav.cshtml | 13 +- .../Shared/ContentPages/_EditWidget.cshtml | 38 ++++ .../ContentPages/PageNavLevelsE2ETests.cs | 162 ++++++++++++++++++ .../ContentPages/ContentNavBuilderTests.cs | 139 ++++++++++++++- .../ContentPages/NavLevelSelectionTests.cs | 59 +++++++ .../PageNavRenderContractTests.cs | 51 ++++++ .../ContentPages/WidgetEditorContractTests.cs | 28 +++ .../ContentPages/WidgetRegistryTests.cs | 11 ++ 13 files changed, 570 insertions(+), 50 deletions(-) create mode 100644 src/DfE.CheckPerformanceData.Application/ContentPages/NavLevelSelection.cs create mode 100644 tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs create mode 100644 tests/DfE.CheckPerformanceData.UnitTests/ContentPages/NavLevelSelectionTests.cs create mode 100644 tests/DfE.CheckPerformanceData.UnitTests/ContentPages/PageNavRenderContractTests.cs diff --git a/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs b/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs index feb63b38a..e9ae81509 100644 --- a/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs +++ b/src/DfE.CheckPerformanceData.Application/ContentPages/ContentNavBuilder.cs @@ -1,54 +1,64 @@ namespace DfE.CheckPerformanceData.Application.ContentPages; -// Builds a page's left-hand nav by walking the content tree (depth-first, document order) for -// Heading widgets. H2 → top-level item; H3 → nested under the most recent H2; H4 → nested under -// the most recent H3. +// Builds a page's contents nav by walking the content tree (depth-first, document order) for +// Heading widgets. // -// Authors skip levels, so each level falls back to the nearest one that exists rather than being -// dropped: an H4 with no H3 above it attaches to the current H2, and a heading with nothing above -// it at all becomes top-level. Dropping it instead would leave a section of the page that the -// nav cannot reach, which is the whole job of this list. +// Which heading levels appear is the author's choice — any combination of H1 to H6. The chosen +// levels form the hierarchy in the order they are chosen rather than by their absolute numbers, +// so an author who picks H2 and H4 because the page does not use H3 gets H4 nested directly +// under H2, and the H3s are ignored. That is the whole point of choosing: the nav should follow +// the structure of this page, not the structure the levels imply in the abstract. // -// H1 is the page title and would duplicate the heading above the nav; H5 and H6 are below the -// depth a contents list stays readable at. Neither contributes. +// Levels fall back rather than being dropped: a heading whose parent level has not appeared yet +// attaches to the nearest chosen level above it that has, and one with nothing above it at all +// becomes top-level. Authors skip levels, and dropping those headings would leave sections of +// the page the nav cannot reach, which is the job it exists to do. public static class ContentNavBuilder { - public static IReadOnlyList Build(IReadOnlyList tree) + // What a page gets when nobody has chosen: the two levels a contents list is usually made of. + public static readonly IReadOnlyList DefaultLevels = [2, 3]; + + public static IReadOnlyList Build( + IReadOnlyList tree, + IReadOnlyCollection? levels = null) { + // Sorted and de-duplicated: the rank of a level is its position in this list, and that + // is what decides nesting. + var chosen = (levels ?? DefaultLevels) + .Where(level => level is >= 1 and <= 6) + .Distinct() + .OrderBy(level => level) + .ToList(); + + if (chosen.Count == 0) return []; + var top = new List(); - MutableItem? currentH2 = null; - MutableItem? currentH3 = null; + + // The most recent item at each rank. A heading attaches to the nearest non-null entry + // above its own rank; everything below its rank is cleared, so a later sibling cannot + // collect the children of the branch that just ended. + var openAtRank = new MutableItem?[chosen.Count]; foreach (var heading in Walk(tree)) { var (level, text) = HeadingProps(heading); - if (text is null || heading.Anchor is null) continue; + if (text is null || heading.Anchor is null || level is null) continue; + + var rank = chosen.IndexOf(level.Value); + if (rank < 0) continue; var item = new MutableItem(text, $"#{heading.Anchor}"); - switch (level) - { - case 2: - top.Add(item); - currentH2 = item; - // A new section starts a fresh branch: without this reset an H4 under the - // new H2 would attach to the previous section's last H3. - currentH3 = null; - break; - - case 3: - if (currentH2 is null) top.Add(item); - else currentH2.Children.Add(item); - currentH3 = item; - break; - - case 4: - if (currentH3 is not null) currentH3.Children.Add(item); - else if (currentH2 is not null) currentH2.Children.Add(item); - else top.Add(item); - break; - - // Other levels do not appear in the nav. - } + + MutableItem? parent = null; + for (var above = rank - 1; above >= 0 && parent is null; above--) + parent = openAtRank[above]; + + if (parent is null) top.Add(item); + else parent.Children.Add(item); + + openAtRank[rank] = item; + for (var below = rank + 1; below < openAtRank.Length; below++) + openAtRank[below] = null; } return top.Select(Freeze).ToList(); diff --git a/src/DfE.CheckPerformanceData.Application/ContentPages/NavLevelSelection.cs b/src/DfE.CheckPerformanceData.Application/ContentPages/NavLevelSelection.cs new file mode 100644 index 000000000..23b36d1c5 --- /dev/null +++ b/src/DfE.CheckPerformanceData.Application/ContentPages/NavLevelSelection.cs @@ -0,0 +1,28 @@ +namespace DfE.CheckPerformanceData.Application.ContentPages; + +// Reads the page-nav widget's per-level tick boxes into the set of heading levels its contents +// list should show. +// +// A widget placed before the tick boxes existed carries none of these props. That is not the +// same as an author unticking everything: the first means "never asked", which is the default +// pair, and the second means "asked for none", which is an empty list. Telling them apart is +// why this checks for the props' presence rather than just reading each one as a boolean. +public static class NavLevelSelection +{ + public static IReadOnlyCollection For(WidgetNode widget) + { + var anyDeclared = false; + var levels = new List(); + + for (var level = 1; level <= 6; level++) + { + var key = $"h{level}"; + if (widget.Props?.ContainsKey(key) != true) continue; + + anyDeclared = true; + if (widget.GetBool(key) == true) levels.Add(level); + } + + return anyDeclared ? levels : ContentNavBuilder.DefaultLevels; + } +} diff --git a/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs b/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs index 158841b26..c45fb6d17 100644 --- a/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs +++ b/src/DfE.CheckPerformanceData.Application/ContentPages/WidgetRegistry.cs @@ -17,7 +17,7 @@ public static class WidgetRegistry new("published", "Published callout", ContributesToNav: false, """{"text":""}"""), new("search", "Search", ContributesToNav: false, """{"label":"Search","placeholder":"","action":"/search","buttonText":"Search","scope":"","searchIn":"site","instant":"false","noResultsText":"No results found"}"""), new("results", "Search results", ContributesToNav: false, """{"scope":"","emptyText":"No results found."}"""), - new("pagenav", "Page navigation", ContributesToNav: false, """{"mode":"headings","childrenParentPath":"","showSearch":false,"searchPath":"","searchLabel":"Search"}""") + new("pagenav", "Page navigation", ContributesToNav: false, """{"mode":"headings","childrenParentPath":"","showSearch":false,"searchPath":"","searchLabel":"Search","h1":"false","h2":"true","h3":"true","h4":"false","h5":"false","h6":"false"}""") ]; private static readonly Dictionary ByType = diff --git a/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml b/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml index 0a560c2d7..07d77affc 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/ContentPage/Edit.cshtml @@ -402,6 +402,7 @@ var wrapper = select.closest('.govuk-details__text'); if (!wrapper) return; var childrenPanel = wrapper.querySelector('[data-cpb-pagenav-children]'); + var headingsPanel = wrapper.querySelector('[data-cpb-pagenav-headings]'); var searchCheckbox = wrapper.querySelector('[data-cpb-pagenav-showsearch]'); var searchPanel = wrapper.querySelector('[data-cpb-pagenav-searchfields]'); if (!childrenPanel) return; @@ -409,6 +410,7 @@ function refresh() { var isChildren = select.value === 'children'; childrenPanel.hidden = !isChildren; + if (headingsPanel) headingsPanel.hidden = isChildren; if (searchPanel) searchPanel.hidden = !isChildren || !(searchCheckbox && searchCheckbox.checked); } select.addEventListener('change', refresh); diff --git a/src/DfE.CheckPerformanceData.Web/Views/Page/Content.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Page/Content.cshtml index 44f40ea14..a87abe6b6 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Page/Content.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Page/Content.cshtml @@ -10,6 +10,9 @@ // on ViewData here — the widget partial reads whichever key its mode dictates. ViewData["PageHeadingNav"] = Model.Nav ?? Array.Empty(); ViewData["PageChildrenNav"] = Model.ChildrenNav ?? Array.Empty(); + // The tree itself, so a PageNav widget can build a heading list for the levels its author + // ticked rather than being stuck with the one the controller happened to precompute. + ViewData["PageContentTree"] = content; // If the author has placed their own H1 widget inside the content tree, suppress the // template-emitted title + subtitle — otherwise we'd render two H1s and the title would diff --git a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_PageNav.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_PageNav.cshtml index 655983e79..c9e558bed 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_PageNav.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/Widgets/_PageNav.cshtml @@ -5,8 +5,10 @@ @* Page-navigation widget. Two modes: - - "headings" (default) → renders the auto-generated heading nav for the current page - (H2 top-level, H3 nested). Pre-computed by PageController and stashed in ViewData. + - "headings" (default) → renders a contents list of the current page, covering whichever + heading levels the author ticked (H2 and H3 unless they said otherwise). Built here from + the page's own tree, because the levels are this widget's choice and two nav widgets on + one page may legitimately want different ones. - "children" → renders a list of direct child pages. Which parent's children are listed is set by the childrenParentPath prop (empty = current page). Optionally shows a @@ -50,7 +52,12 @@ } else { - items = ViewData["PageHeadingNav"] as IReadOnlyList ?? []; + var tree = ViewData["PageContentTree"] as IReadOnlyList; + items = tree is not null + ? ContentNavBuilder.Build(tree, NavLevelSelection.For(Model)) + // No tree in scope — the widget is being previewed in the editor, where the + // controller's precomputed list is the best available answer. + : ViewData["PageHeadingNav"] as IReadOnlyList ?? []; } var showSearch = mode == "children" && (Model.GetBool("showSearch") ?? false); diff --git a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_EditWidget.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_EditWidget.cshtml index 1d322ff07..56dc337a4 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_EditWidget.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Shared/ContentPages/_EditWidget.cshtml @@ -102,6 +102,44 @@
+ @* Heading-levels: which levels the contents list covers. Shown when + mode=headings. The chosen levels nest in the order they are + chosen, so ticking H2 and H4 on a page that skips H3 puts the + H4s directly under the H2s. *@ +
+
+ Heading levels to show +
+ They nest in the order ticked. If this page skips a level — + H2s and H4s but no H3s — tick just the ones it uses. +
+
+ @for (var lvl = 1; lvl <= 6; lvl++) + { + var lvlKey = $"h{lvl}"; + // A widget placed before these existed declares none of them, and + // means the default pair rather than nothing. + var declared = w.Props?.ContainsKey(lvlKey) == true; + var ticked = declared ? (w.GetBool(lvlKey) ?? false) : lvl is 2 or 3; +
+ + + @* An unticked box posts nothing, and the props builder keeps the + registry default for a field it never sees — so without this, + unticking a level that defaults to on would not turn it off. + It goes AFTER the label, not between input and label: the + browser posts in DOM order and the first value wins, so the + checkbox's "true" must come first, and the GDS tick and focus + ring are drawn by an adjacent-sibling rule on input + label + that anything in between would break. *@ + +
+ } +
+
+
@* Children-only fields — shown when mode=children. The script at the bottom of Edit.cshtml toggles visibility live as the select changes. *@
diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs new file mode 100644 index 000000000..ae81778b4 --- /dev/null +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs @@ -0,0 +1,162 @@ +using DfE.CheckPerformanceData.E2ETests.Fixtures; +using DfE.CheckPerformanceData.E2ETests.Helpers; + +namespace DfE.CheckPerformanceData.E2ETests.ContentPages; + +// Choosing which heading levels the page-nav widget lists. The case worth proving in a browser +// is the one an author actually hits: a page that skips a level, where ticking H2 and H4 has to +// nest the H4s under the H2s rather than dropping them or flattening them. +// +// Pairs with ContentNavBuilderTests (the nesting rules), NavLevelSelectionTests (props to level +// set) and WidgetEditorContractTests (the form fields). +[Collection("E2E")] +public sealed class PageNavLevelsE2ETests(PlaywrightFixture fixture) : SeedingPageTest(fixture) +{ + private readonly List _createdPages = []; + + public override async Task DisposeAsync() + { + for (var i = _createdPages.Count - 1; i >= 0; i--) + await CmsSeedHelpers.TryDeletePageAsync(Fixture.SeedClient, _createdPages[i]); + await base.DisposeAsync(); + } + + // A page whose headings run H2, H3, H4 so every combination has something to show. + private async Task<(Guid Id, string Url)> SeedAsync(Dictionary? navProps = null) + { + var segment = $"e2e-navlevels-{Guid.NewGuid():N}"; + var id = await CmsSeedHelpers.CreatePageNodeAsync( + Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", segment, "E2E nav levels"); + _createdPages.Add(id); + + await CmsSeedHelpers.AddWidgetAsync(Fixture.SeedClient, id, "0.0", "pagenav"); + if (navProps is not null) + await CmsSeedHelpers.UpdateWidgetAsync(Fixture.SeedClient, id, "0.0", "pagenav", navProps); + + var levels = new[] { 2, 3, 4, 2 }; + var names = new[] { "Section one", "Sub of one", "Detail of one", "Section two" }; + for (var i = 0; i < levels.Length; i++) + { + var path = $"0.{i + 1}"; + await CmsSeedHelpers.AddWidgetAsync(Fixture.SeedClient, id, path, "heading"); + await CmsSeedHelpers.UpdateWidgetAsync(Fixture.SeedClient, id, path, "heading", + new Dictionary { ["level"] = levels[i].ToString(), ["text"] = names[i] }); + } + await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); + + return (id, $"/help/{segment}"); + } + + private static Dictionary Nav(params int[] ticked) + { + var props = new Dictionary + { + ["mode"] = "headings", + ["childrenParentPath"] = "", + ["searchPath"] = "", + ["searchLabel"] = "Search", + }; + // Every level is posted explicitly, the way the editor form does it — the props builder + // keeps the registry default for a key it never receives, so a partial post would leave + // a level that defaults to on still on. + for (var level = 1; level <= 6; level++) + props[$"h{level}"] = ticked.Contains(level) ? "true" : "false"; + return props; + } + + private async Task> NavTextsAsync() => + await Page.Locator("nav.moj-side-navigation a").AllInnerTextsAsync(); + + [Fact] + public async Task ByDefault_TheNavShowsH2AndH3Only() + { + var (_, url) = await SeedAsync(); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + + var texts = await NavTextsAsync(); + Assert.Equal(["Section one", "Sub of one", "Section two"], texts); + } + + [Fact] + public async Task TickingH2AndH4_NestsTheH4UnderTheH2_AndDropsTheH3() + { + var (_, url) = await SeedAsync(Nav(2, 4)); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + + var texts = await NavTextsAsync(); + Assert.Equal(["Section one", "Detail of one", "Section two"], texts); + + // And it is nested, not flattened: the H4 sits inside the first H2's own list. + var nestedUnderFirst = await Page + .Locator("nav.moj-side-navigation > ul > li:first-child ul a") + .AllInnerTextsAsync(); + Assert.Equal(["Detail of one"], nestedUnderFirst); + } + + [Fact] + public async Task TickingOneLevel_GivesAFlatList() + { + var (_, url) = await SeedAsync(Nav(2)); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + + Assert.Equal(["Section one", "Section two"], await NavTextsAsync()); + Assert.Equal(0, await Page.Locator("nav.moj-side-navigation ul ul").CountAsync()); + } + + [Fact] + public async Task TickingAllThree_NestsThreeDeep() + { + var (_, url) = await SeedAsync(Nav(2, 3, 4)); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + + Assert.Equal( + ["Section one", "Sub of one", "Detail of one", "Section two"], + await NavTextsAsync()); + Assert.True(await Page.Locator("nav.moj-side-navigation ul ul ul a").CountAsync() > 0); + } + + [Fact] + public async Task UntickingALevelInTheEditor_TurnsItOff() + { + // The whole round trip, through the real form. An unticked box posts nothing, and the + // props builder keeps the registry default for a field it never sees — so a level that + // defaults to on stays on unless the form also posts an explicit "false" for it. This + // is the test that holds that companion field in place. + var (id, url) = await SeedAsync(); + + await Page.GotoAsync($"{Fixture.BaseUrl}/admin/pages/{id}/edit"); + await Page.Locator("summary:has-text(\"Edit page navigation\")").First.ClickAsync(); + + await Page.Locator("input[type=checkbox][name='props[h3]']").First.UncheckAsync(); + await Page.Locator("input[type=checkbox][name='props[h4]']").First.CheckAsync(); + await Page.Locator("[data-cpb-pagenav-headings]") + .Locator("xpath=ancestor::form") + .Locator("button[type=submit]").First.ClickAsync(); + await Page.WaitForLoadStateAsync(Microsoft.Playwright.LoadState.NetworkIdle); + + await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + + Assert.Equal(["Section one", "Detail of one", "Section two"], await NavTextsAsync()); + } + + [Fact] + public async Task TheEditorOffersABoxForEveryLevel_WithH2AndH3TickedOnANewWidget() + { + var (id, _) = await SeedAsync(); + + await Page.GotoAsync($"{Fixture.BaseUrl}/admin/pages/{id}/edit"); + await Page.Locator("summary:has-text(\"Edit page navigation\")").First.ClickAsync(); + + var boxes = Page.Locator("input[type=checkbox][name^='props[h']"); + Assert.Equal(6, await boxes.CountAsync()); + + var ticked = await Page.Locator("input[type=checkbox][name^='props[h']:checked") + .EvaluateAllAsync("els => els.map(e => e.name)"); + Assert.Equal(["props[h2]", "props[h3]"], ticked); + } +} diff --git a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs index a39fe0e0d..23c697876 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/ContentNavBuilderTests.cs @@ -3,10 +3,11 @@ namespace DfE.CheckPerformanceData.Application.UnitTests.ContentPages; -// The left-hand nav is built automatically by walking the content tree for Heading widgets, in -// document order: H2 → top-level item, H3 → nested under the most recent H2, H4 → nested under -// the most recent H3. Editors never hand maintain it. Headings nested inside regions are found -// just the same. +// The left-hand nav is built automatically by walking the content tree for Heading widgets in +// document order. Which levels appear is the author's choice — any combination of H1 to H6, H2 +// and H3 by default. The chosen levels form the hierarchy in the order they are chosen, so +// picking H2 and H4 nests H4 under H2 and ignores H3 entirely. Editors never hand maintain the +// list. Headings nested inside regions are found just the same. public class ContentNavBuilderTests { private static WidgetNode Heading(int level, string text, string anchor) => new() @@ -111,7 +112,7 @@ public void H4s_NestUnderThePrecedingH3() Heading(3, "Evidence", "evidence") ]; - var nav = ContentNavBuilder.Build(tree); + var nav = ContentNavBuilder.Build(tree, [2, 3, 4]); var section = Assert.Single(nav); Assert.Equal(2, section.Children.Count); @@ -137,7 +138,7 @@ public void AnH4WithNoPrecedingH3_NestsUnderTheH2() Heading(4, "Admitted following permanent exclusion", "admitted-exclusion") ]; - var nav = ContentNavBuilder.Build(tree); + var nav = ContentNavBuilder.Build(tree, [2, 3, 4]); var section = Assert.Single(nav); Assert.Equal("admitted-exclusion", Trim(Assert.Single(section.Children).Href)); @@ -152,7 +153,7 @@ public void AnH4BeforeAnyH2OrH3_IsTopLevel() Heading(2, "Real section", "real-section") ]; - var nav = ContentNavBuilder.Build(tree); + var nav = ContentNavBuilder.Build(tree, [2, 3, 4]); Assert.Equal(["orphan", "real-section"], nav.Select(n => Trim(n.Href))); } @@ -170,7 +171,7 @@ public void AnH3AfterAnH4_ReturnsToTheH3Level() Heading(3, "Second", "second") ]; - var nav = ContentNavBuilder.Build(tree); + var nav = ContentNavBuilder.Build(tree, [2, 3, 4]); var section = Assert.Single(nav); Assert.Equal(["first", "second"], section.Children.Select(c => Trim(c.Href))); @@ -187,7 +188,7 @@ public void ANewH2_ResetsTheH3Branch() Heading(4, "Detail", "detail") ]; - var nav = ContentNavBuilder.Build(tree); + var nav = ContentNavBuilder.Build(tree, [2, 3, 4]); Assert.Equal(2, nav.Count); // The H4 belongs to the second section, not to the first section's H3. @@ -195,5 +196,125 @@ public void ANewH2_ResetsTheH3Branch() Assert.Equal("detail", Trim(Assert.Single(nav[1].Children).Href)); } + + // ----- Choosing which levels appear ----- + + [Fact] + public void ByDefault_OnlyH2AndH3Appear() + { + IReadOnlyList tree = + [ + Heading(1, "Title", "title"), + Heading(2, "Section", "section"), + Heading(3, "Sub", "sub"), + Heading(4, "Detail", "detail") + ]; + + var nav = ContentNavBuilder.Build(tree); + + var section = Assert.Single(nav); + Assert.Equal("section", Trim(section.Href)); + Assert.Equal("sub", Trim(Assert.Single(section.Children).Href)); + } + + [Fact] + public void SkippingALevel_NestsTheNextChosenOneDirectlyUnderIt() + { + // The case an author actually asks for: this page does not use H3, so pick H2 and H4 + // and have the H4s sit under the H2s. + IReadOnlyList tree = + [ + Heading(2, "Section", "section"), + Heading(3, "Ignored", "ignored"), + Heading(4, "Detail", "detail"), + Heading(4, "Another detail", "another-detail") + ]; + + var nav = ContentNavBuilder.Build(tree, [2, 4]); + + var section = Assert.Single(nav); + Assert.Equal(["detail", "another-detail"], section.Children.Select(c => Trim(c.Href))); + Assert.DoesNotContain("ignored", section.Children.Select(c => Trim(c.Href))); + } + + [Fact] + public void AnUnchosenLevel_DoesNotBreakTheChainAroundIt() + { + // The ignored H3 must not detach the H4 that follows it from its H2. + IReadOnlyList tree = + [ + Heading(2, "First", "first"), + Heading(3, "Ignored", "ignored"), + Heading(4, "Under first", "under-first"), + Heading(2, "Second", "second"), + Heading(4, "Under second", "under-second") + ]; + + var nav = ContentNavBuilder.Build(tree, [2, 4]); + + Assert.Equal(2, nav.Count); + Assert.Equal("under-first", Trim(Assert.Single(nav[0].Children).Href)); + Assert.Equal("under-second", Trim(Assert.Single(nav[1].Children).Href)); + } + + [Fact] + public void ASingleChosenLevel_ProducesAFlatList() + { + IReadOnlyList tree = + [ + Heading(2, "One", "one"), + Heading(3, "Sub", "sub"), + Heading(2, "Two", "two") + ]; + + var nav = ContentNavBuilder.Build(tree, [2]); + + Assert.Equal(["one", "two"], nav.Select(n => Trim(n.Href))); + Assert.All(nav, n => Assert.Empty(n.Children)); + } + + [Fact] + public void H1_CanBeChosen_AndBecomesTheTopLevel() + { + IReadOnlyList tree = + [ + Heading(1, "Title", "title"), + Heading(2, "Section", "section") + ]; + + var nav = ContentNavBuilder.Build(tree, [1, 2]); + + var title = Assert.Single(nav); + Assert.Equal("title", Trim(title.Href)); + Assert.Equal("section", Trim(Assert.Single(title.Children).Href)); + } + + [Fact] + public void AllSixLevels_NestSixDeep() + { + IReadOnlyList tree = + [ + Heading(1, "L1", "l1"), Heading(2, "L2", "l2"), Heading(3, "L3", "l3"), + Heading(4, "L4", "l4"), Heading(5, "L5", "l5"), Heading(6, "L6", "l6") + ]; + + var nav = ContentNavBuilder.Build(tree, [1, 2, 3, 4, 5, 6]); + + var depth = 0; + var items = nav; + while (items.Count > 0) { depth++; items = items[0].Children; } + + Assert.Equal(6, depth); + } + + [Fact] + public void ChoosingNoLevels_ProducesAnEmptyNav() + { + // An author who unticks everything has asked for no contents list, and gets one. + IReadOnlyList tree = [Heading(2, "Section", "section")]; + + Assert.Empty(ContentNavBuilder.Build(tree, [])); + } + private static string Trim(string href) => href.TrimStart('#'); } diff --git a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/NavLevelSelectionTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/NavLevelSelectionTests.cs new file mode 100644 index 000000000..a8df4e326 --- /dev/null +++ b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/NavLevelSelectionTests.cs @@ -0,0 +1,59 @@ +using System.Text.Json.Nodes; +using DfE.CheckPerformanceData.Application.ContentPages; + +namespace DfE.CheckPerformanceData.Application.UnitTests.ContentPages; + +// Turning the page-nav widget's tick boxes into a set of heading levels. The distinction that +// matters is between a widget that was never asked (placed before the tick boxes existed, which +// gets the default pair) and one whose author unticked everything (which gets nothing). +public sealed class NavLevelSelectionTests +{ + private static WidgetNode Nav(JsonObject? props) => new() { Type = "pagenav", Props = props }; + + [Fact] + public void AWidgetWithNoLevelProps_GetsTheDefaultPair() + { + var widget = Nav(new JsonObject { ["mode"] = "headings" }); + + Assert.Equal([2, 3], NavLevelSelection.For(widget)); + } + + [Fact] + public void NullProps_GetTheDefaultPair() + { + Assert.Equal([2, 3], NavLevelSelection.For(Nav(null))); + } + + [Fact] + public void TickedLevels_AreReturnedInOrder() + { + var widget = Nav(new JsonObject + { + ["h1"] = "false", ["h2"] = "true", ["h3"] = "false", + ["h4"] = "true", ["h5"] = "false", ["h6"] = "false", + }); + + Assert.Equal([2, 4], NavLevelSelection.For(widget)); + } + + [Fact] + public void UntickingEverything_MeansNoLevels_NotTheDefault() + { + var widget = Nav(new JsonObject + { + ["h1"] = "false", ["h2"] = "false", ["h3"] = "false", + ["h4"] = "false", ["h5"] = "false", ["h6"] = "false", + }); + + Assert.Empty(NavLevelSelection.For(widget)); + } + + [Fact] + public void ARealBooleanIsReadTheSameAsTheFormsString() + { + // Registry defaults arrive as JSON values; a form post arrives as "true"/"false" text. + var widget = Nav(new JsonObject { ["h2"] = true, ["h3"] = false }); + + Assert.Equal([2], NavLevelSelection.For(widget)); + } +} diff --git a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/PageNavRenderContractTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/PageNavRenderContractTests.cs new file mode 100644 index 000000000..e61a6b62b --- /dev/null +++ b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/PageNavRenderContractTests.cs @@ -0,0 +1,51 @@ +namespace DfE.CheckPerformanceData.Application.UnitTests.ContentPages; + +// Static-Razor contract for the page-nav widget. The behaviour worth pinning is that a headings +// nav is built from the page's own tree using this widget's chosen levels — reading the list the +// controller precomputed instead would silently ignore the author's choice, and two nav widgets +// on one page could not differ. +public sealed class PageNavRenderContractTests +{ + private static readonly string View = ReadView(); + + [Fact] + public void HeadingsMode_BuildsFromThePagesOwnTree() + { + Assert.Contains("PageContentTree", View); + Assert.Contains("ContentNavBuilder.Build(tree", View); + } + + [Fact] + public void HeadingsMode_UsesTheLevelsThisWidgetChose() + { + Assert.Contains("NavLevelSelection.For(Model)", View); + } + + [Fact] + public void WithNoTreeInScope_ItFallsBackToThePrecomputedList() + { + // The editor preview renders the widget with no page tree around it; falling back keeps + // it showing something rather than rendering blank. + Assert.Contains("PageHeadingNav", View); + } + + private static string ReadView() + { + var root = FindSolutionRoot(AppContext.BaseDirectory); + return File.ReadAllText(Path.Combine( + root, "src", "DfE.CheckPerformanceData.Web", "Views", + "Shared", "ContentPages", "Widgets", "_PageNav.cshtml")); + } + + private static string FindSolutionRoot(string startDir) + { + var dir = new DirectoryInfo(startDir); + while (dir is not null) + { + if (dir.GetFiles("*.slnx").Length > 0 || dir.GetDirectories("src").Length > 0) + return dir.FullName; + dir = dir.Parent; + } + throw new InvalidOperationException($"Could not locate solution root from {startDir}."); + } +} diff --git a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetEditorContractTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetEditorContractTests.cs index 88310ffe4..a1c5d655a 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetEditorContractTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetEditorContractTests.cs @@ -100,6 +100,34 @@ public void Search_EditorKeepsTheFallbackFieldsVisible() Assert.Contains("props[buttonText]", b); } + // ----- PageNav widget ----- + + [Fact] + public void PageNav_EditorExposesAHeadingLevelBoxForEachOfH1ToH6() + { + // The boxes come out of a loop, so the source carries the bounds and the name pattern + // rather than six literals. That the rendered form really has six is asserted in the + // browser, by PageNavLevelsE2ETests. + var b = BranchSlice("case \"pagenav\":"); + Assert.Contains("for (var lvl = 1; lvl <= 6; lvl++)", b); + Assert.Contains("name=\"props[@lvlKey]\"", b); + Assert.Contains("$\"h{lvl}\"", b); + } + + [Fact] + public void PageNav_HeadingLevelBoxes_AreCheckboxes() + { + Assert.Contains("type=\"checkbox\" value=\"true\"", BranchSlice("case \"pagenav\":")); + } + + [Fact] + public void PageNav_AWidgetPlacedBeforeTheBoxesExisted_ShowsH2AndH3Ticked() + { + // Not the same as an author unticking everything, and the editor has to render the + // difference or opening an old widget would look like it had been turned off. + Assert.Contains("lvl is 2 or 3", BranchSlice("case \"pagenav\":")); + } + // ----- helpers ----- // Returns the slice of the editor between the `case "results":` line and its `break;`. diff --git a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetRegistryTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetRegistryTests.cs index b559f72a0..031d1c450 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetRegistryTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/ContentPages/WidgetRegistryTests.cs @@ -110,4 +110,15 @@ public void CreateDefaultProps_Search_CarriesNoResultsCopy() Assert.False(string.IsNullOrWhiteSpace((string?)props["noResultsText"])); } + + [Fact] + public void CreateDefaultProps_PageNav_TicksH2AndH3Only() + { + var props = WidgetRegistry.CreateDefaultProps("pagenav"); + + Assert.Equal("true", (string)props["h2"]!); + Assert.Equal("true", (string)props["h3"]!); + foreach (var off in new[] { "h1", "h4", "h5", "h6" }) + Assert.Equal("false", (string)props[off]!); + } } From 48cdc70a264a46dc496c9b034c27fd21ac2ba253 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Tue, 15 Sep 2026 13:41:19 +0100 Subject: [PATCH 14/16] Index every heading on the page, and mark the word searched for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three faults in the on-page search, all visible on one long page. The index only took headings that carried an id. The CMS anchors heading widgets, so a heading an author typed inside a rich-text block arrives as plain markup with none — 41 of the 77 headings on a page assembled from real guidance. Skipping them did not merely lose them: their text ran on into the previous anchored heading, so searching for a word underneath one offered a section some distance up the page. Searching "pregnant" returned "Life-limiting or critical illness". Every heading is indexed now, and given an id if it has none. Jumping to a section answered "where" but not "where exactly" — on a long section the word can still be paragraphs further down. Every occurrence is now wrapped in , the same element the search results page uses for its snippets, so the yellow means the same thing in both places. A new search clears the marks from the last one. The heading jumped to was outlined only when the browser's :focus-visible heuristic chose to, so the box came and went between jumps. It is asked for explicitly now, in GDS focus yellow. AB#303114 --- .../wwwroot/css/site.css | 15 +++ .../wwwroot/js/instant-search.js | 92 +++++++++++++++++-- .../InstantSearchWidgetE2ETests.cs | 70 ++++++++++++++ 3 files changed, 169 insertions(+), 8 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css b/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css index 150b9c8dd..1aaba2c43 100644 --- a/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css +++ b/src/DfE.CheckPerformanceData.Web/wwwroot/css/site.css @@ -1771,6 +1771,21 @@ body:has(.cpb-breadcrumbs) main.govuk-main-wrapper { padding-top: 0 !important; padding-top: 30px; } +/* On-page search: the heading it took you to. + The heading is given focus so a screen reader moves with the jump, and the browser drew a + box around it as a side effect — but only when its own :focus-visible heuristic felt like + it, so the box came and went between jumps. Asking for it explicitly on :focus makes it + the same every time. GDS focus yellow, matching the rest of the service. */ +.cpb-content h1:focus, +.cpb-content h2:focus, +.cpb-content h3:focus, +.cpb-content h4:focus, +.cpb-content h5:focus, +.cpb-content h6:focus { + outline: 3px solid #fd0; + outline-offset: 3px; +} + /* Page-contents nav: show the nesting. moj-side-navigation sets margin and padding to zero on every list at every depth, so a nested item sits at exactly the same left edge as its parent and the hierarchy exists only diff --git a/src/DfE.CheckPerformanceData.Web/wwwroot/js/instant-search.js b/src/DfE.CheckPerformanceData.Web/wwwroot/js/instant-search.js index 507aca45a..66f924ea8 100644 --- a/src/DfE.CheckPerformanceData.Web/wwwroot/js/instant-search.js +++ b/src/DfE.CheckPerformanceData.Web/wwwroot/js/instant-search.js @@ -153,8 +153,16 @@ return safe.slice(0, at) + '' + safe.slice(at, end) + '' + safe.slice(end); } - // Every heading with an anchor becomes a section; the text between it and the next heading - // becomes that section's body, so a match on a paragraph still points somewhere landable. + // Every heading becomes a section; the text between it and the next heading becomes that + // section's body, so a match on a paragraph still points somewhere landable. + // + // Headings without an id are indexed too, and given one. Only heading WIDGETS are anchored + // by the CMS — a heading an author typed inside a rich-text block is just markup, and on a + // long page most headings are those. Skipping them did not merely lose them: their text ran + // on into the previous anchored heading, so searching for a word under an unanchored + // heading offered the wrong section, some distance up the page. + var generatedIdSeq = 0; + function buildPageIndex() { var root = document.querySelector('.cpb-content'); if (!root) return null; @@ -166,9 +174,18 @@ while ((node = walker.nextNode())) { if (node.nodeType === 1) { - if (/^H[1-6]$/.test(node.tagName) && node.id && !node.closest(NON_CONTENT)) { - current = { anchor: node.id, label: node.textContent.trim(), text: '', el: node }; - if (current.label) sections.push(current); + if (/^H[1-6]$/.test(node.tagName) && !node.closest(NON_CONTENT)) { + var label = node.textContent.trim(); + if (!label) continue; + + if (!node.id) { + do { generatedIdSeq++; } + while (document.getElementById('cypmd-section-' + generatedIdSeq)); + node.id = 'cypmd-section-' + generatedIdSeq; + } + + current = { anchor: node.id, label: label, text: '', el: node }; + sections.push(current); } continue; } @@ -183,6 +200,61 @@ return sections; } + // Marking the term on the page + // --------------------------- + // Jumping to a section answers "where", but not "where exactly" — on a long section the word + // can still be several paragraphs down. Every occurrence is wrapped in , the same + // element the search results page uses for its snippets, so the yellow means the same thing + // in both places. + var MARK_CLASS = 'cypmd-onpage-mark'; + + function clearMarks(root) { + var marks = root.querySelectorAll('mark.' + MARK_CLASS); + for (var i = 0; i < marks.length; i++) { + var mark = marks[i]; + var parent = mark.parentNode; + while (mark.firstChild) parent.insertBefore(mark.firstChild, mark); + parent.removeChild(mark); + // Re-join the text nodes the unwrap left adjacent, so a later pass sees whole words. + parent.normalize(); + } + } + + function markTerm(root, term) { + clearMarks(root); + var needle = (term || '').trim().toLowerCase(); + if (needle.length < MIN_LENGTH) return; + + // Collected first: wrapping a node while walking would have the walker step into the + // just inserted. + var targets = []; + var walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT, null); + var node; + while ((node = walker.nextNode())) { + if (!node.nodeValue || node.nodeValue.toLowerCase().indexOf(needle) < 0) continue; + if (node.parentElement && node.parentElement.closest(NON_CONTENT)) continue; + targets.push(node); + } + + targets.forEach(function (textNode) { + var value = textNode.nodeValue; + var lower = value.toLowerCase(); + var fragment = document.createDocumentFragment(); + var at = 0; + var found; + while ((found = lower.indexOf(needle, at)) >= 0) { + if (found > at) fragment.appendChild(document.createTextNode(value.slice(at, found))); + var mark = document.createElement('mark'); + mark.className = MARK_CLASS; + mark.appendChild(document.createTextNode(value.slice(found, found + needle.length))); + fragment.appendChild(mark); + at = found + needle.length; + } + if (at < value.length) fragment.appendChild(document.createTextNode(value.slice(at))); + textNode.parentNode.replaceChild(fragment, textNode); + }); + } + function pageSource(sections) { return function (query, populateResults) { var q = (query || '').trim().toLowerCase(); @@ -227,9 +299,13 @@ }; } - function goToSection(section) { + function goToSection(section, term) { var target = document.getElementById(section.anchor); if (!target) return; + + var root = document.querySelector('.cpb-content'); + if (root) markTerm(root, term); + // Focus, not just scroll: a screen-reader or keyboard user needs the reading position // to move, not only the viewport. window.location.hash = section.anchor; @@ -329,7 +405,7 @@ } report.selected(keyOf(result), position); - if (searchIn === 'page') goToSection(result); + if (searchIn === 'page') goToSection(result, lastQuery); else if (result.url) window.location.assign(result.url); } }; @@ -384,7 +460,7 @@ if (match) { event.preventDefault(); - goToSection(match); + goToSection(match, q); } }); } diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs index 95408698c..29899d9dd 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs @@ -63,6 +63,14 @@ await AddAndSetAsync(id, "0.3", "heading", new Dictionary { ["level"] = "2", ["text"] = "Uploading files" }); await AddAndSetAsync(id, "0.4", "richtext", new Dictionary { ["html"] = $"

Accepted formats include {bodyToken} archives.

" }); + // A heading an author typed inside a rich-text block. The CMS anchors heading WIDGETS + // only, so this one reaches the page with no id — which is most of the headings on a + // long page assembled from real guidance. + await AddAndSetAsync(id, "0.5", "richtext", + new Dictionary + { + ["html"] = "

Typed inside rich text

Only reachable by its own words: zarquon.

", + }); await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); @@ -168,6 +176,68 @@ public async Task PageMode_BodyOnlyMatch_SuggestsItsEnclosingHeading() await Expect(Options.First).ToContainTextAsync("Uploading files"); } + // ============================================================ + // 2b. A heading written inside a rich-text block is a section too. + // + // Only heading widgets get an anchor from the CMS, so these arrive with no id. Skipping + // them did not just lose them: their text ran on into the previous anchored heading, so + // a word underneath one offered a section some distance up the page. + // ============================================================ + [Fact] + public async Task AHeadingInsideRichText_IsItsOwnSection_NotPartOfThePreviousOne() + { + var (url, _) = await SeedPageWithSectionsAsync(SearchProps("page", instant: true)); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + await TypeAsync("zarquon"); + + await Expect(Options.First).ToBeVisibleAsync(); + await Expect(Options.First).ToContainTextAsync("Typed inside rich text"); + // The section it used to be wrongly attributed to. + var labels = await Options.AllInnerTextsAsync(); + Assert.DoesNotContain(labels, l => l.Contains("Uploading files", StringComparison.Ordinal)); + } + + // ============================================================ + // 2c. The searched word is marked on the page, so it can be found within the section. + // ============================================================ + [Fact] + public async Task ChoosingASection_MarksTheSearchedWordOnThePage() + { + var (url, _) = await SeedPageWithSectionsAsync(SearchProps("page", instant: true)); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + await TypeAsync("evidence"); + await Expect(Options.First).ToContainTextAsync("Providing evidence"); + await Options.First.ClickAsync(); + + var marks = Page.Locator("mark.cypmd-onpage-mark"); + await Expect(marks.First).ToBeVisibleAsync(); + Assert.All( + await marks.AllInnerTextsAsync(), + text => Assert.Equal("evidence", text.ToLowerInvariant())); + } + + [Fact] + public async Task SearchingAgain_ClearsTheMarksFromThePreviousTerm() + { + var (url, _) = await SeedPageWithSectionsAsync(SearchProps("page", instant: true)); + + await Page.GotoAsync($"{Fixture.BaseUrl}{url}"); + await TypeAsync("evidence"); + await Expect(Options.First).ToContainTextAsync("Providing evidence"); + await Options.First.ClickAsync(); + await Expect(Page.Locator("mark.cypmd-onpage-mark").First).ToBeVisibleAsync(); + + await TypeAsync("uploading"); + await Expect(Options.First).ToContainTextAsync("Uploading files"); + await Options.First.ClickAsync(); + + Assert.All( + await Page.Locator("mark.cypmd-onpage-mark").AllInnerTextsAsync(), + text => Assert.Equal("uploading", text.ToLowerInvariant())); + } + // ============================================================ // 3. This page: nothing matches — the author's copy is what the visitor reads. // ============================================================ From da699ebe1fe0e11da62e3a03a36b90f2f6bb40ec Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Wed, 16 Sep 2026 21:08:17 +0100 Subject: [PATCH 15/16] Move the new search fixtures onto the fixture root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The five test classes added here seed their pages under /help and build their URLs from it. Main has since moved runtime-created browser-test content to its own /development-testing root, so CmsSeedHelpers.HelpRootId no longer exists and this branch stopped compiling the moment the two met. Both halves move across: the parent the pages are created under, and the paths and widget scope built from it. Same reason as the pages that went before them — teardown is best-effort and the delete route is a soft delete, so anything that leaks stays in the tree and travels to the next environment inside a content export. Better that it leaks somewhere nobody has to ask what it is. FixtureContent gains the root's bare segment alongside its path, because a widget scope is written without a leading slash. AB#303114 --- .../Admin/OnPageSearchSectionTests.cs | 4 ++-- .../InstantSearchReportingE2ETests.cs | 4 ++-- .../ContentPages/InstantSearchWidgetE2ETests.cs | 16 ++++++++-------- .../ContentPages/PageNavLevelsE2ETests.cs | 4 ++-- .../Helpers/FixtureContent.cs | 5 ++++- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs index a35e726a7..6ee4f28e7 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/Admin/OnPageSearchSectionTests.cs @@ -44,7 +44,7 @@ public async Task AnOnPageSearch_AppearsInTheSinglePageSection_AndDrillsInToItsT var segment = $"e2e-section-{Guid.NewGuid():N}"; var id = await CmsSeedHelpers.CreatePageNodeAsync( - Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", segment, "E2E single-page section"); + Fixture.SeedClient, FixtureContent.RootId, "content", segment, "E2E single-page section"); _createdPages.Add(id); await CmsSeedHelpers.AddWidgetAsync(Fixture.SeedClient, id, "0.0", "search"); @@ -63,7 +63,7 @@ await CmsSeedHelpers.UpdateWidgetAsync(Fixture.SeedClient, id, "0.1", "heading", new Dictionary { ["level"] = "2", ["text"] = "Providing evidence" }); await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); - var hostPath = $"/help/{segment}"; + var hostPath = $"{FixtureContent.RootPath}/{segment}"; // Search as a visitor would, and settle the query by leaving the box. await Page.GotoAsync($"{Fixture.BaseUrl}{hostPath}"); diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs index 2470f44ff..aaafd4a89 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchReportingE2ETests.cs @@ -56,7 +56,7 @@ private async Task SeedPageAsync(string searchIn, string scope = "") { var segment = $"e2e-report-{Guid.NewGuid():N}"; var id = await CmsSeedHelpers.CreatePageNodeAsync( - Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", segment, "E2E instant reporting"); + Fixture.SeedClient, FixtureContent.RootId, "content", segment, "E2E instant reporting"); _createdPages.Add(id); await CmsSeedHelpers.AddWidgetAsync(Fixture.SeedClient, id, "0.0", "search"); @@ -82,7 +82,7 @@ await CmsSeedHelpers.UpdateWidgetAsync(Fixture.SeedClient, id, "0.3", "heading", new Dictionary { ["level"] = "2", ["text"] = "Uploading files" }); await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); - return $"/help/{segment}"; + return $"{FixtureContent.RootPath}/{segment}"; } // Same shape as the admin suites use: mirror an impersonation cookie into the browser diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs index 29899d9dd..de2be6026 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs @@ -48,7 +48,7 @@ public override async Task DisposeAsync() var id = await CmsSeedHelpers.CreatePageNodeAsync( Fixture.SeedClient, - parentId: CmsSeedHelpers.HelpRootId, + parentId: FixtureContent.RootId, pageType: "content", segment: segment, title: "E2E instant search"); @@ -74,7 +74,7 @@ await AddAndSetAsync(id, "0.5", "richtext", await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); - return ($"/help/{segment}", bodyToken); + return ($"{FixtureContent.RootPath}/{segment}", bodyToken); } private async Task AddAndSetAsync(Guid pageId, string path, string widgetType, Dictionary props) @@ -264,7 +264,7 @@ public async Task PathMode_SuggestsDocumentsUnderThePath_AndChoosingOneNavigates var containerSegment = $"e2e-section-{Guid.NewGuid():N}"; var containerId = await CmsSeedHelpers.CreatePageNodeAsync( - Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", containerSegment, "E2E section"); + Fixture.SeedClient, FixtureContent.RootId, "content", containerSegment, "E2E section"); _createdPages.Add(containerId); await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, containerId); @@ -276,12 +276,12 @@ public async Task PathMode_SuggestsDocumentsUnderThePath_AndChoosingOneNavigates var outsideSegment = $"e2e-outside-{Guid.NewGuid():N}"; var outsideId = await CmsSeedHelpers.CreatePageNodeAsync( - Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", outsideSegment, $"Outside {token} page"); + Fixture.SeedClient, FixtureContent.RootId, "content", outsideSegment, $"Outside {token} page"); _createdPages.Add(outsideId); await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, outsideId); var (hostUrl, _) = await SeedPageWithSectionsAsync( - SearchProps("path", instant: true, scope: $"help/{containerSegment}")); + SearchProps("path", instant: true, scope: $"{FixtureContent.RootSegment}/{containerSegment}")); await Page.GotoAsync($"{Fixture.BaseUrl}{hostUrl}"); await TypeAsync(token); @@ -293,7 +293,7 @@ public async Task PathMode_SuggestsDocumentsUnderThePath_AndChoosingOneNavigates Assert.DoesNotContain(labels, l => l.Contains("Outside", StringComparison.Ordinal)); await Options.First.ClickAsync(); - await Expect(Page).ToHaveURLAsync(new Regex($"/help/{Regex.Escape(containerSegment)}/{Regex.Escape(insideSegment)}$")); + await Expect(Page).ToHaveURLAsync(new Regex($"{FixtureContent.RootPath}/{Regex.Escape(containerSegment)}/{Regex.Escape(insideSegment)}$")); } // ============================================================ @@ -361,7 +361,7 @@ public async Task TwoWidgetsOnOnePage_DoNotShareAnInputId() { var segment = $"e2e-instant-{Guid.NewGuid():N}"; var id = await CmsSeedHelpers.CreatePageNodeAsync( - Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", segment, "E2E two widgets"); + Fixture.SeedClient, FixtureContent.RootId, "content", segment, "E2E two widgets"); _createdPages.Add(id); await AddAndSetAsync(id, "0.0", "search", SearchProps("page", instant: true)); @@ -370,7 +370,7 @@ await AddAndSetAsync(id, "0.1", "heading", await AddAndSetAsync(id, "0.2", "search", SearchProps("page", instant: true)); await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); - await Page.GotoAsync($"{Fixture.BaseUrl}/help/{segment}"); + await Page.GotoAsync($"{Fixture.BaseUrl}{FixtureContent.RootPath}/{segment}"); var inputs = Page.Locator("input.autocomplete__input"); Assert.Equal(2, await inputs.CountAsync()); diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs index ae81778b4..0c2d5b66f 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/PageNavLevelsE2ETests.cs @@ -26,7 +26,7 @@ public override async Task DisposeAsync() { var segment = $"e2e-navlevels-{Guid.NewGuid():N}"; var id = await CmsSeedHelpers.CreatePageNodeAsync( - Fixture.SeedClient, CmsSeedHelpers.HelpRootId, "content", segment, "E2E nav levels"); + Fixture.SeedClient, FixtureContent.RootId, "content", segment, "E2E nav levels"); _createdPages.Add(id); await CmsSeedHelpers.AddWidgetAsync(Fixture.SeedClient, id, "0.0", "pagenav"); @@ -44,7 +44,7 @@ await CmsSeedHelpers.UpdateWidgetAsync(Fixture.SeedClient, id, path, "heading", } await CmsSeedHelpers.PublishDraftAsync(Fixture.SeedClient, id); - return (id, $"/help/{segment}"); + return (id, $"{FixtureContent.RootPath}/{segment}"); } private static Dictionary Nav(params int[] ticked) diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Helpers/FixtureContent.cs b/tests/DfE.CheckPerformanceData.E2ETests/Helpers/FixtureContent.cs index 535091b66..c78d0f039 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/Helpers/FixtureContent.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/Helpers/FixtureContent.cs @@ -18,7 +18,10 @@ public static class FixtureContent /// Pinned Guid of the /development-testing root. Parent for runtime-created fixtures. public static readonly Guid RootId = new("00000000-cd94-4a01-8f01-00000000000e"); - public const string RootPath = "/development-testing"; + /// The root's URL segment, with no leading slash — the shape a widget scope takes. + public const string RootSegment = "development-testing"; + + public const string RootPath = "/" + RootSegment; /// Long, wiki-typed page — the half of the back-to-top contract that scrolls. public const string LongPagePath = $"{RootPath}/long-page"; From c26a4f9ff5a45dd823d6107b26ddf44ff5107895 Mon Sep 17 00:00:00 2001 From: Lance Keay Date: Thu, 17 Sep 2026 10:20:01 +0100 Subject: [PATCH 16/16] Wait for the focus to move, rather than reading it once The section-jump test read document.activeElement the moment the URL assertion was satisfied. Those two things do not happen together: the module sets the hash synchronously and then moves focus in a later task, deliberately, because the autocomplete puts focus back on its own input while closing the menu and the move has to land after that. So the URL matches first and the read catches whatever held focus at that instant. On a developer's machine the round trip to the browser is slow enough that the deferred task has already run; on the CI runner it is not, and the assertion came back with an empty string twice. It now uses the same retrying assertion the keyboard twin beside it already used. Confirmed by widening the deferral to 300ms: the old form then fails locally with exactly the CI message, the new one passes. AB#303114 --- .../ContentPages/InstantSearchWidgetE2ETests.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs index de2be6026..7c5c981e3 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/ContentPages/InstantSearchWidgetE2ETests.cs @@ -133,8 +133,14 @@ public async Task PageMode_SuggestsSections_AndChoosingOneJumpsAndMovesFocus() await Options.First.ClickAsync(); await Expect(Page).ToHaveURLAsync(new Regex($"#{Regex.Escape(anchor!)}$")); - var focusedId = await Page.EvaluateAsync("() => document.activeElement && document.activeElement.id"); - Assert.Equal(anchor, focusedId); + + // A retrying assertion, not a single read of document.activeElement. The module sets the + // hash synchronously and then moves focus in a later task on purpose — the autocomplete + // puts focus back on its own input while closing the menu, so the move has to land after + // that. The URL therefore matches before the focus has moved, and a one-shot read taken + // the moment ToHaveURLAsync is satisfied catches whatever held focus first. It did on CI. + // Same assertion the keyboard twin below already uses. + await Expect(Page.Locator($"#{anchor}")).ToBeFocusedAsync(); } // ============================================================