#12042 closed enhancement (fixed)
[Patch][CLA] Keyboard multi-select for dijit.tree
Reported by: | Jason Priestley | Owned by: | bill |
---|---|---|---|
Priority: | high | Milestone: | 1.6 |
Component: | Dijit | Version: | 1.5 |
Keywords: | dijit tree a11y multiselect | Cc: | |
Blocked By: | Blocking: |
Description (last modified by )
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.
Attachments (1)
Change History (10)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
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.
comment:3 Changed 10 years ago by
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.
comment:4 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | multiselect.diff added |
---|
comment:5 Changed 10 years ago by
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.
comment:6 follow-up: 7 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Milestone: | tbd → 1.6 |
Owner: | set to bill |
Status: | new → assigned |
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?
comment:7 Changed 10 years ago by
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
comment:8 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(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.
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().