#16244 closed defect (fixed)
[PATCH][CCLA] dijit.layout clean up aria labels and roles for tab containers
Reported by: | mikeb | Owned by: | mikeb |
---|---|---|---|
Priority: | undecided | Milestone: | 1.9 |
Component: | Dijit | Version: | 1.8.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
There were a few slight issues with dijit.layout test pages throwing accessibility violations.
The patch fixes these violations:
- 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.
- 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.
- 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.
Attachments (1)
Change History (17)
Changed 7 years ago by
Attachment: | layout-tab-controllers-a11y.patch added |
---|
comment:1 Changed 7 years ago by
Owner: | changed from bill to mikeb |
---|---|
Status: | new → pending |
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?
comment:2 Changed 7 years ago by
Status: | pending → new |
---|
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.
comment:3 Changed 7 years ago by
Milestone: | tbd → 1.9 |
---|
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.
comment:16 Changed 7 years ago by
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?
comment:17 follow-up: 18 Changed 7 years ago by
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.
comment:18 Changed 7 years ago by
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?
comment:19 Changed 7 years ago by
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.
Accessibility fixes for the rest of dijit.layout, please proxy commit for Michael Billau CCLA on file with IBM