Do not move the cursor to the top of the page on ajax calls

So this seems to be general problem of the ajax system.

The ajax system is using the ID as selector, so once the update happens, the ID is replaced and the focus is lost.

In theory drupal could automatically add a focus once an ajax selection happened.

readd tag

"really annoying" != critical bug.

@falcon03 Why? It would be discoverable & operable. Just not intuitive. I'm not opposed to it being major, but it is important to point folks to specifically what part of WCAG 2.0 this breaks.

I tend to agree with @mgifford here. This is a usability issue, not really a WCAG issue.

Unfortunately, causing people to tab a million times does not an accessibility issue make.

Bumping priority based on #6 and #7

I think this is true of all of the elements when editing a view such as admin/structure/views/view/test

It would be great if when opening a modal dialog that the point that the user left the page was remembered so that they could just go on with the next item.

There must be other places too where a modal is opened and when closing it the user isn't where they used to be.

Related issue #1806308: Review Views JavaScript + generic modals for accessibility

I confirm this is still an issue in HEAD. We could just .focus() the select box to keep the cursor at the right position after the ajax call.

Let's try this way.
I'm not sure about this patch at all. Someone should try to look for side effects even if the testbot passed.

@DuaelFr have you done what @dawehner suggested in #2 "In theory drupal could automatically add a focus once an ajax selection happened."

I can confirm that with the patch when adding a new view admin/structure/views/add I the focus didn't jump back up to View settings when the Show or of type is changed and after Please wait... is displayed.

This will benefit more than just screen reader users.

@mgifford My solution is very specific to this case.

I think that we could try to make it automatic for all ajax requests but I'm not sure it could be shipped in 8.0.
The idea would be to define a new FocusCommand that would extend InvokeCommand and to always add a FocusCommand, on the triggering element, at the end of all AjaxResponses when there is not already one. I just don't know yet how to get the triggering element outside the ajax callback.

@DuaelFr Can you make a new issue for the feature you described to make it automatic for all ajax requests?

It should be it's own issue and probably not one we can get in 8.0 at this point.

So your patch in #12 works. What do we need to do to get this specific issue RTBC?

I finally built a more generic way to fix this issue.
I think it would need some tests, though.

If we follow this way, we should update the title and issue summary or open a new issue.

I'm not a JS guy, but this seems like a sound approach.

Like your comment here:

+    // Search the existing commands to find out if there is already an
+    // InvokeCommand command with a "focus" method. If not, and if the request
+    // references the triggering element name, create an InvokeCommmand to
+    // focus the triggering element. This is made for accessibility reasons
+    // to help the screen readers or Braille keyboard users to keep their
+    // context while triggering an ajax action.

So a new title could be, "Ensure that the focus is intentionally set rather than default to the top of the page"

What additional tests are needed?

I think it's not the most perfect implementation.
It only works when the ajax callback returns a string instead of an ajax command array.
I'm thinking about using a hook_ajax_render_alter instead as it should also work in the other case.

I'd like to have the opinion of an ajax system expert. I'm not sure that relying on the triggering element name is the best approach as it should only works on classic form elements and not on links as they usualy do not have a name attribute.
Could you find someone to help us here, please?

@mgifford does this solve part of the problem at least?

@DuaelFr nice partial solution, it's weird trying to interpret PHP as JavaScript. What causes the moving to the top after an ajax command in the first place? Is it browser specific?

Steps to reproduce this would great to add to the Issue Summary.

Edit: removed tests because we don't have a JS testing framework.

The cursor is moved to the top because the element on which the focus is is removed from the DOM then recreated by the ajax request.

I still think we should write tests on the PHP side to be sure that AjaxRenderer::renderResponse() returns a proper commands array.

Anyway, I'm still not sure this is the best solution. I think that issue could be fixed on the JS side. That could be the best solution to be sure we have the right triggering element.

So, we have 3 different ways to fix that :

• in AjaxRenderer::renderResponse() #16
  Pros: request object available to get the triggering element name
  Cons: only called for ajax callbacks that does not return an AjaxResponse object
• in hook_ajax_render_alter()
  Pros: called for all ajax request, request stack available
  Cons: less readable while browing the code to understand what happens
• in JS on ajax return
  Pros: best way to get the triggering element even when not in a form, called for each ajax request
  Cons: untestable, harder to ensure that there is not already a focus call in the response

What should we do?

Oh if the dom element is replaced then it's likely being replaced into something, maybe put the focus on the containing element?

The problem is that we does not currently have any information about the container of the triggering element.
I think the best solution is to fix this on the JS side where we can keep the triggering element and its parent in a variable so we can try to refocus the element and if it does not exist anymore try to focus its container.

Planning to work on this with @nod_ at DC Barcelona next week.

I finally did it!
All the logic is now on JS side so we have more chance to find and focus the good element.
Not providing an interdiff as everything changed from the last patch.

I ran ESLint on my fix so I'm sure it respects the Core standards.

Manually tested, works as advertised™

On quickedit requests (or any request using the new .execute() method of Drupal.Ajax objects) js error:Uncaught TypeError: Cannot read property 'id' of undefined ajax.js?v=8.0.0-dev:766

Since view previews are enabled by default and trigger an ajax call on page load it messes the focus: on pages such as admin/structure/views/view/content the focus is essentially on the bottom of the page.

Discussed on IRC but for the record, since this is very much a FormAPI fix first, let's use the new data-drupal-selector instead of messing around with id and name. That's the kind of things this attribute is meant for.

