Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#16242 closed defect (fixed)

[PATCH][CCLA] dijit.layout.AccordionContainer: accessibility fixes

Reported by: mikeb Owned by: mikeb
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

It seems that we only need to make a few slight changes to AccordionContainer? to make it follow the WAI-ARIA best practices guide (http://www.w3.org/TR/wai-aria-practices/#accordion) and also remove the accessibility violations that internal tools are discovering.

The first thing is to set the role of the content area to "tabpanel."

Next we needed to remove aria-multiselectable attribute. This was getting set on the tabpanel area if you create an accordion pane with a Tree inside of it. I will investigate this further when I look at Tree accessibility.

Since we added role=tabpanel, we have to set aria-label attribute. If there isn't an aria-label, I just point to the text of the tab button via aria-labelledby. This required a slight addition to the template.

I fixed the test pages but they will be uploaded together as a separate ticket.

Attachments (4)

layout-accordian-a11y.patch (1.8 KB) - added by mikeb 7 years ago.
Accessibility fixes for AccordionContainer? including role=tabpanel and aria label. Please proxy commit for Michael Billau CCLA on file with IBM
layout-accordian-a11y.2.patch (1.2 KB) - added by mikeb 7 years ago.
layout-accordian-a11y.4.patch (4.0 KB) - added by mikeb 7 years ago.
Try to fix a11y roles on StackContainer? family of dijits by wrapping children with a div for roles before they go into the Container
layout-accordion-a11y.5.patch (6.4 KB) - added by bill 7 years ago.
updated previous patch with tests to show issue with addChild()

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by mikeb

Attachment: layout-accordian-a11y.patch added

Accessibility fixes for AccordionContainer? including role=tabpanel and aria label. Please proxy commit for Michael Billau CCLA on file with IBM

comment:1 Changed 7 years ago by mikeb

Component: GeneralDijit
Owner: set to bill

comment:2 Changed 7 years ago by bill

Owner: changed from bill to mikeb
Status: newpending

I have some questions on this one too.

You are adding role=tabpanel to the ContentPane/Tree/etc. child of the AccordionContainer. But StackContainer adds role=tabpanel to the StackContainer.containerNode, i.e. to the parent. This is inconsistent, isn't it? http://test.cita.uiuc.edu/aria/tabpanel/tabpanel2.php list role=tabpanel on the child, so maybe this means the code in StackContainer.js is wrong.

I'm also uneasy about the Tree related change. Tree's normally need aria-multiselectable set on them. How can you remove that attribute just because the Tree is a child of an AccordionContainer?

comment:3 Changed 7 years ago by mikeb

Status: pendingnew

I was accidentally setting role=tabpanel on the child of the AccordionContainer?. This was fine when the panes were just ContentPanes? and did not have a role. The problem was when some other dijit was used, like the Tree, and I was rewriting the role. This is why I had to remove the aria-multiselectable. To fix it I just decided to move the role=tabpanel up one node to the element that wraps the contentWidget. This also needs aria-label which should be the same as whatever title they set on the pane. I added multiselectable to the root of the AccordionContainer? as per best practices.

Changed 7 years ago by mikeb

comment:4 Changed 7 years ago by bill

Milestone: tbd1.9
Status: newpending

OK. That looks better for AccordionContainer. The one issue I have is, why are you setting aria-multiselectable? You can only select one accordion child at a time, so it seems like aria-multiselectable shouldn't be there (or should be set to false), according to http://www.w3.org/TR/wai-aria/states_and_properties#aria-multiselectable.

The other bigger problem is that you can't use the same approach for StackContainer or TabContainer because they don't have a wrapper <div> around each child.

So looking at StackContainer and in the current code, there are apparently two problems:

  • sets role=tabpanel of StackContainer.domNode, which makes no sense
  • conversely, there's no role=tabpanel for the child widgets, or a div node wrapping the child widgets

TabContainer is in exactly the same boat. I'm looking at http://test.cita.uiuc.edu/aria/tabpanel/tabpanel1.php#lsc for reference.

Agree?

Last edited 7 years ago by bill (previous) (diff)

comment:5 Changed 7 years ago by mikeb

Status: pendingnew

I was setting aria-multiselectable on the AccordionContainer? because I was following the Best Practices guide (http://www.w3.org/TR/wai-aria-practices/#accordion). It says to use aria-multiselectable so assistive technologies can determine it is an accordion container. However, I tested setting aria-multiselectable on the main DOM node and there was no difference with JAWS13, so I removed it.

For StackContainer? and TabContainer?, I thought it would be best to just wrap the child widgets in a small <div> that we could then set the role and aria-labelledby attributes. I tried to do this in the updated patch (layout-accordian-a11y.4.patch). I set up the wrapper in _setupChild of StackContainer? and remember the wrapper div so that later in StackController? we can add the aria-labelledby attribute when we are creating the associated buttons. We need to do this extra step in StackController? because in StackContainer? the buttons (child.controlButton) may not be created yet. Also, we can't just leave aria-label because aria-labelledby is the more appropriate use since we are using the tab and tabpanel roles. However, it is possible that users can create a StackContainer? without an associated StackController? (the example page has one), so we need to have an aria-label for this (since having role=tabpanel needs an aria-label or aria-labelledby.)

For AccordionContainer? I just replace the new wrapper node with the child.domNode so that we don't have two wrappers.

Changed 7 years ago by mikeb

Try to fix a11y roles on StackContainer? family of dijits by wrapping children with a div for roles before they go into the Container

comment:6 in reply to:  5 ; Changed 7 years ago by bill

Status: newpending

Replying to mikeb:

I was setting aria-multiselectable on the AccordionContainer because I was following the Best Practices guide (http://www.w3.org/TR/wai-aria-practices/#accordion). It says to use aria-multiselectable so assistive technologies can determine it is an accordion container. However, I tested setting aria-multiselectable on the main DOM node and there was no difference with JAWS13, so I removed it.

OK, thanks. For the record I think they were setting it because in their accordions you can have multiple panes open simultaneously, similar to dojox/widget/TitleGroup. But for dijit/layout/AccordionContainer, only one pane can be open at a time.

For StackContainer and TabContainer, I thought it would be best to just wrap the child widgets in a small <div> that we could then set the role and aria-labelledby attributes. I tried to do this in the updated patch (layout-accordian-a11y.4.patch).

This is something that AccordionContainer does, but it's quite complicated to do correctly. I'll take a look over your patch; need to check that getChildren() still works correctly, and sizing too.

I'm wondering though if StackContainer should just [re]set the roles on its children to be tabpanel, rather than having the wrapper div. Is there a practical advantage to having nested divs with two separate roles?

I set up the wrapper in _setupChild of StackContainer and remember the wrapper div so that later in StackController we can add the aria-labelledby attribute when we are creating the associated buttons. We need to do this extra step in StackController because in StackContainer the buttons (child.controlButton) may not be created yet. Also, we can't just leave aria-label because aria-labelledby is the more appropriate use since we are using the tab and tabpanel roles. However, it is possible that users can create a StackContainer without an associated StackController (the example page has one), so we need to have an aria-label for this (since having role=tabpanel needs an aria-label or aria-labelledby.)

Right, and there could also be multiple StackController widgets controlling a single StackContainer, although I guess in that unusual case it wouldn't matter which one was referenced.

I'll wait for an answer about the nested divs, but in the meantime will check in the other change to AccordionContainer.

comment:7 Changed 7 years ago by bill

In [29898]:

Add role=tabpanel and aria-labelledby to wrappers of AccordionContainer child widgets, refs #16242 !strict.

comment:8 in reply to:  6 Changed 7 years ago by mikeb

Status: pendingnew

Replying to bill:

I'm wondering though if StackContainer should just [re]set the roles on its children to be tabpanel, rather than having the wrapper div. Is there a practical advantage to having nested divs with two separate roles?

StackContainer? cannot just set the roles on its children to tabpanel because that child may already have a role that will get reset. This will cause problems when there are role-specific aria attributes on the child. For example, if you use a Tree in a StackContainer?, it will have role=tree and aria-multiselectable. But since there is no wrapper for the child, if we just reset the role to tabpanel, we'll have to remove all of the tree related aria attributes. Then we also have the bigger problem of now the child tree dijit is not accessible. So yes, the practical advantage of having two nested divs with separate roles exists as long as those roles are different.

Right, and there could also be multiple StackController widgets controlling a single StackContainer, although I guess in that unusual case it wouldn't matter which one was referenced.

aria-labelledby allows us to put a list of IDs, so I think in this case we should just list all of the buttons. This is what I did with the var labelledby = page._wrapper.getAttribute("aria-labelledby") ? page._wrapper.getAttribute("aria-labelledby") + " " + button.id : button.id;

(Also I'm not sure why my posts keep resetting the ticket status to "new" - sorry.)

Last edited 7 years ago by mikeb (previous) (diff)

comment:9 Changed 7 years ago by bill

I see, makes sense, OK I'll look more closely at your patch to StackContainer. It's scary to add an extra <div> but I do see your reasoning.

Changed 7 years ago by bill

updated previous patch with tests to show issue with addChild()

comment:10 Changed 7 years ago by bill

In [29904]:

For aria compliance, StackContainer's children need role="tabpanel", but since the child widgets may have their own roles and aria attributes, for example Tree, put a wrapper <div role="tabpanel"> around each child widget.

Modifies _Container.addChild(widget, num) to position widget as the n+1'th direct child of _Container.containerNode, even if the child widgets are not direct children of _Container.containerNode. This is necessary for StackContainer.addChild() to keep working after the above change, and also simplifies AccordionContainer.addChild(), which is complicated due to the AccordionInnerContainer wrapper widgets.

Also sets aria-label or preferably aria-labelledby on each <div role="tabpanel">.

Patch partly from Mike Billau (IBM, CCLA).

Refs #16242, #16244 !strict.

comment:11 Changed 6 years ago by bill

Resolution: fixed
Status: newclosed

IIUC this is now fixed. If not please comment back.

comment:12 Changed 6 years ago by bill

In [30088]:

Hide and show StackContainer's child's wrapper, rather than child itself, to allow app to do CSS styling of children and their wrappers to height: 100%.

Also fixing setting of aria-label. It shouldn't be set on the node unless there's a label defined (otherwise you end up with aria-label="undefined", and also need to worry about escaping special characters.

Fixes #16394, refs #16242, #16244 !strict

comment:13 Changed 6 years ago by bill

In [30189]:

Clear the page._wrapper reference of StackContainer?.removeChild(page). Because although the wrapper was destroyed, the page will live on... unless removeChild() was called as part of the destruction of the StackContainer. Refs #16242, #16244 !strict

comment:14 Changed 6 years ago by bill

In [30252]:

Fix SplitContainer?.addChild() breakage from [29904]. Fixes #16504, refs #16242, #16244 !strict.

comment:15 Changed 6 years ago by Douglas Hays

In [30321]:

Refs #16242. Backport [29898] to 1.8.

Note: See TracTickets for help on using tickets.