Problem/Motivation

Toolbar module causes page to not follow W3C.
Attribute aria-owned-by not allowed on element div at this point.

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#10 remove-unrelated-aria-tag-and-update-existing-2471809-10.patch929 bytesskippednote
#2 remove-unvalid-aria-toolbar-2471809-2.patch586 bytesDom.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Related issues: +#2471529: Review aria attributes use
Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Active » Needs review
FileSize
remove-unvalid-aria-toolbar-2471809-2.patch586 bytes

To be honest, I can't even find a reference to aria-owned-by neither in Google nor on the aria spec document (http://www.w3.org/TR/html-aria/). Thus here is a patch to actually remove it.

axe312’s picture

axe312 CreditAttribution: axe312 commented

This might provide more infos about the aria attribute:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Issue tags: +Novice
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Critical here is to know if screenreaders use it, whether it's in the spec or not. Accessibility is for people not validators. Once that's confirmed (or not) we can talk about a patch. I know a screenreader user has been involved in the making of toolbar to check accessibility, so it's probably not been added for the fun of it.

Dom.’s picture

Dom. CreditAttribution: Dom. commented

@nod : true. Just I can see anything of aria-owned-by in the doc.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Related issues: +#1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop

Attaching #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop which originally added the aria-owned-by attribute.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Pinged Jesse Beach :) https://twitter.com/wimleers/status/593439139057512448

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Dom, thank you for bringing this issue up.

You're right, aria-owned-by doesn't exist in the spec. What we're trying to do here is create a relationship between DOM elements that isn't represented through hierarchy. That relationship should be established with the aria-owns (http://www.w3.org/TR/wai-aria/states_and_properties#aria-owns) attribute on the toolbar button that opens the tab. This code is there, but I believe it's wrong. This is the code now.

$attributes += array(
  'data-toolbar-tray' => $id . '-tray',
  'aria-owns' => $id,
  'role' => 'button',
  'aria-pressed' => 'false',
);

It should be 

$attributes += array(
  'data-toolbar-tray' => $id . '-tray',
  'aria-owns' => $id . '-tray',
  'role' => 'button',
  'aria-pressed' => 'false',
);

aria-owns should indicate the ID of the DOM element that this element owns.

Dom, could you update your patch to reflect this?

skippednote’s picture

skippednote CreditAttribution: skippednote at Axelerant commented
FileSize
remove-unrelated-aria-tag-and-update-existing-2471809-10.patch929 bytes

Tested Dom's patch and the aria-owned-by errors goes away when I run it through the W3C validator.
I've attached a patch with both Dom's and jessebeach's changes included.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Title: Toolbar module does follow W3C » Toolbar module does not follow W3C
Issue tags: +Needs accessibility review, +Accessibility

Adding missing word to make the title less confusing. :)

Also adding some tags.

Status: Needs review » Needs work

The last submitted patch, 10: remove-unrelated-aria-tag-and-update-existing-2471809-10.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

PS: Nice to see you again, jessebeach! :D Thanks for stopping by!

Status: Needs work » Needs review

himanshupathak3 queued 10: remove-unrelated-aria-tag-and-update-existing-2471809-10.patch for re-testing.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs review » Reviewed & tested by the community

"This document was successfully checked as HTML5!" using #10 !
RTBC since I have :
- applied and check patch on latest trunk : it now validate the page using W3C.
- checked manually all instance of "aria-owns" in the generated source to check it references an existing ID
- read through the patch for coding standars
For all this reasons, move this issue to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: remove-unrelated-aria-tag-and-update-existing-2471809-10.patch, failed testing.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs work » Needs review

Hum.. test fail seems weird here... can't clone git repo ?!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Testbot is completely broken today :( We'll have to wait until the DA fixes the testbots…

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

LGTM.

PPS: Hi webchick!!

webchick queued 10: remove-unrelated-aria-tag-and-update-existing-2471809-10.patch for re-testing.

Dom.’s picture

Dom. CreditAttribution: Dom. commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC then after #15.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: -w3c, -Needs accessibility review

Removing w3c tag, the w3c validation seems to be used a little more. w3c alone isn't very clear.

alexpott’s picture

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

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 437fa37 and pushed to 8.0.x. Thanks!

  • alexpott committed 437fa37 on 8.0.x 
    Issue #2471809 by Dom., skippednote, nod_, jessebeach: Toolbar module...

Status: Fixed » Closed (fixed)

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