Draft: Listing rework code review fixes

[jhancock532]

PR #438 — replies to review comments

Draft replies describing the implementation as it stands.

Commit progress

The review work is split across these commits:

Commit	Purpose	Review relationship
a802608e	Remove redundant :has() support checks	Helen's _header.scss / _primary-nav-dropdown.scss comments
01f1cdd7	Clarify listing filter labels for screen readers	Helen's filter-group label comment
6ccea52d	Move listing placement into grid styles	Helen's grid-placement and #listing-panel selector comments
38369e9c	Isolate tests from the development cache	Independently discovered test-environment fix
ecfc0a53	Fix division filtering on Work listings	Independently discovered production bug
5ced3801	Improve listing filter behavior and validation	Event timing, dropdown visibility, validation, SEO, and backend performance work
77b7ff31	Disable htmx history snapshots on listings	Independently discovered browser-navigation fix
d63348b8	Configure webpack to use eslintrc	Independent tooling compatibility fix
3bc6e438	Improve listing filter progressive enhancement	Non-JS support, data targets, stable controls, htmx swaps, keyboard behavior, and announcements
d9d5d3d4	Update listing filter documentation	Documents the resulting behavior
00b1f273	Correct listing option swap assertion	Distinguishes option-list swaps from the intentional active-filter OOB swap

Some backend and frontend review fixes are grouped into a shared commit where their final-state changes touch the same functions and are easier to review together. No replies from this file have been posted automatically.

Verification

• Container listing suite: 70 tests passed.
• Jest: 4 suites and 7 tests passed.
• ESLint and stylelint passed.
• git diff --check passed.

helenb

Culture dropdown with one “Culture” option

Agreed. Dropdown visibility is now based on the unfiltered listing and hides a dropdown when its only option repeats the dropdown label (case-insensitively). An active selection still keeps it visible, and a single option with a different label remains usable. This is shared by the taxonomy and Events listings through dropdown_is_visible().

Commit: 5ced3801

Events “When” filter cannot be deselected

The timing choices are now independent checkboxes. With neither selected, the listing uses its default upcoming view without counting timing as an active filter. Selecting upcoming or past shows that group; selecting both shows all events, with upcoming events first. timing is therefore a repeatable query parameter.

Commit: 5ced3801

Dropdowns without JavaScript / why not use <select>?

Filtering now works without JavaScript. The option panels are rendered expanded, and the form submits selected checkboxes with a normal GET request using the visible Apply filters button. When JavaScript initialises, it turns the groups into button-controlled disclosures, hides Apply filters, and enhances changes with htmx.

We retained checkboxes rather than a native <select multiple> because the checkbox presentation matches the agreed design and gives a clearer multi-select experience than a native multiple-select control. Both submit without JavaScript, so the accessibility concern is addressed without changing the interaction we designed for.

Commit: 3bc6e438

Keep or remove the Apply filters button

Kept. It is the submit path when JavaScript is unavailable and is hidden when the enhanced behavior initialises.

Commit: 3bc6e438

Use data attributes for htmx targets

Updated. Active-filter links, Clear all, pagination, and the programmatic htmx request target [data-listing-results]; styling continues to use classes. Swaps use outerHTML so the selected results wrapper replaces the existing wrapper rather than becoming nested inside it.

Commit: 3bc6e438

Track dropdown open state without a CSS class

Open state is represented by the toggle's aria-expanded value and the panel's hidden property. The JavaScript no longer uses a styling class as application state.

Commit: 3bc6e438

Focus and screen-reader updates

The open filter and focus are retained after a checkbox update so another option can be selected. Tabbing out of an option panel closes it and returns focus to its toggle; Escape also closes the disclosures and restores focus.

A polite aria-live region outside the swapped results announces “Listing updated” with the result count after an htmx update, rather than causing the result set to be re-read.

Commit: 3bc6e438

Filter group label

Changed to “Filter the {{ item.title }} listing” for both Events and taxonomy listings.

Commit: 01f1cdd7

Robots meta tags

Robots metadata is decided by the server response, not by htmx. An unfiltered or single-filter listing is indexable; two or more active filters render noindex, nofollow. This matrix is covered directly by the SEO tests in tbx/core/listing/tests/test_filters.py: test_no_filters_uses_plain_title_and_no_robots, test_single_filter_title_and_no_robots, and test_multi_filter_is_noindex. test_context_only_contains_title_and_robots also confirms the listing context no longer carries a canonical value.

The tag does not appear to change while filtering in the browser because htmx swaps only the document title; the robots tag stays at whatever the initial full page load produced. This is intentional — crawlers read the server response, not the htmx-updated DOM — so indexing stays correct even though the in-page value looks static. The empty placeholder meta element (the second document-robots node with no name attribute) has been removed.

