Focus state bug on text field AJAX calls

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I have a multivalue field (a list of emails with an Add another button) with a change event ajax trigger on each item of the field (eg field_email[0][value]). The problem I am having is that if I disable refocus, and then end up having to replace the entire field, the focus ends up at the top of the form.

Would this fix cover that problem? Or maybe one of the related issues? The best thing would be if the cursor ended up where it was going to go before ajax intervened, I don't know if that's possible.

Patch for 10.1.x.

adding sc https://www.w3.org/WAI/WCAG21/Understanding/no-keyboard-trap

Let's see what the testbot thinks

Woo all green! Moving to NW for the tests though.

Here's tests for this.

Passing all linters is hard 😅

The last submitted patch, 60: 2627788-60-test-only.patch, failed testing. View results →

Indeed, it is hard to pass all linters 🤦‍♂️

1. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -327,6 +333,16 @@ public static function preRenderAjaxForm($element) {
   +      $text_types = ['password', 'textfield', 'number', 'tel', 'textarea'];
   ...
   +        if ($element['#ajax']['event'] === 'blur' || $element['#ajax']['event'] === 'change' && in_array($element['#type'], $text_types)) {
   

   These 5 types all have their #ajax][event type set to blur unless it's manually set to something else... Not sure if that's what this is protecting against or not.

   Additionally, the mixing of || and && without parentheses makes this extremely hard to parse.

2. +++ b/core/misc/ajax.js
   @@ -1083,19 +1094,30 @@
   +                for (
   +                  let n = elementParents.length - 1;
   +                  !target && n >= 0;
   +                  n--
   +                ) {
   

   Is this how JS wants us to format for loops?

   Also, any reason to not use the more standard i instead of n for the counter?

#63.1 I tried to clarify the comments and added parentheses to help with the grouping.
#63.2 This is how prettier wants to format it.. Not much I can do about it. Note that that part is pre-existing code, but it was reformatted because there's an additional if now.

The last submitted patch, 64: 2627788-64.patch, failed testing. View results →

The prettier reformatting was so jarring I didn't even notice that it was existing code, lol sorry for being distracted.

The comment change and added () make that better, thanks.

The last submitted patch, 64: 2627788-64.patch, failed testing. View results →

Looks like unrelated random fails. #65 was 🐛 [random test failure] MenuUiTest::testMenuAdministration Active and I can't see how #67 could be caused by this given that the error occurred before any AJAX events had been triggered.

I think we need a change record for the new refocus-previous setting. Plus the issue summary needs updating for the current solution.

Opened a CR and updated the issue summary.

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -273,6 +273,12 @@ public static function preRenderAjaxForm($element) {
+    // Add a data attribute to attempt to focus element that was focused before
+    // executing ajax commands.
+    if ($element['#ajax']['refocus-previous'] ?? FALSE) {
+      $element['#attributes']['data-refocus-previous'] = "true";
+    }

Discussed the meaning of "previous" with @lauriii and we decided that it is really hard to know what it means in this context. So we're going to rename it and improve the documentation.

Also we should open an issue to document advanced AJAX settings somewhere - maybe in a section following the AJAX docs in core.api.php

Addressed #71.

Opened follow-up for the advanced settings: 🐛 Improve ajax system documentation to include advanced settings Active .

The last submitted patch, 73: 2627788-73.patch, failed testing. View results →

Helps if I change the attribute name in the JavaScript code too 😇

Seems point in #71 has been addressed.

Crediting re-rollers and commenters.

Committed and pushed 27d76911e88 to 11.x and df73d39e0ec to 10.2.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.