Opened 9 years ago

Closed 7 years ago

#12135 closed defect (fixed)

[PATCH][CLA] dijit.Tree has the wrong levels

Reported by: liucougar Owned by: mikeb
Priority: blocker Milestone: 1.8
Component: Accessibility Version: 1.5
Keywords: Cc: cjolif, mikeb
Blocked By: Blocking:

Description

if showRoot is false, all the visible root element are announced as level 2 items by JAWS 12

Attachments (5)

12135.patch (1000 bytes) - added by liucougar 9 years ago.
proposed patch
rootlessTree.patch (2.7 KB) - added by mikeb 7 years ago.
fixes aria-multiselectable and aria-expanded for rootless trees; update Tree_a11y test
tree_a11y_fixes_race_condition.patch (7.0 KB) - added by mikeb 7 years ago.
fixes race condition by setting multiselectable in postCreate after rootNode has been set up
tree_fix_a11y_race_with_onLoadDeferred.patch (6.7 KB) - added by mikeb 7 years ago.
fixes aria-multiselectable race condition with deferred
tree_race_condition_move_load.patch (8.2 KB) - added by mikeb 7 years ago.

Download all attachments as: .zip

Change History (24)

Changed 9 years ago by liucougar

Attachment: 12135.patch added

proposed patch

comment:1 Changed 9 years ago by liucougar

Summary: dijit.Tree has the wrong levels[PATCH][CLA] dijit.Tree has the wrong levels

the patch assigns role=tree to the invisible root tree node's containerNode, and set role=presentation on the invisible root node if showRoot is false

this fixes the issue with the screen reader

comment:2 Changed 9 years ago by liucougar

Owner: changed from cougar to liucougar

comment:3 Changed 9 years ago by liucougar

Resolution: fixed
Status: newclosed

(In [23487]) fixes #12135: fixes wrong level of treeitems in dijit.Tree with showRoot=false

modified robot test to check this

!strict

comment:4 Changed 7 years ago by mikeb

Milestone: 1.6
Resolution: fixed
Status: closedreopened

Hello. We have been seeing problems with incorrect aria properties on rootless trees, the attached patch moves the aria-multiselectable and aria-expanded properties from the root node to the rootless tree's container node (like the original patch.) Everything still sounds and works the same with JAWS13. This patch is for the nightly build. CLA on file with IBM

comment:5 Changed 7 years ago by mikeb

Cc: cjolif added
Owner: changed from liucougar to mikeb
Status: reopenedassigned

comment:6 Changed 7 years ago by bill

This patch breaks the regression test:

     _AssertFailure: assertEqual() failed: 	expected
		true
	but got
		null

 with hint: 
		mytree: aria state expanded=true
     ERROR IN: 		 function ariaTreeStateExpanded(){
					for(i=0; i<treeIds.length; i++){
						var tree = registry.byId(treeIds[i]);
						var wasExpanded = tree.rootNode.isExpanded;
						tree.rootNode.expand();
						var nowExpanded = tree.domNode.getAttribute("aria-expanded");
						tree.rootNode.collapse();
						var nowCollapsed = tree.domNode.getAttribute("aria-expanded");
						if(wasExpanded){
							tree.rootNode.expand();
						}
						doh.is("true", nowExpanded, tree.id + ": aria state expanded=true");
						doh.is("false", nowCollapsed, tree.id + ": aria state expanded=false ");
					}
				} FAILED test: ../../dijit/tests/tree/robot/Tree_a11y.html::a11yAria::ariaTreeStateExpanded 78 ms

Changed 7 years ago by mikeb

Attachment: rootlessTree.patch added

fixes aria-multiselectable and aria-expanded for rootless trees; update Tree_a11y test

comment:7 Changed 7 years ago by mikeb

Oops. I updated the patch to fix the test case. Verified with JAWS13 that the levels (and grouping of treenodes) are announced correctly.

comment:8 Changed 7 years ago by bill

Milestone: 1.8

OK, not sure if this patch should have been attached to a ticket that was already closed and put into a release (ie, 1.6), but anyway the regression tests are working for me now (thanks), so I'll check this in.

comment:9 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [28931]:

Fixes for dijit/Tree and JAWS screenreader where Tree.showRoot == false, fixes #12135 !strict, thanks Mike.

comment:10 Changed 7 years ago by bill

In [28932]:

oops, previous change introduced a race condition, commenting out until better fix, refs #12135 !strict

comment:11 Changed 7 years ago by bill

Mike - your change introduces a race condition on startup. The Tree tests didn't catch it but I happened to notice when testing the doc viewer. The problem is that the Tree rootNode may not yet exist when _dndSelector.js is initialized, so this line is dangerous:

this.tree.rootNode.containerNode.setAttribute("aria-multiselectable", !this.singular);

Particularly if the rootNode data is being fetched via XHR. So, please provide a patch without that problem.

comment:12 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

comment:13 Changed 7 years ago by bill

Cc: mikeb added
Priority: highblocker

Mike, I need a fix to that race condition, otherwise I'll have to rollback your change.

Changed 7 years ago by mikeb

fixes race condition by setting multiselectable in postCreate after rootNode has been set up

comment:14 Changed 7 years ago by mikeb

Hi Bill. I had a lot of trouble reproducing the race condition but the patch should fix it. I moved the code setting multiselectable in postCreate after the call to _load (which sets up the rootNode.) I also updated all of the non robot tests to be WAI-ARIA compliant.

comment:15 Changed 7 years ago by bill

No, you can't access this.rootNode in postCreate(), because postCreate() could run before this.rootNode is created. this.rootNode is created when this.model.getRoot() calls it's callback.

Changed 7 years ago by mikeb

fixes aria-multiselectable race condition with deferred

comment:16 Changed 7 years ago by mikeb

Hi Bill. I saw today you checked in a fix for another race condition (#14058) and it looks like I can just wait for that deferred to finish and then assign the aria attributes.

comment:17 Changed 7 years ago by bill

onLoadDeferred has existed for months, but presumably the correct spot for setting aria roles on rootNode/domNode is when rootNode is created, and there's already code for that at http://bugs.dojotoolkit.org/browser/dojo/dijit/trunk/Tree.js?rev=29266#L864

Changed 7 years ago by mikeb

comment:18 Changed 7 years ago by mikeb

Put the aria code where it belongs, moved the call to _load() to after the dndController is created so that dndController.singular is there, thanks Bill

comment:19 Changed 7 years ago by bill

Resolution: fixed
Status: reopenedclosed

In [29284]:

Fixes race condition setting aria roles before Tree rootNode is created, plus test updates, thanks Mike (IBM, CCLA), fixes #12135 again, !strict.

Note: See TracTickets for help on using tickets.