Another questions about expected behavior: when clicking on a 'add another item' button, where should the focus be? with this patch focus stays on the button but what's interesting is above the button. Should we focus on the updated container or try to find the last item added?

Added a test on this.element to avoid QuickEdit to throw errors
Used data-drupal-selector instead of id and name

No solution for Views preview or any automated ajax call for now.

@nod_ The purpose of this patch is to fix the most common behaviour. Currently, the focus is resetted at the beginning of the page so it's still better. A part of the patch is about not trying to do focus the triggering element if an invoke:focus ajax command has been fired. This way, custom behaviours can implement what they want.

Patch in #29is just the interdiff...

Simplified the code a bit and fixed the variable naming (forgot to mention it earlier), we use camelCase, not snake_case in JS.

Only changing JS should not make the tests fail.

I just test with current 8.0.x-dev.

Manually tests work as expected and no errors or warning are displayed in console.

Patch #33 looks great.

As expected, it does not fix all the cases because there are a lot of ways to do ajax on Drupal.
But, most of the ajax loads made from Form API is going to benefit from this patch.
I think it a really great improvement for a11y we want to have in 8.0.

That looks good to me. I'm happy to mark this as RTBC.

Well I'm pretty sure we need to fix the views ui auto preview issue before we can get it in.

If this breaks views preview we definitely are not committing this just before RC1. Can we get confirmation pro/con?

That does not break views preview. It introduces an a11y issue on the views admin form but that replaces an other a11y issue...

It's kind of hard to explain for me byt let's try:
- without that patch, each time an Ajax action is triggered (except opening a dialog window), the keyboard focus is resetted to the page start
- with that patch, each time an Ajax action is triggered from a form element, the focus stays on the element
but, the thing is that the preview action of the views pages is an ajax action. So, as this action is automatically triggered, that patch do what it's been made for and refocus the triggering element each time the preview is reloaded.

So, with and without that patch, the view admin is hard to use for keyboard users.

#2124397: Keyboard focus is lost after an AJAX page update is a dup, and is major. raising this one.

Issue is fixed with the latest patch

before patch

after patch

We should also add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

Thanks @xjm, I think I've got that now.

I wonder how this interacts with core/drupal.tabbingmanager. Which surprisingly hasn't been mentioned at all.

An AJAXy form in Bartik where contextual links are also present and triggered using the keyboard via the toolbar's pencil icon would allow that to be tested.

That should definitely get tested.

Ok, got the time to test this.

Switched the admin theme to Bartik, went to /admin/structure/views/add and then went through the steps.

Seems to work as expected.

Switching to Bartik is not key. The ability to still use contextual links using the keyboard is key. Did you test that?

@Wim Leers the purpose of this patch is to refocus elements after an ajax call. It only do something if it finds the triggering element. Contextual links being links, that patch won't change anything for that case.
I'm not sure I totally understood what you meant, though. Could you explain your point a bit more, please?

What if you're in a TabbingManager-constrained focus, and performing an action triggers an AJAX request — does this then break the TabbingManager?

However, with Quick Edit's tabbing not yet having been updated to once again use TabbingManager, perhaps it doesn't make sense to raise this concern right now. The clear benefits seem to vastly outweigh the potential downside here. Just note that we may need to refine/revisit this later, once more things start to use TabbingManager.

I do agree that when folks start using the TabbingManager we're going to have to look at a few things. Let's address that as a new issue though.

I've tagged this to try to help us remember that there may be an impact. Would there be some other issues we should tag now and revisit in 8.1?

Discussed this with @alexpott, @catch, and @effulgentsia, and we agreed to make this issue an RC target for the accessibility bugfix.

#44 does not seem to be addressed and yep refocussing on the preview after adding a field in the views ui looks really weird and makes me think that this patch might have a few unintended side-effects.

Please review that proposition that allows to disable the refocus action from the #ajax settings.
I applied it to the Views preview button so the automatic preview does not move the focus anymore but neither the non-automatic one.

Keep in mind that, even if that does not cover all the existing use cases and even if that could introduce some strange behaviors on complex forms, that's still a lot better than nothing on all the other ajax-enabled pages.

I like this solution, and it works.

Committed 5352b3d and pushed to 8.0.x. Thanks!

I credited everyone who worked on #2124397: Keyboard focus is lost after an AJAX page update too.

Nice! thanks.

I just get a Bad object id: 5352b3d maybe that's temporary.

Here is the proper link to the commit: 87ac540 :)

Thanks @DuaelFr

I believe this introduces a bug where you can't unfocus a text field that triggers Ajax on the default 'blur' event.

I included a D8 module example to test. Enable ajax_test and navigate to /admin/config/development/ajax-test to see the form.

Can you open a follow-up issue?

Follow issue created here: #2627788: Focus state bug on text field AJAX calls

Any chance of getting this backported to d7? I've tried, but haven't been able to figure it out.

@chrisgross Open the backport issue and relate it to this one. Use the same title with "[D7]" in front (I think).

This issue is still marked as for 7.x, even though it contains patches for 8.x and the related issue 2124397 was changed to 8.x, even though it contained patches for 7.x. I'm not really sure what's going on.

We changed the way that backports are tracked several years ago. It used to be we used Patch (to be ported) and set the "needs backport to D7" tag, but these days we create a follow-up issue instead to avoid clutter.

I created the back port issue: #2960934: [D7] Do not move the cursor to the top of the page on ajax calls.

In that case would expect it to be set to some 8 version, no idea which one.