Focus state bug on text field AJAX calls

I was mentioned it in old issue:
https://www.drupal.org/node/2124397#comment-9159611

Here is a patch that disable the automatic refocus when the defined event is "blur".
I had to move the code a bit to benefit from the default event defined according to the element type.

By looking at ajax.js I found the following logic which does the focus() call:

    // If the focus hasn't be changed by the ajax commands, try to refocus the
    // triggering element or one of its parents if that element does not exist
    // anymore.
    if (!focusChanged && this.element && !$(this.element).data('disable-refocus')) {
      var target = false;

      for (var n = elementParents.length - 1; !target && n > 0; n--) {
        target = document.querySelector('[data-drupal-selector="' + elementParents[n].getAttribute('data-drupal-selector') + '"]');
      }

      if (target) {
        $(target).trigger('focus');
      }
    }

This seems to be a non-documented feature: if you add a data attribute disable-refocus to your element, then this issue won't happen any more. I added the following to the affected form element and the issue stopped happening: '#attributes' => array('data-disable-refocus' => 'true'),.

I feel that more people will encounter this bug and waste time trying to figure it out. Why are we setting focus in the first place?

You are right, I didn't document this feature when I developed it.
The best way to disable the auto-refocus is to use $element['#ajax']['disable-refocus'] = TRUE;, though.
I don't know how the FAPI documentation is generated but I suppose we should explain this a bit more there.

I still think we need to fix that particular use case because it's generating an infinite focus loop.

Thanks for the feedback @DuaelFr. Do you why do we have that trigger logic? I still don't understand what it is supposed to do. Sounds too opinionated to me that core focuses on the element's wrapper.

Okay, I just finished reading the summary of #1824636: Do not move the cursor to the top of the page on ajax calls and now I get it. To be honest, I don't know what would be best to do to fix this.

I think my patch fixes the issue. The blur event is really special so we should just avoid it. I suppose we should do the same with the focus event as it also could lead to an infinite loop in some cases.

Agree, your patch looks fine to me.

There is another problem with TabIndex in Internet Explorer (not covered/solved by the patch in #5) and
"disable-refocus"-Inputs. (Tested in IE11 Win7 & Win10 IE13)

If you leave the field with TAB while the Ajax process is running, the focus/cursor is completely removed.
So one can't finish the form that way.

I've been banging my head on this issue for a few weeks now. Adding 'disable-refocus' => true to my '#ajax' array in the field generation array fixed the problem.

I have a thought and I think it's prime time to take this action now. RBTC. #12, we need another approach for that unfortunately

Yeah #12 is totally another problem related to the replacement of some parts of the form by the AjaX callbacks.
Thanks for the review @droplet

Now we have javascript tests this is testable - so we don;t ever break it again.

Adding 'disable-refocus' => true to my '#ajax' array fixed the problem.

Hi, here is a patch Drupal 8.3.x.

Hello,

Thanks all for the work done here.

I didn't know about the 'disable-refocus' attributes. It solved my problem.

Just tested this patch on 8.6.2 and it doesn't seem to be working - live example www.kobejet.com - the ultimenu dropdowns have ajax textbox filters.

Just had this issue with the Drupal Commerce Quantity field (e.g a number input).

Tested to apply the patch from #12.
Patch applies correctly but the refocus issue is not fixed.

Tested to simply add $element['#ajax']['disable-refocus'] = TRUE in my form.
This works :) Tested on Drupal 8.6.3 and 8.6.4.

Just reapplied the patch from #21 on 8.7.x HEAD and tried it with the little module linked in the Issue Summary.

Without the patch it is not possible to leave the form fields as the ajax call refocus the blured field.
With the patch, the ajax call does not refocus the blured field anymore.

I tested both 'blur' and 'change' with and without 'disable-refocus'.

While on 'change' the refocus is just a bit annoying, on 'blur' it creates an endless loop, as shown above.
And yes, you want 'change' on a textfield if you don't want AJAX to be triggered unless something was actually changed.
See these two screencasts for 'blur' and 'change' (without patch applied):

Blur:	Change:

I don't have to embed the ones with the patch (resp. 'disable-refocus' enabled) as they are flawless. I'm appending them though.

So think this patch clearly is a first, important stop-gap, as we definitely need to avoid an endless loop.
However, refocussing back to the triggering element is a very opinionated default setting. And not refocussing at all is not always a useful alternative, for example if the target element gets removed.
Furthermore, there are valid usecases - even for 'blur' - where AJAX field validation should indeed force the cursor back to the triggering element. We should provide a framework that works for all these cases, though in a followup.

As this is just a stop-gap, we don't want to remove functionality possibly used somewhere in contrib. I wonder if we shouldn't just default to 'disable-refocus' on blur, while at least allowing to override it with a 'disable-refocus' => FALSE;

As this is just a stop-gap, we don't want to remove functionality possibly used somewhere in contrib. I wonder if we shouldn't just default to 'disable-refocus' on blur, while at least allowing to override it with a 'disable-refocus' => FALSE;

Sorry for citing myself, but I really think we shouldn't go as far to completely prohibit refocussing after 'blur', so I'm preparing a patch that only changes the default, if nothing is specified.

Also think this is a major usability bug, even though this is "only" the slightly neglected Ajax API everybody thinks is only for "pros" who know what they're doing. A default that locks the cursor in, basically rendering the whole form unusable, clearly needs to be fixed and backported.

Everything else may go to the followup #3033151: Add ['#ajax']['refocus'] property to FormAPI elements.

Here's the patch. Assigning to myself for the tests.

Tests are a bit tricky, so I probably need the weekend. in the meantime, please feel free to review patch #31.

Putting this on my review list. I'm still trying to grok our AJAX system, finding some of the abstractions a bit hard to follow.

Fixing tags

Is 8.6.x the correct target version, now that 8.7.0 has been released? (Patch is working on 8.7 for me, fwiw).

Still needs tests.

Does anyone know how to write a test for something like this? It would be great to have this fixed.

Thank you very much for pointing out the 'disable-refocus' option, it saved my day. That option seems to miss in the documentations about AJax in Drupal forms. Will write a documentation part once I find time for it.

Reroll 9.2.x.

Fixing tests.

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 😅

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 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.

Looks like unrelated random fails. #65 was #3386682: [random test failure] MenuUiTest::testMenuAdministration 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: #3393894: Improve ajax system documentation to include advanced settings.

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!