WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

Last modified 5 months ago

#30421 closed enhancement (fixed)

Add ARIA attributes to globally permitted HTML attributes in kses

Reported by: jwenerd Owned by: jorbin
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: kses early has-unit-tests needs-testing needs-patch fixed-5.0
Focuses: accessibility, administration Cc:

Description

Core currently supports the role attribute as globally permitted. It would be helpful if ARIA attributes were permitted here. This would be helpful so that users without the unfiltered_html capability could use ARIA within their content.

I can do this with a plugin, but currently run into issues making TinyMCE not strip away the ARIA attributes when toggling between the text and visual mode.

Attachments (5)

aria-kses-test.diff (1.7 KB) - added by mattheu 4 years ago.
aria-kses-1.diff (2.3 KB) - added by mattheu 4 years ago.
Add all aria attributes to global attributes
aria-kses-2.diff (548 bytes) - added by mattheu 4 years ago.
Simpler - permit any aria-* attributes
30421.diff (4.0 KB) - added by swissspidy 4 years ago.
30421.2.diff (514 bytes) - added by peterwilsoncc 10 months ago.

Download all attachments as: .zip

Change History (42)

#1 @boonebgorges
5 years ago

  • Version trunk deleted

#2 @DrewAPicture
5 years ago

  • Component changed from General to Formatting

#3 @miqrogroove
5 years ago

  • Keywords kses added

#4 @jorbin
5 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.2

We should fully support people that want to make there sites more accessible. Let's do this.

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


5 years ago

#6 @garcialo
5 years ago

Here is a list of HTML elements and whether to use ARIA and which attributes to use.

http://www.w3.org/TR/aria-in-html/#recommendations-table

#7 @afercia
5 years ago

Also, the HTML5 spec lists allowed roles and attributes for each element, e.g.:
http://www.w3.org/TR/html5/grouping-content.html#the-ul-element

#8 @lukecarbis
5 years ago

The list of tags which aren't allowed to use aria-* attributes is very short. Should we allow aria-* for all tags?

Version 0, edited 5 years ago by lukecarbis (next)

@mattheu
4 years ago

Add all aria attributes to global attributes

@mattheu
4 years ago

Simpler - permit any aria-* attributes

#9 @mattheu
4 years ago

I'm not sure whe want to go too far down the path of trying to validate aria attributes and should probably allow aria-* attributes on all elements. The bigger complexity is that certain aria attributes are only supported for certain role attributes.

I have uploaded a few patches.

The first adds unit tests for all the allowed aria attributes. It currently fails.

The second patch - I looked into adding all the aria attributes to global attributes. This works OK, but the list of attributes is a long one, and we would have to maintain this if anything changes. It is useful if we want to go further with this.

The third patch is a bit simpler and just whitelists any attributes that start with aria-.

This ticket was mentioned in Slack in #core by drew. View the logs.


4 years ago

#11 @jorbin
4 years ago

  • Keywords early added
  • Milestone changed from 4.2 to Future Release

https://core.trac.wordpress.org/attachment/ticket/30421/aria-kses-1.diff combined with https://core.trac.wordpress.org/attachment/ticket/30421/aria-kses-test.diff is really close to what I think we should go with, but we need to do a bit more verification that none of these are going to introduce any unintended security holes.

I'm adding the early tag as I think a bit more verification on what each aria attribute does to ensure they can't be exploited, along with making sure we have all of the aria attributes covered this can make it in early in 4.3

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


4 years ago

#13 @wonderboymusic
4 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to jorbin
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

#15 follow-up: @jorbin
4 years ago

  • Milestone changed from 4.4 to Future Release

Punting. Someone still needs to do some research to show that aria attributes can't be used to create security issues (yes, I know proving a negative is hard)

#16 in reply to: ↑ 15 ; follow-up: @miqrogroove
4 years ago

Replying to jorbin:

Punting. Someone still needs to do some research to show that aria attributes can't be used to create security issues (yes, I know proving a negative is hard)

For the record, our standard for entry is significantly higher than that. The KSES whitelist is used to allow only the elements and attributes that should be used in anonymous comments $allowedtags or in non-administrative posts by contributors $allowedposttags.

In addition to safety, we need a convincing argument that a proposed entry is needed for one of those author groups.

For the proposed ARIA feature, specifically, I see no reason why this would ever be used in anonymous comments. It is neither needed nor desirable in most situations. According to the ticket description "This would be helpful so that users without the unfiltered_html capability could use ARIA within their content. I can do this with a plugin." I would like to know in what situation is this actually useful? Who has non-admin contributors that are trying to use ARIA? Is a plugin not adequate for those who need this feature?

#17 in reply to: ↑ 16 ; follow-up: @jorbin
4 years ago

Replying to miqrogroove:

Replying to jorbin:

Punting. Someone still needs to do some research to show that aria attributes can't be used to create security issues (yes, I know proving a negative is hard)

For the record, our standard for entry is significantly higher than that. The KSES whitelist is used to allow only the elements and attributes that should be used in anonymous comments $allowedtags or in non-administrative posts by contributors $allowedposttags.

In addition to safety, we need a convincing argument that a proposed entry is needed for one of those author groups.

For the proposed ARIA feature, specifically, I see no reason why this would ever be used in anonymous comments. It is neither needed nor desirable in most situations. According to the ticket description "This would be helpful so that users without the unfiltered_html capability could use ARIA within their content. I can do this with a plugin." I would like to know in what situation is this actually useful? Who has non-admin contributors that are trying to use ARIA? Is a plugin not adequate for those who need this feature?

