[PATCH][CCLA] dijit.layout clean up aria labels and roles for tab containers

[mikeb]

There were a few slight issues with dijit.layout test pages throwing accessibility violations.

The patch fixes these violations:

1. in ScrollingTabController?, remove aria-owns from the _popupStateNode. This refers to the drop down menu that gets created. For a ScrollingTabController?, it looks like we remove the menu from the DOM tree when the user closers it. This means that we also have to remove the aria-owns attribute since the ID is no longer valid. We didn't have to do this for 15885 because ScrollingTabController? seems to be the only one that removes the DOM node.

2. in StackContainer?, add aria-label to the containerNode if there is not already a label. This is required because we are setting the containerNode to role="tabpanel" which requires an aria-label.

3. in StackController?, remove the aria-labelledby pointing to newButton.id. I'm not really sure what this was being used for. There isn't a role attribute on this element so aria-labelledby wouldn't be needed.

I updated all of the test pages but will attach them in a separate ticket.

[mikeb]

Accessibility fixes for the rest of dijit.layout, please proxy commit for Michael Billau CCLA on file with IBM

[bill]

About (2), it seems pointless to set StackContainer.containerNode's aria-label to the StackContainer's id. Often the id will be a meaningless auto-generated string. <input> tags need an associated <label> tag, or at least you need to set an aria-label attribute on the <input>. Maybe the same rule should apply for StackContainer and descendants?

[mikeb]

Hi Bill,

You are right, setting the aria-label to the auto-generated ID will probably not help out users with disabilities that much. Developers should be setting aria-label on the StackContainer? itself - if they don't, they will get an accessibility violation. However, I put code in place to set aria-label if they don't already have it because I was thinking that it would be a more proactive approach to accessibility. A "do it for them if they can't remember" sort of thing. (Or if they can't actually add the aria-label...maybe there any instances where a dijit creates its own StackContainer? and would need to set aria-label.)

But I can see why we shouldn't clutter up the code with aria-* attributes if, for example, they purposely are ignoring adding accessibility. It probably does make more sense to force/require developers to add aria-label attribute to StackContainer? themselves anyway, since accessibility is something they should be thinking about from the beginning.

I will submit a suggestion to add a note about using aria-label to the documentation for StackContainer? once this ticket goes through.

[bill]

OK, so I will check in the patch except for that part. Also, I'm going to stop setting role="tabpanel" on StackContainer.containerNode, since IIUC it's supposed to be set on each child, not on the wrapper.

[bill]

In [29885]:

A11y updates for StackContainer and TabContainer:

1. In ScrollingTabController, remove aria-owns from this._popupStateNode when the corresponding menu is destroyed. Otherwise it's pointing to a node that no longer exists.
2. In StackContainer, remove role="tabpanel" on this.containerNode since IIUC that is actually supposed to be set on the children of the StackContainer
3. In StackController, remove the aria-labelledby pointing to newButton.id. There isn't a role attribute on this element so aria-labelledby isn't needed.

Patch mainly from Mike Billau (IBM, CCLA), thanks.

Fixes #16244 !strict, although as I mentioned in #16242 I think there's still more accessibility work left for StackContainer and TabContainer.

[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.

[bill]

In [29932]:

Remove disclaimer no longer needed after [29904], refs #16244 !strict

[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

[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

[bill]

In [30252]:

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

[Douglas Hays]

In [30320]:

Refs #16244. Backport [29885] to 1.8. !strict

[Eric Durocher]

It seems like [29904] broke something in dojox/mobile/_EditableIconMixin.js.

Launch dojox/mobile/tests/test_IconContainer-editable.html, click Start Edit, click and drag one of the icons, move it, release, click again => _Container.js throws 'Cannot read property 'nodeType' of null' at line 38. The case of adding a child at the end seems broken?

[bill]

Hmm, I do see it's broken, although maybe it's the mobile code that needs to be changed. It's doing something complicated with the drag and drop, having a phantom element. There are 14 real children but when you start to drag there's suddenly a 15th child too, with a "" label, in the place where the node being dragged used to be. But the second time you try to drag, it's calling this.paneContainerWidget.addChild(node, 15) when there are only 14 children (presumably there's no phantom element). I didn't debug more than that.

[Eric Durocher]

OK I see the problem... Yes _EditableIconMixin calls addChild(node, 15) in a 14-children container... I think it just used to behave like addChild(node, 14) before [29904], so it worked well. I will fix this on our side (​http://bugs.dojotoolkit.org/ticket/16597).

I don't know if the addChild behavior change was intended though, I guess it is just a limit case that was not explicitly documented?

[bill]

I didn't intend the behavior change but I was never expecting addChild() to work with an invalid index either. But if a lot of people get hit by it we could add in guard code for indexes that are too high.

[bill]

In [30677]:

Move this.open() call from postCreate() to startup(). Fixes problem with double call to iframe's onload method when StackContainer._setupChild() messes around with the DOM. Also probably allows Editor creation without specifying a srcNodeRef. It does require calling Editor.startup() though. Refs #16244 ([29904]), fixes #16756 !strict.

[bill]

In [31274]:

Fix regression where selectChild() fails when called before startup()... although not sure if that's something we want to support in 2.0. Also doing a small cleanup in _setupChild(). Refs #16244 !strict.