Note that this is a separate mechanism from the site-wide robots tag in base.html, which emits noindex whenever SEO_NOINDEX is set, the request host doesn't match the site record, or the page is the incident page. On a local :8000 setup that global tag fires on every page regardless of filters, because request.get_host() includes the port (localhost:8000) while the Wagtail Site.hostname does not (localhost), so the comparison never matches. That masks the listing's own robots logic in view-source, so the tests above — which assert the context values directly — are the reliable confirmation rather than a local page load.

Commit: 5ced3801

Listing SCSS comments

• Listing header and panel placement now use the shared grid classes; the inert grid declarations on the nested filter form were removed.
• The arbitrary 9rem dropdown minimum was removed.
• Enhanced panels use min-width: 100% on small screens, with the wider minimum applied from the medium breakpoint, so they remain inside the viewport.
• Styling targets .listing-panel; #listing-panel remains the JavaScript/htmx hook.

Commit: 6ccea52d

@supports selector(:has(...))

Removed the redundant wrappers around all three existing :has() rules. The rules themselves remain.

Commit: a802608e

Separate listing bundle / htmx dependency

Kept as a separate entry so htmx and the filter behavior load only on the three listing pages and changes to them do not invalidate the site-wide main.js bundle.

“public” filter uses the listing page theme

This remains a consequence of the listing page having one configured theme rather than a behavior introduced by filtering. It needs a separate design decision if the listing theme should react to a selected division.

Running the review pass (tbx-pr-review)

Thanks — this was useful. Running a review pass over the branch helped surface several fixes that were unrelated to the specific inline threads, which are in their own commits: division filtering on Work listings (ecfc0a53), test isolation from the development cache (38369e9c), and disabling htmx history snapshots on listings (77b7ff31), alongside query-efficiency work on the listing context. Test coverage was extended accordingly.

tm-kn

Use django-filter rather than reading request.GET

Agreed on the underlying point: no user value should reach the ORM without being validated first. Rather than adding django-filter, the implementation applies that principle directly. No raw request.GET value is passed to a query — every parameter is checked against a known set of valid values and unknown or stale values are discarded before filtering runs.

This is done in two ways because the two listings filter different data shapes, and I'd rather each validation live next to the thing it validates than force both through one abstraction:

• Taxonomy (Work/News) filters a queryset. TaxonomyFilterState.from_request() allow-lists every sector, service, and division slug against live database values before a value reaches an ORM filter. The validated state is then shared by result filtering, faceting, selected labels, URLs, and SEO. Work also needs different relations from News (WorkPage and HistoricalWorkPage plus page-tree paths), so those application functions stay explicit and testable.
• Events filter a Python list of StreamField values rather than a queryset, so a Django form is the natural fit: EventFilterForm validates ?timing= / ?type= against its choices (via LenientMultipleChoiceField, which drops stale values instead of erroring).

Both paths implement the same rule — keep known values, silently drop the rest — so the validation guarantee is consistent even though the mechanism differs. django-filter would add a dependency and a second layer over state that already drives facets, pills, URLs, and SEO, which felt like more than this needs; happy to revisit if you feel strongly.

Commit: 5ced3801

Why swap metadata with htmx?

Only the document title is swapped now, so the browser tab and history reflect the selected filters. Canonical output comes from base_page.html, and robots metadata is rendered only in the initial server response; neither is swapped by htmx.

Commit: 5ced3801

Canonical URLs for repeated content

Agreed. Listing-specific canonical handling has been removed. base_page.html supplies the canonical URL and omits filter parameters while preserving pagination.

Commit: 5ced3801

JakubMastalerz

Conflict with #440

Resolved in the same direction as #440: listing templates do not emit a canonical tag. Canonical output is owned by base_page.html; the listing SEO context contains only the filtered document title and the conditional robots value.

Commit: 5ced3801

Additional changes and open questions (no existing review thread)

Event type dropdown now shows all types used by the listing

Not raised in review, flagged during manual testing. The event-type dropdown previously faceted against the default upcoming-only view, so a type used only by past events (e.g. a past-only "In person" or "Online event") never appeared even though the type exists on the listing. It now facets across all timings when no timing is selected, so the default view offers every type used by any event — matching how the Work/News dropdowns show every taxonomy term used by the listing when nothing is selected. Selecting a timing still narrows the type options to that timing. Covered by two new tests in test_events_listing.py.

Commit: 06a483c0

Open question: should "Upcoming" show as active by default on Events?

The events listing shows upcoming events by default, so it may read more clearly if the "Upcoming" timing control appeared active/selected by default. Raising this rather than changing it, because it interacts with the "can't deselect" issue resolved earlier:

• Current behaviour: no timing selected = default upcoming view, but not a counted filter — no pill, no "Clear all", and it doesn't affect the indexable/noindex filter count.
• If "Upcoming" were pre-selected and counted as a filter, the default page would gain a pill and "Clear all" and become a single-filter (indexable) page, and deselecting it with nothing else selected would fall back to the upcoming default — i.e. it would look un-deselectable again, close to the original complaint.
• If it were only a visual pre-check with no filter semantics, deselecting it would still snap back to upcoming, which is also potentially confusing.