Problem/Motivation

The removal of jQuery UI includes no longer having use of the :tabbable selector.
Currently, the only way to assign focus via an AJAX Command is using the :tabbable selector in InvokeCommand. There needs to be a way to do this that does not depend on jQuery UI (and ideally no jQuery either). When #3113649: Remove drupal.tabbingmanager's jQueryUI dependency is completed this will be possible.

Steps to reproduce

Proposed resolution

Add a new AJAX command the makes it possible to focus an element.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3188938-5-REROLL.patch10.51 KBbnjmnm
#2 3188938-2.patch23.29 KBbnjmnm

Issue fork drupal-3188938

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
3188938-2.patch23.29 KB

This patch includes parts from #3113649: Remove drupal.tabbingmanager's jQueryUI dependency which hasn't yet landed, but is needed to provide ajax.js the new tabbable library.

This adds the FocusFirst AJAX command and test coverage for it.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs change record
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and commented
Title: [PP-1 ]Create AjaxCommand for focusing that does not require :tabbable selector » Create AjaxCommand for focusing that does not require :tabbable selector
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Active » Needs review
FileSize
3188938-5-REROLL.patch10.51 KB

The blocker landed so this is now ready for review. I'll take care of the change record after the first round of reviews so there's evidence I won't have to scrap the overall approach.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Ajax/FocusFirstCommand.php
@@ -0,0 +1,39 @@
+ * AJAX command for focusing an element.

    Let's expand this documentation a bit to include what happens if selector matches multiple elements, and what happens if no element matches the selector. We should also clarify that the command will focus the first focusable element inside that selector, not the selector itself, and what happens if there are not focusable elements inside the element matching the selector. We could also add a @see referencing the client side code implementing this command. 

  2. +++ b/core/lib/Drupal/Core/Ajax/FocusFirstCommand.php
@@ -0,0 +1,39 @@
+   * Implements Drupal\Core\Ajax\CommandInterface:render().

    Nit: this should be {@inheritdoc}. 

  3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FocusFirstCommandTest.php
@@ -0,0 +1,48 @@
+  public function testFocusFirst() {

    Can we expand the test coverage to include use case where selector matches multiple elements, use case where selector doesn't match any elements and use case where the matched element doesn't contain any focusable elements?

bnjmnm opened merge request !430

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review

Addressed #6 in Gitlab fork.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

I think the MR is in a good state now. We still need a CR before this can be moved to RTBC.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue tags: -Needs change record

CR added!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

Change record looks good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work

Did a review on gitlab - some comments to address.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
CreditAttribution: zrpnr at Acquia commented
Status: Needs review » Reviewed & tested by the community

The commits in 573e2efb and 4a599bb2 rename all the variables that were added to dictionary.txt, that file now has no changes in this MR. I did a case sensitive search and didn't find any other uses of the lowercased variables, looks like @bnjmnm caught them all.
The other change was the formatting for the array in FocusFirstCommandTest.php, I agree that looks better now and matches @alexpott suggestion in the MR review.

This was RTBC, those 2 issues are now addressed, setting it back.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed cf214a4 and pushed to 9.2.x. Thanks!

  • alexpott committed cf214a4 on 9.2.x 
    Issue #3188938 by bnjmnm, lauriii, alexpott, zrpnr: Create AjaxCommand...

Status: Fixed » Closed (fixed)

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