Opened 11 years ago

Closed 11 years ago

#8971 closed defect (fixed)

[patch] [cla] tree.js typo? setWaitState()

Reported by: kyngchaos Owned by: bill
Priority: low Milestone: 1.4
Component: Accessibility Version: 1.3.0b2
Keywords: Cc: Becky Gibson
Blocked By: Blocking:

Description

tree.js, dijit._TreeNode, postCreate function - has setWaitState() instead of setWaiState():

if(this.isExpandable){
	dijit.setWaiState(this.labelNode, "expanded", this.isExpanded);
	if(this == this.tree.rootNode){
		dijit.setWaitState(this.tree.domNode, "expanded", this.isExpanded);
	}
}

Attachments (1)

8971.patch (382 bytes) - added by Joseph Scheuhammer 11 years ago.
Fixed typo by removing entire "if" statement.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 11 years ago by bill

Component: DijitAccessibility
Owner: set to Joseph Scheuhammer

This is from [15630], definitely a typo but should be tested again before checking in the change.

comment:2 Changed 11 years ago by Adam Peller

Cc: Becky Gibson added
Priority: normalhigh

comment:3 Changed 11 years ago by Joseph Scheuhammer

Yes, it is a typo. What's mysterious is that it hasn't caused a problem, yet. The reason, it appears, is that the line is not executed. I'm investigating to see if it can simply be removed -- want to be sure.

comment:4 Changed 11 years ago by Joseph Scheuhammer

The mistyped line is not called because:

  • either this is the postCreate() of the tree's rootNode, in which case this.tree.rootNode is 'undefined' (it's in the middle of being defined).
  • or, this is the postCreate() of a tree node other than the root node, in which case, this won't equal this.tree.rootNode.

In either case, if(this == this.tree.rootNode) always fails. This is conditional that contains the mistyped call to setWaiState().

The file "8971.patch" removes the entire if statement, including the type. Tested using the automated ".../dijit/tests/robot/Tree_a11y.html" on FF2, FF3 (OS X/WinXP), IE6, and IE7.

Changed 11 years ago by Joseph Scheuhammer

Attachment: 8971.patch added

Fixed typo by removing entire "if" statement.

comment:5 Changed 11 years ago by Joseph Scheuhammer

Owner: changed from Joseph Scheuhammer to bill

As per peller's IRC request, reassigning to bill so he can review the patch and decide to either apply it for the 1.3 release, or change the Milestone to some later release.

comment:6 Changed 11 years ago by Joseph Scheuhammer

FWIW, I simulated a situation where the if() succeeds, and the mistyped setWaitState() is called. In that case, no tree is successfully created. Firebug reports "dijit.setWaitState() is not a function".

Hence, the typo would have been obvious had that line executed in the last 4 months (the time period since it was committed in [15630]).

comment:7 Changed 11 years ago by bill

Milestone: 1.31.4
Priority: highlow
Summary: tree.js typo? setWaitState()[patch] [cla] tree.js typo? setWaitState()

OK sounds good, that's why I wanted you to look at it instead of just fixing the typo.

Since it isn't breaking anything we can just check this in for 1.4... but I'm confused, it seems like the expanded state wasn't/won't be set correctly on initialization.

Also, I don't understand why we are only setting the "expanded" state on the root node rather than on every node (or at the least, why we need special case code for the root node in postCreate(), expand(), and collapse()).

comment:8 in reply to:  7 Changed 11 years ago by Joseph Scheuhammer

Replying to bill:

OK sounds good, that's why I wanted you to look at it instead of just fixing the typo.

Yes, it was a worthwhile exercise.

I don't understand why we are only setting the "expanded" state on the root node

We aren't. TreeNode.postCreate() sets aria-expanded on the tree node that is postCreate()'ing. See the call to dijit.setWaiState(this.labelNode, ... just above the if() clause I removed (in the patch). Here, this isa TreeNode.

or at the least, why we need special case code for the root node

The special case is for handling the Tree object itself -- it also has an expanded state: http://www.w3.org/WAI/PF/aria/#tree. As far as I can tell, a tree is expanded/collapsed if and only if its root node is expanded/collapsed. Thus, if aria-expanded is updated on the root node, then similarly update the tree's aria-expanded state.

comment:9 Changed 11 years ago by bill

Resolution: fixed
Status: newclosed

(In [17393]) Remove unneeded a11y code; patch from Joseph (CLA on file). Fixes #8971 !strict.

Note: See TracTickets for help on using tickets.