[Patch][CLA] Keyboard multi-select for dijit.tree

[Jason Priestley]

Attached is a diff adding keyboard support for multi-select to dijit.tree. This involves a slight refactoring, in that selection functionality is now fully delegated to the dndController. Patch tested on IE6,7,FF,Safari,Chrome.

This was written on behalf of TeamPatent.

[bill]

Thanks for the work on this! It's definitely a good idea to get rid of the duplicated selection code.

Seems like you developed against an old version of the Tree code? I removed _getSelectedNodeAttr(), _getSelectedItemAttr, _getPathAttr() a few weeks ago but there they are again in your patch. In other words, it seems like this patch will break watch() notifications for path and selectedItem.

Also, since you are promoting multiple selection as a first-class feature of Tree, seems like there should be some new (settable, gettable, and watchable) Tree attributes called "paths" and "selectedItems", to set/get/watch multiple selected nodes programatically. "Path" and "selectedItem" need to be supported for back-compat but could leverage the new code.

One other note: currently _setSelectedItemAttr() is supposed to set the selection to a single item, clearing all other selected nodes. It doesn't look like it will do that, especially when multiple nodes are selected. You need tests for this. Likewise with _setPathAttr().

[bill]

PS: perhaps _setSelectedItemAttr() is working. I got confused because the summary of the function toggleSelection():

Add or remove the given node from selection

That sounds like toggleSelection() will only add/remove the specified node from the selection, but in actuality that function does other things.

[Jason Priestley]

Thanks for the feedback. I'm almost finished with a new patch to add paths and selectedItems attributes, and back out the getters. I'll also clarify the role and behavior of toggleSelection() in comments.

[bill]

Great, looking forward to it. About toggleSelection(), maybe there's a better name like adjustSelection() or handleClick() or onClick() but anyway I'm not good at names.

[Jason Priestley]

Attached an updated patch. Notable changes:

toggleSelection is now called userSelect - this indicates that it is meant to respond to a user action, such as a click or keypress.

Tree attributes (selectedNode, selectedPath, etc.) are now set after every change to the selection.

New attributes are available to set the selection to multiple items, nodes, or paths. The setPathsAttr code includes a refactoring of the path selection code from setPathAttr.

[bill]

This all looks great, except for a few spacing issues (to match our coding styles), but I updated those locally.

I just want to make sure about the CLA, according to ​http://docs.dojocampus.org/developer/contributors neither you nor Teampatent has one. Did you (or the company) send one in?

[liucougar]

Replying to bill:

I just want to make sure about the CLA, according to ​http://docs.dojocampus.org/developer/contributors neither you nor Teampatent has one. Did you (or the company) send one in?

just sent you an email about TeamPatent? CCLA

[bill]

(In [23347]) Adding keyboard support for multi-select to dijit.tree. This involves a slight refactoring, in that selection functionality is now fully delegated to the dndController. Previously selection code was duplicated between dijit.Tree and dijit.tree._dndSelector.

Introduces watchable "paths" and "selectedNodes" attributes representing [the path to] each selected node in the tree

Thanks to jhpriestley (TeamPatent, CCLA) for the patch!

Fixes #12042 !strict.

[bill]

(In [23465]) For testing keyboard selection on mac use ENTER, meta-SPACE controls the IME (switching input language), but on IE6 use SPACE, because ctrl-shift-ENTER doesn't do anything (although not sure why we are testing ctrl-shift combination anyway).

Refs #12042.