The standard also is that all users should be able to create accessible content. You shouldn't need to have unfiltered_html or to install a plugin in order to make sure that all end users can have a great experience reading content you create. While many of the aria attributes have little likelihood of being useful in comments(aria-label and aria-labelledby being notable exceptions), there is a high likelihood that the others they would be usefull in posts.

#18 in reply to: ↑ 17 ; follow-up: @miqrogroove
4 years ago

Replying to jorbin:

You shouldn't need to have unfiltered_html or to install a plugin in order to make sure that all end users can have a great experience reading content you create. While many of the aria attributes have little likelihood of being useful in comments(aria-label and aria-labelledby being notable exceptions), there is a high likelihood that the others they would be usefull in posts.

Again, we're not talking about end users, or the content that I create. KSES is a specific limitation for anonymous and non-administrative authors. If there is little likelihood of ARIA usage in comments, then we would not "Add ARIA attributes to globally permitted HTML attributes". Perhaps we should re-focus the intent and ticket summary toward augmenting $allowedposttags only.

And again, do any non-administrative authors need to use ARIA in posts? The description of ARIA on the linked pages very clearly states that this is a scripting technology, which is something we generally block from such posts.

@swissspidy
4 years ago

#19 @swissspidy
4 years ago

  • Keywords has-patch has-unit-tests needs-testing added; needs-patch needs-unit-tests removed

30421.diff is an updated patch that bundles aria-kses-1.diff and aria-kses-test.diff and applies cleanly against trunk.

Per comment:15 this needs testing / research.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#20 in reply to: ↑ 18 @GaryJ
4 years ago

Replying to miqrogroove:

Again, we're not talking about end users, or the content that I create. KSES is a specific limitation for anonymous and non-administrative authors.

I'm not sure if this is the same source of problem, but I'm an administrator on a WordCamp site (can't use a plugin), and adding an aria-label to a span in widget content, gets removed when the widget is saved. This results in less-than-ideal content for screen readers.

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


3 years ago

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


3 years ago

#23 @rianrietveld
3 years ago

Discussed this in the accessibility team meeting:
Because this functionality is used for content creation, we only need aria attributes that are used in the content.
This reduces the list dramatically.
Proposal by @afercia: add just aria-describedby
@GaryJ: do you know other aria attributes you may need?

#24 @joedolson
3 years ago

As Gary indicated in his previous comment, aria-label; and I'd also argue for aria-labelledby and aria-hidden. Outside of these, I suspect that most other aria attributes would only be practically useful being provided programmatically, rather than written into content.

#25 @joedolson
3 years ago

In reviewing the requirements for Authoring Tool Accessibility Guidelines, I note that in order to meet those guidelines, an authoring tool must not remove accessibility information added to content. Stripping out ARIA attributes does mean that accessibility information could be stripped out by the editor, violating that requirement.

I don't think that this means we should allow absolutely any usage of ARIA in the editor, since many attributes are impractical in normal usage in content, but it does emphasize the importance of allowing at least a basic set of attributes.

#26 @ocean90
3 years ago

#39288 was marked as a duplicate.

#27 @jeremyfelt
2 years ago

  • Keywords needs-patch added; has-patch removed

I did a bit of poking around and wasn't able to find any examples of attacks using these attributes.

It sounds like to move forward this ticket needs a patch that adds aria-describedby, aria-label, aria-labelledby, and aria-hidden to the list of allowed post tags in kses. It should probably also add to the allowed tags for TinyMCE too.

Heavy +1 btw. A plugin in the meantime: https://wordpress.org/plugins/allow-aria-attributes/ :)

#28 @afercia
2 years ago

There are a few, new, attributes in ARIA 1.1 (still a Candidate Recommendation at the time of writing) worth considering. For example:
https://www.w3.org/TR/wai-aria-1.1/#aria-current
https://www.w3.org/TR/wai-aria-1.1/#aria-details

More details about aria-current:

Using the aria-current attribute
http://tink.uk/using-the-aria-current-attribute/

Current support (January 2017)
https://ljwatson.github.io/design-patterns/aria-current/

Edit:
aria-current support landing in NVDA 2017.2
https://www.nvaccess.org/post/nvda-2017-2rc1-released/

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

#29 @pento
11 months ago

  • Milestone changed from Future Release to 5.0

Moving to 5.0, as several Gutenberg blocks add aria-* attributes which shouldn't be removed.

#30 @peterwilsoncc
10 months ago

I've looked through Gutenberg and it appears only the subset @rianrietveld and @joedolson mention above are required. Other aria attributes used by the new editor are only used by the edit screen.

I've uploaded 30421.2.diff with the subset aria-describedby, aria-details, aria-label, aria-labelledby and aria-hidden.

#31 @pento
10 months ago

👍🏻 Let's go with this subset for 5.0, we can investigate adding all aria-* attributes in a subsequent release.

#32 @peterwilsoncc
10 months ago

In 43731:

KSES: Add selected ARIA attributes support.

Allow low-privileged users to use the ARIA attributes aria-describedby, aria-details, aria-label, aria-labelledby and aria-hidden.

Props mattheu, swissspidy, rianrietveld, afercia, GaryJ.
See #30421.

#33 @peterwilsoncc
10 months ago

  • Keywords fixed-5.0 added

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


9 months ago

#35 @jeremyfelt
8 months ago

In 43984:

KSES: Add selected ARIA attributes support.

Allow low-privileged users to use the ARIA attributes aria-describedby, aria-details, aria-label, aria-labelledby and aria-hidden.

Merges [43731] to trunk.

Props mattheu, swissspidy, rianrietveld, afercia, GaryJ.
See #30421.

#36 @SergeyBiryukov
8 months ago

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

This ticket was mentioned in Slack in #core-editor by peterwilsoncc. View the logs.


5 months ago

Note: See TracTickets for help on using tickets.