Problem/Motivation

Targeting the parent of the removed DOM element on ajax sets focus, which is implicitly applicable to a limited set of elements, such as form elements (input, select, etc.) and links (a), based on https://api.jquery.com/focus.

Noticed that while working on the Entity browser issue linked here.

Steps to reproduce

  • Use an entity browser and edit an image.
  • After clicking "Save" in the modal, the "Save" button on the node gets focused - fixable by setting 'disable-refocus' => TRUE on the submit button in the Entity browser
  • With that fix above, no focus is set at all

Proposed resolution

We can use all element types by using tabindex.

CommentFileSizeAuthor
#26 3080834-nr-bot.txt144 bytesneeds-review-queue-bot
#21 interdiff-3080834-20-21.txt1.33 KBabhisekmazumdar
#21 3080834-21.patch2.33 KBabhisekmazumdar
#20 3080834-20.patch2.27 KBabhisekmazumdar
#17 3080834-eb-focus-on-all-elements-16.patch2.67 KBsasanikolic
#17 3. Previous element is focused.png89.36 KBsasanikolic
#17 2. Replace image.png74.5 KBsasanikolic
#17 1. Expand paragraph.png87.56 KBsasanikolic
#11 Screenshot 2019-11-11 at 10.50.09.png203.06 KBsasanikolic
#10 3080834-eb-focus-on-all-elements-10.patch1.18 KBrenrhaf
#8 3080834-eb-focus-on-all-elements-8.patch1.19 KBrenrhaf
#2 3080834-eb-focus-on-all-elements-2.patch681 bytessasanikolic

Comments

sasanikolic created an issue. See original summary.

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new 3080834-eb-focus-on-all-elements-2.patch681 bytes

Here is the patch with the proposed fix.

sasanikolic’s picture

sasanikolic commented
Title: Focus divs on ajax calls » Focus any element on ajax calls
sasanikolic’s picture

sasanikolic commented
Issue summary: View changes
cilefen’s picture

cilefen commented
Status: Active » Needs review

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented
Status: Needs review » Reviewed & tested by the community

Setting to RTBC, tested in our project (see https://www.drupal.org/project/entity_browser/issues/3053181#comment-133...).

As per https://api.jquery.com/focus/ :
"The focus event is sent to an element when it gains focus. This event is implicitly applicable to a limited set of elements, such as form elements (input, select, etc.) and links. In recent browser versions, the event can be extended to include all element types by explicitly setting the element's tabindex property."

And https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabi... :
"A negative value (usually tabindex="-1") means that the element is not reachable via sequential keyboard navigation, but could be focused with Javascript or visually... The user won't be able to focus any element with a negative tabindex using the keyboard, but a script can do so by calling the focus() method."

renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3080834-eb-focus-on-all-elements-8.patch1.19 KB

Ooops, maybe we should make sure not to alter the tabindex if there is already one set on the focused element.
Setting back to review.

sasanikolic’s picture

sasanikolic commented

Thanks for the feedback @Renrhaf!

+++ b/core/misc/ajax.js
@@ -451,7 +451,14 @@ function _toConsumableArray(arr) { if (Array.isArray(arr)) { for (var i = 0, arr
+        var tabindex = $(target).attr('tabindex');
+        if (typeof tabindex !== typeof undefined && tabindex !== false) {

Doesn't this contradict the statement "not to alter the tabindex if there is already one set on the focused element."?

renrhaf’s picture

renrhaf
Primary language French
Location 📍 Strasbourg 🐦🦜
commented
StatusFileSize
new 3080834-eb-focus-on-all-elements-10.patch1.18 KB

Oh yeah, you're right sasanikolic... sorry for this.
Here is the updated patch.

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new Screenshot 2019-11-11 at 10.50.09.png203.06 KB

Hmm, seems like this is causing some side effects. For example, when clicking Edit on a paragraph, the '.paragraph-top' parent class is focused.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

In any case @Renrhaf this issue can not be RTBC without test coverage. The pass above lead to two learnings:

  • The original bug is not test covered, this needs an explicit coverage of the expectation.
  • The regressions we caused are not test covere, asking for additional coverage in existing cases.

EDIT: When reading this issue i had trouble to reproduce what the bug was and what the fix would exactly result in. Test coverage would clarify this a lot.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Issue tags: +Accessibility

I checked Paragraphs accessibility and improved the situation for keyboard navigation.
See #3092762-15: Unable to access toggle button options using keyboard navigation.

This seems to be the next most annoying problem as after any ajax operation, the focus is lost and you need to tab through the whole form again, making the keyboard UX pretty unacceptable.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Related issues: +#2736461: [META] Review Paragraphs widget accessibility
kasey_mk’s picture

kasey_mk commented

Thanks for this. #10 works for us and makes editing paragraphs much less annoying.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Forcing any element to become focusable seems a strange hack to me.

In Paragraphs, we add scroll persistency recovery when switching the behavior tabs. So we modify scroll position to make sure the active element stays in the viewport. This is needed because document length can vary, most importantly the length of elements above the viewport.

As one approach, Paragraphs could put the tabindex: -1 on each widget.
This is however not a Paragraphs only thing. It likely also affects Inline Entity Form. Does the problem also apply to regular fields with many values?

How about this alternative: Focus the first focussable element. The loop that identifies a target needs to also check for focussability.
I feel like this is the best we can do while maintaining the designed semantics of UX instead of force changing them.

sasanikolic’s picture

sasanikolic commented
Status: Needs work » Needs review
StatusFileSize
new 1. Expand paragraph.png87.56 KB
new 2. Replace image.png74.5 KB
new 3. Previous element is focused.png89.36 KB
new 3080834-eb-focus-on-all-elements-16.patch2.67 KB

I tried your proposed solution @miro_dietiker, but couldn't figure out a couple of issues and I'm unsure this is the right direction to solve this, as we are forcing a decision to focus the first element of a parent, which might not be related to the clicked (and removed) element at all?

The issues I faced with this approach was, that it didn't work at all when opening and closing modals - e.g. entity browser for selecting images. Initially the close button was focused when opening the EB, but with these changes the "Select entities" was being focused instead. And when closing the EB by clicking the x button, the focus was not persisted but moved to the previous focused element. Not sure what is forcing that behavior, probably some code elsewhere. When replacing the image, however, this triggered the link "Show row weights" link to be focused, instead of the "Replace button"

See the new patch and screenshots attached.

Will try to figure out what is going on during the weekend.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Assigned: sasanikolic » Unassigned
StatusFileSize
new 3080834-20.patch2.27 KB

Re-roll the patch to 9.2.x.
Also unassigning the issue from @sasanikolic as it needed to be reviewed by others.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
StatusFileSize
new 3080834-21.patch2.33 KB
new interdiff-3080834-20-21.txt1.33 KB

Fixed lint errors.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3080834-nr-bot.txt144 bytes

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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Closed (duplicate)
Issue tags: +Needs Review Queue Initiative
Related issues: +#3188938: Create AjaxCommand for focusing that does not require :tabbable selector

Is this even still relevant?

AFAICT it's not possible to automatically determine which element to focus after an AJAX command — it's context-dependent.

#3188938: Create AjaxCommand for focusing that does not require :tabbable selector added an AJAX command to make it feasible to do focus handling correctly, allowing each UI to handle it correctly in their context. Closing as a duplicate of that.