Problem/Motivation

This is a followup issue to #3084166: Add a focus-within polyfill to core.

Convert hidden.module.css to use focus-within instead of focus for the benefits that it brings.

Example use-case

Allow arbitrary hidden-but-focusable skip-links that are nested with <ul><li>, such as the "Add new comment" link.

Bartik

Bartik skip link example

Olivero

Olivero skip link example

Proposed resolution

Convert hidden.module.css to use focus-within.

Remaining task

Update for stable9
Check for other themes

CommentFileSizeAuthor
#14 3229647-14.patch550 bytesmansi agarwal
#2 drupal-hidden-module-focus-within-3229647-2.patch550 byteskentr
2021-08-21 16.31.41.gif64.25 KBkentr
2021-08-21 15.53.21.gif76.9 KBkentr

Issue fork drupal-3229647

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

Comments

kentr created an issue. See original summary.

kentr’s picture

kentr commented
StatusFileSize
new drupal-hidden-module-focus-within-3229647-2.patch550 bytes

Here's a patch against 9.3.x.

kentr’s picture

kentr commented
Issue summary: View changes
gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review

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.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +CSS

This seems like a great idea to me and totally makes sense.

Note that that the :focus-within pseudo-class will activate when the selected element has focus, too. So, this won't break BC

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue tags: +Needs reroll
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work

Needs re-roll for D10.

Discussing this on accessibility off-hours today.

mgifford’s picture

mgifford
he/him
Primary language English
commented

This looks like it will be a good improvement. Happy to see this change.

rpayanm made their first commit to this issue’s fork.

rpayanm opened merge request !3133

mansi agarwal’s picture

mansi agarwal commented
StatusFileSize
new 3229647-14.patch550 bytes

added patch against #2 in 10.1.x

_utsavsharma’s picture

_utsavsharma commented
Status: Needs work » Needs review
gaurav-mathur’s picture

gaurav-mathur commented
Assigned: Unassigned » gaurav-mathur
gaurav-mathur’s picture

gaurav-mathur commented
Assigned: gaurav-mathur » Unassigned

Patch #14 work fine in drupal 10.1.x-dev.
Thank you.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs accessibility review

As an accessibility improvement, this should have accessibility review.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue tags: -Needs reroll, -Needs accessibility review

As an accessibility improvement, this should have accessibility review.

Accessibility maintainer @mgifford commented in #11 (per our discussion in Drupal Accessibility office hours).

jitender vaidh’s picture

jitender vaidh
Primary language Hindi
commented
xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @mherchel for the context. :)

Hiding patches for clarity. Saving credits for reviewers, but uncrediting #14 as it just duplicated the work in the MR in #12 and #13, which in turn were also just applying a one-word change to an issue fork instead even though I can still apply #2 to 10.1.x now, so it did not actually need a reroll? (Also uncrediting my own clicking of the MR rebase button.)

As a change to module CSS, this is allowed in a Drupal minor version (so 10.1.x).

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Also happy to see Drupal get more accessible!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Category: Feature request » Bug report
Status: Reviewed & tested by the community » Needs review

Changing this to a bug since this is an accessibility improvement.

I'm wondering if we should be making this change to Stable 9 too? It seems like it would be a relatively low risk accessibility improvement that we could ship to all themes.

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Needs work

For accessibility my answer is always yes. If it can be added lets do it.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed #23, Updated changes in stable9 as well. Please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Perfect

xjm’s picture

xjm
she/her
Primary language English
commented

Fixing attribution.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Needs change record

Sorry, I forgot to mention in #23 that we would also need a change record since we are changing Stable. The CR should mention what the implications of this change is, and how someone with a theme extending Stable could revert to the old behavior if this is causing regressions for them.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Setting to NW to get the change record written.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue tags: +wcag211

Adding https://www.w3.org/WAI/WCAG21/Understanding/keyboard

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

saurav-drupal-dev made their first commit to this issue’s fork.

saurav-drupal-dev’s picture

saurav-drupal-dev commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

#28

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review

Created drafted Change Record. Feel free to update the change record if needed.
https://www.drupal.org/node/3484054

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs change record

Actually looking at the change introduced #32 and not sure why it's needed. Think it should be reverted.

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Comment has been removed.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

The mr should target the 11.x branch

arunkumark opened merge request !10217

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review

As per comment #39 created a new MR for 11.x version.

smustgrave closed merge request !3133

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

nod_ closed merge request !10217

  • nod_ committed f5fb18ed on 10.4.x 
    Issue #3229647 by rpayanm, kentr, xjm, gauravvvv, mherchel, mgifford,...

  • nod_ committed b5a626eb on 10.5.x 
    Issue #3229647 by rpayanm, kentr, xjm, gauravvvv, mherchel, mgifford,...

  • nod_ committed eb0636fd on 11.1.x 
    Issue #3229647 by rpayanm, kentr, xjm, gauravvvv, mherchel, mgifford,...

  • nod_ committed fcde87f5 on 11.x 
    Issue #3229647 by rpayanm, kentr, xjm, gauravvvv, mherchel, mgifford,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed fcde87f and pushed to 11.x. Thanks!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

(changing my attribution to add my company)

Status: Fixed » Closed (fixed)

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