Make WordPress Core

Opened 2 years ago

Closed 10 months ago

Last modified 9 months ago

#55370 closed enhancement (fixed)

Enable aria-controls and aria-expanded in content

Reported by: crs1138's profile crs1138 Owned by: joedolson's profile joedolson
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: good-first-bug has-patch commit add-to-field-guide
Focuses: accessibility Cc:

Description (last modified by sabernhardt)

We have created a custom collapsible block, but the Gutenberg validation fails as the aria-controls and aria-expanded attributes are stripped from the saved content if the block is used by a user with role below super administrator.

Could these attributes be added to the list of allowed attributes?

Our problem is described in more detail at Gutenberg issue 39351 and @ocean90 recommended opening a ticket in Trac.

Similar topic has been discussed at Trac #30421

Attachments (5)

53234.2022032000.patch (3.1 KB) - added by rsiddharth 2 years ago.
55370.v202203242230.patch (491 bytes) - added by rsiddharth 2 years ago.
55370.patch (484 bytes) - added by mukesh27 2 years ago.
Working patch
55370.diff (853 bytes) - added by SergeyBiryukov 13 months ago.
55370.1.diff (1.7 KB) - added by joedolson 11 months ago.
Refreshed patch

Download all attachments as: .zip

Change History (30)

#1 @sabernhardt
2 years ago

  • Component changed from Role/Capability to Formatting
  • Description modified (diff)
  • Focuses accessibility added

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#3 @ryokuhi
2 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 6.0

Hello @crs1138, and thanks for opening this ticket after bringing up discussion in the Gutenberg repository.
We reviewed this ticket today during the accessibility team's weekly bug-scrub.
We agreed that both aria-expanded and aria-controls, but also aria-current, should be added to the list of allowed attributes.
As the related patch seems to be very easy to create and test, we're milestoning this ticket for 6.0 and adding the good-first-bug label.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#5 @rsiddharth
2 years ago

  • Keywords has-patch added; needs-patch removed

@ryokuhi, I've uploaded the initial version of the patch.

#6 @rsiddharth
2 years ago

Ignore 53234.2022032000.patch; uploaded the wrong one.

#7 follow-up: @mukesh27
2 years ago

Thanks for the patch!

Using which method do you have created the patch?

If you're doing a first time contribution then check these links in order to create a valid patch in Trac.

https://make.wordpress.org/core/handbook/tutorials/working-with-patches/
https://make.wordpress.org/core/handbook/best-practices/writing-patches/

If you're familiar with code and git, you can use the wordpress-develop Github repository to easily contribute to WordPress: https://make.wordpress.org/core/handbook/contribute/git/

@mukesh27
2 years ago

Working patch

#8 in reply to: ↑ 7 @rsiddharth
2 years ago

Replying to mukesh27:

Thanks for the patch!

Using which method do you have created the patch?

If you're doing a first time contribution then check these links in order to create a valid patch in Trac.

https://make.wordpress.org/core/handbook/tutorials/working-with-patches/
https://make.wordpress.org/core/handbook/best-practices/writing-patches/

If you're familiar with code and git, you can use the wordpress-develop Github repository to easily contribute to WordPress: https://make.wordpress.org/core/handbook/contribute/git/

Thank you, @mukesh27.

I created the patch using svn diff as described in https://make.wordpress.org/core/handbook/tutorials/working-with-patches/#creating-a-patch-with-the-command-line

I'm using the svn checkout at https://develop.svn.wordpress.org/trunk/ for creating the patches.

From the patch you uploaded it looks like you're using https://core.svn.wordpress.org/trunk/

The documentation at https://make.wordpress.org/core/handbook/tutorials/installing-wordpress-locally/from-svn/#1-2-using-command-line uses https://develop.svn.wordpress.org/trunk/ svn checkout.

Which svn checkout is the correct one to use?

Last edited 2 years ago by rsiddharth (previous) (diff)

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


2 years ago

#10 @sabernhardt
2 years ago

Thanks for the patches! I use develop for SVN patches. The pull request method is another option, but should not be necessary.

55370.v202203242230.patch applies cleanly for me at the root folder, and 55370.patch applies to the src.

#11 @peterwilsoncc
2 years ago

  • Milestone changed from 6.0 to Awaiting Review
  • Version 5.8.3 deleted

On #30421 there is quite a discussion about whether or not there are any security implications adding the aria-* attributes globally.

The conversation was never resolved one way or the other as it was decided to support a sub-set of attributes initially.

I'm going to move this ticket back in to review to allow time for the discussion to happen about these attributes.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#14 @audrasjb
2 years ago

FWIW, I don't see any security reason to not allowlist at least aria-expanded, aria-controls and aria-current attributes.

#15 @joedolson
14 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


13 months ago

#17 @ryokuhi
13 months ago

  • Keywords needs-testing added

#18 @SergeyBiryukov
13 months ago

Refreshed the patch to alphabetize the list and add a @since tag.

#19 @nataliat2004
13 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/ticket/55370

Environment

  • OS: macOS 13.1
  • Web Server: Nginx
  • PHP: 8.0
  • WordPress: 6.3
  • Browser: Chrome 111.0.5563.146, Safari 16.2
  • Theme: Twenty Twenty-Three
  • Active Plugins:

Actual Results

  • ✅ Issue resolved with a patch.
  • aria-controls and aria-expanded attributes were not stripped.
  • tested under admin and author roles

Additional Notes

  • For custom collapsible block I used next plugins: Ultimate Blocks 2.5.8 (Content Toggle and Expand), Spectra 2.4.1 (FAQ) and Easy Accordion 2.2.2

Supplemental Artifacts

https://monosnap.com/file/bsELeWUa3ksNU5jzp0DLhkzpzL5LpMhttps://monosnap.com/file/8jrTP5lHZFMYMmyophgX0MT8rc58CV

Last edited 13 months ago by nataliat2004 (previous) (diff)

@joedolson
11 months ago

Refreshed patch

#20 @joedolson
11 months ago

  • Keywords commit added; needs-testing removed

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


10 months ago

#23 @joedolson
10 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55937:

Formatting: Support aria content attributes.

Add aria-controls, aria-expanded, and aria-current to allowed attributes in KSES.

Props crs1138, rsiddharth, mukesh27, SergeyBiryukov, joedolson, ryokuhi, peterwilsoncc, audrasjb, nataliat2004.
Fixes #55370.

@joedolson commented on PR #4634:


10 months ago
#24

Fixed in r55937

#25 @milana_cap
9 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.