Problem/Motivation
On most ajax triggered actions, we are losing the focus on the triggering element as it is removed from the dom then re-added.
That forces screen readers users or people navigating with their keyboard to restart from the beginning of the page.
Steps to reproduce
- Install drupal standard
- Go to the views add form (admin/structure/views/add)
- Focus the "View settings" > "Show" field with your keyboard
- Change its value using the keyboard
- Once it has been reloaded, hit tab
Expected result: the field just after the "Show" field is focused
Current result: the "Skip to main content" link is focused
Proposed resolution
Refocus the triggering element at the end of the ajax call unless an other element has been explicitely focused by the ajax call.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | Done | |
Update the issue summary | Instructions | Done | |
Update the issue summary noting if allowed during the beta | Instructions | Done | |
Manually test the patch | Novice | Instructions | |
Add steps to reproduce the issue | Novice | Instructions | Done |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
None.
API changes
None.
Data model changes
None.
Why this should be an RC target
Without this patch, the admin side of Drupal will be more difficult for screen reader users and keyboard only users to navigate. Predictability is really important for blind users and having the cursor location move unexpectedly can be a major disruption.
RC phase evaluation
Issue category | Bug because the expected behavior is to keep the focused element |
---|---|
Issue priority | Normal because it only affects a really small percentage of users |
Needed in RC | The main goal of this issue is accessibility. (Reduces screen readers pain using the Drupal interface) |
Disruption: Fixes a bug, blocks a contributed project, or should be backported to Drupal 7 | Not disruptive as it takes care to do nothing in the case the focus were called by the ajax response on purpose. |
Comment | File | Size | Author |
---|---|---|---|
#72 | bNKH3ijhFy.gif | 132.18 KB | Danny_Joris |
#71 | ajax_test.zip | 2.14 KB | Danny_Joris |
#62 | interdiff.1824636.33.62.txt | 1.8 KB | DuaelFr |
#62 | core-js-ajaxfocus-1824636-62.patch | 2.91 KB | DuaelFr |
#46 | Do not move the cursor to the top of the page on ajax calls with patch.png | 66.8 KB | meenakshi.r |
Comments
Comment #1
falcon03 CreditAttribution: falcon03 commentedtagging
Comment #2
dawehnerSo 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.
Comment #3
dawehnerreadd tag
Comment #4
webchick"really annoying" != critical bug.
Comment #5
falcon03 CreditAttribution: falcon03 commentedOK, maybe it's not a critical bug, but it can be a major one: if we don't fix this I think we cannot comply with WCAG 2.0 AA.
Comment #6
mgifford@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.
Comment #7
CBI 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.
Comment #8
larowlanBumping priority based on #6 and #7
Comment #9
mgiffordI 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.
Comment #10
mgiffordRelated issue #1806308: Review Views JavaScript + generic modals for accessibility
Comment #11
DuaelFrI 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.
Comment #12
DuaelFrLet'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.
Comment #13
mgifford@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.
Comment #14
DuaelFr@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.
Comment #15
mgifford@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?
Comment #16
DuaelFrI 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.
Comment #17
mgiffordI'm not a JS guy, but this seems like a sound approach.
Like your comment here:
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?
Comment #18
DuaelFrI 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?
Comment #19
joelpittet@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.
Comment #20
DuaelFrThe 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 :
Pros: request object available to get the triggering element name
Cons: only called for ajax callbacks that does not return an AjaxResponse object
Pros: called for all ajax request, request stack available
Cons: less readable while browing the code to understand what happens
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?
Comment #21
joelpittetOh if the dom element is replaced then it's likely being replaced into something, maybe put the focus on the containing element?
Comment #22
DuaelFrThe 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.
Comment #23
DuaelFrPlanning to work on this with @nod_ at DC Barcelona next week.
Comment #24
DuaelFrI 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.
Comment #25
DuaelFrComment #26
larowlanManually tested, works as advertised™
Comment #27
nod_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.Comment #28
nod_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?
Comment #29
DuaelFrAdded 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.
Comment #30
DuaelFr@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.
Comment #32
DuaelFrPatch in #29is just the interdiff...
Comment #33
nod_Simplified the code a bit and fixed the variable naming (forgot to mention it earlier), we use camelCase, not snake_case in JS.
Comment #38
DuaelFrOnly changing JS should not make the tests fail.
Comment #39
GoZ CreditAttribution: GoZ at Centarro commentedI 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.
Comment #40
DuaelFrAs 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.
Comment #41
mgiffordThat looks good to me. I'm happy to mark this as RTBC.
Comment #42
nod_Well I'm pretty sure we need to fix the views ui auto preview issue before we can get it in.
Comment #43
webchickIf this breaks views preview we definitely are not committing this just before RC1. Can we get confirmation pro/con?
Comment #44
DuaelFrThat 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.
Comment #45
nod_#2124397: Keyboard focus is lost after an AJAX page update is a dup, and is major. raising this one.
Comment #46
meenakshi.r CreditAttribution: meenakshi.r as a volunteer and at Srijan | A Material+ Company commentedIssue is fixed with the latest patch
before patch
after patch
Comment #47
catchComment #48
xjmWe 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.Comment #49
mgiffordComment #50
mgiffordThanks @xjm, I think I've got that now.
Comment #52
mgiffordComment #53
Wim LeersI 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.
Comment #54
mgiffordThat should definitely get tested.
Comment #55
mgiffordOk, 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.
Comment #56
Wim LeersSwitching to Bartik is not key. The ability to still use contextual links using the keyboard is key. Did you test that?
Comment #57
DuaelFr@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?
Comment #58
Wim LeersWhat 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.
Comment #59
mgiffordI 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?
Comment #60
xjmDiscussed this with @alexpott, @catch, and @effulgentsia, and we agreed to make this issue an RC target for the accessibility bugfix.
Comment #61
alexpott#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.
Comment #62
DuaelFrPlease 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.
Comment #63
nod_I like this solution, and it works.
Comment #64
alexpottCommitted 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.
Comment #66
nod_Nice! thanks.
Comment #67
mgiffordI just get a
Bad object id: 5352b3d
maybe that's temporary.Comment #68
DuaelFrHere is the proper link to the commit: 87ac540 :)
Comment #69
mgiffordThanks @DuaelFr
Comment #71
Danny_Joris CreditAttribution: Danny_Joris at Lullabot commentedI 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.Comment #72
Danny_Joris CreditAttribution: Danny_Joris at Lullabot commentedComment #73
Danny_Joris CreditAttribution: Danny_Joris at Lullabot commentedComment #74
Danny_Joris CreditAttribution: Danny_Joris at Lullabot commentedComment #75
tim.plunkettCan you open a follow-up issue?
Comment #76
Danny_Joris CreditAttribution: Danny_Joris at Lullabot commentedFollow issue created here: #2627788: Focus state bug on text field AJAX calls
Comment #81
chrisgross CreditAttribution: chrisgross commentedAny chance of getting this backported to d7? I've tried, but haven't been able to figure it out.
Comment #82
cilefen CreditAttribution: cilefen commented@chrisgross Open the backport issue and relate it to this one. Use the same title with "[D7]" in front (I think).
Comment #83
chrisgross CreditAttribution: chrisgross commentedThis 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.
Comment #84
mradcliffeWe 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.
Comment #85
mmjvb CreditAttribution: mmjvb as a volunteer commentedIn that case would expect it to be set to some 8 version, no idea which one.
Comment #86
sjerdoComment #87
Wim Leers