Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#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 bill)

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)

multiselect.diff (33.5 KB) - added by Jason Priestley 9 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 years ago by 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().

comment:2 Changed 9 years ago by 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.

comment:3 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by 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.

Changed 9 years ago by Jason Priestley

Attachment: multiselect.diff added

comment:5 Changed 9 years ago by 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.

comment:6 Changed 9 years ago by bill

Description: modified (diff)
Milestone: tbd1.6
Owner: set to bill
Status: newassigned

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 in reply to:  6 Changed 9 years ago by 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

comment:8 Changed 9 years ago by bill

Resolution: fixed
Status: assignedclosed

(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.

comment:9 Changed 8 years ago by 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.

Note: See TracTickets for help on using tickets.