Skip to content

Issue #3517035 by richardgaunt, joshua1234511, fionamorrison23: WCAG - Focus lands in wrong place upon form submission when error present #1443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Issue #3517035 by richardgaunt, joshua1234511, fionamorrison23: WCAG - Focus lands in wrong place upon form submission when error present #1443

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c3b1bb
[3517035] WCAG - Focus lands in wrong place upon form submission when…
joshua-salsadigital Oct 29, 2025
46530fa
[3517035] Lint fixes.
joshua-salsadigital Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// CivicTheme status-messages.js
(function ($, Drupal) {
Drupal.behaviors.ctFocusErrorSummary = {
attach: function (context, settings) {
// Wait for DOM update after AJAX or normal load
setTimeout(function () {
// Find any element with id starting with 'error-summary-'
var $error = $('[id^="error-summary-"]', context).first();
if ($error.length) {
$error.attr('tabindex', '-1'); // ensure focusable (in case)
$error.focus();
}
}, 30);
}
};
})(jQuery, Drupal);
16 changes: 16 additions & 0 deletions ...es/contrib/civictheme/civictheme_starter_kit/assets/js/status-messages/status-messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// CivicTheme status-messages.js
(function ($, Drupal) {
Drupal.behaviors.ctFocusErrorSummary = {
attach: function (context, settings) {
// Wait for DOM update after AJAX or normal load
setTimeout(function () {
// Find any element with id starting with 'error-summary-'
var $error = $('[id^="error-summary-"]', context).first();
if ($error.length) {
$error.attr('tabindex', '-1'); // ensure focusable (in case)
$error.focus();
}
}, 30);
}
};
})(jQuery, Drupal);
Comment on lines +1 to +16
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Critical: Code duplication between theme and starter kit.

This file is identical to web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js. Having duplicate code in both the main theme and starter kit creates a maintenance burden where changes must be synchronized across both files.

Recommended approach:

Since this is accessibility-critical functionality, the main theme version should handle it. The starter kit should either:

  1. Option 1 (Preferred): Remove this file entirely and inherit the behavior from the parent theme.
  2. Option 2: Only include this file if the starter kit needs to customize or extend the behavior. If so, document why the override is necessary.

If duplication is intentional (e.g., for easier customization), add a comment explaining that this file mirrors the parent theme's version and must be kept in sync:

// This file mirrors the parent theme's status-messages.js
// Keep in sync with: web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js
🤖 Prompt for AI Agents 
web/themes/contrib/civictheme/civictheme_starter_kit/assets/js/status-messages/status-messages.js
lines 1-16: this file duplicates the identical accessibility-focused
status-messages.js in the parent civictheme, creating maintenance burden; remove
this file from the starter kit so the parent theme's script is used (preferred),
or if the starter kit must provide it, replace its contents with a short comment
stating it intentionally mirrors the parent file and include the parent filepath
for sync tracking, or alternatively keep the file only when adding custom
behavior and document the reason for the override.

3 changes: 3 additions & 0 deletions web/themes/contrib/civictheme/templates/misc/status-messages.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
#}
{% for type, messages in message_list %}
{% for message in messages %}
{% set id_suffix = type == 'error' ? loop.parent.loop.index ~ '-' ~ loop.index : '' %}
{% set id_attr = type == 'error' ? 'id="error-summary-' ~ id_suffix ~ '" tabindex="-1"' : '' %}
{% include 'civictheme:message' with {
title: status_headings[type] ? status_headings[type] : type|capitalize,
content: message|render,
type: (type == 'status' or type == 'info') ? 'information' : type,
vertical_spacing: 'both',
attributes: id_attr
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Passing string to attributes may break rendering.

If the civictheme:message template expects an Attribute object or array for the attributes parameter, passing a plain HTML string will likely not render correctly or may be escaped as text rather than merged into the element's attributes.

🤖 Prompt for AI Agents 
In web/themes/contrib/civictheme/templates/misc/status-messages.html.twig around
line 16, the template is passing a plain HTML string to the `attributes`
parameter which may break rendering; change the call so `attributes` is an
Attribute object or an associative array (e.g. an attributes object or array
with keys like "id") instead of a raw string, creating or merging the
Attribute/array before passing it to the template and ensuring the id is set as
an attribute (not concatenated HTML), so the rendering engine can properly merge
and render the attributes.

} only %}
{% endfor %}
{% endfor %}
Loading