[patch] [cla] Tree: support dropping between items

[bill]

Currently tree can only support dropping onto the parent item, not dropping between two child items.

Taken from Alex's email.

[Matt Sgarlata]

Does anyone have an idea when this might be fixed? This is a major feature of our application.

[Matt Sgarlata]

I'd like to point out this is working in dojo 0.4. Is there any chance this could be moved up to the 1.2 milestone?

Without this feature we may have to downgrade, or load both the old and new versions of dojo at the same time =/

[cwalters]

Bill, I'm going to take a look at developing this functionality. If there are any considerations or advice that you're able to share, that would be great. :-)

[bill]

Oh that would be great. I think it's the onMouseMove() function in dijit/_tree/dndSource.js (search for "allowBetween"), and you can compare against dojo/dnd/Source.js (same function), since IIRC between-dropping works on a normal DnD source.

When you drop between the ordering info also needs to be communicated back to the data store (on drop). I think currently it puts the element last in the list.

[cwalters]

This patch permits drops above and below nodes as well as the existing drop into functionality within the same tree. (External drop support seems to require changes to the store.newItem, and I didn't want to mess with that)

Single and multi-node reordering within a tree should now work as expected.

[bill]

Hi cwalters, thanks for the patch! Can you file a CLA though (or if your company has a CCLA too that's fine), and let me know, so that I can look at this? Also, if you are signed onto IRC dojo let me know your id there (I'm wild.bill).

[cwalters]

Bill, the company I work for, Spider Strategies, has a CCLA. My IRC id is cwa1ters.

[bill]

Great, OK, thanks for the patch!

[bill]

bumping 1.4 tickets to 1.5, and most 1.3 tickets to 1.4

[scottoreilly]

Was this bumped back accidentally with the mass change or are you planning on applying the patch in 1.4?

[bill]

Hmm, well given that there's a patch might be able to insert it for 1.3. I'll mark it there for now.

[doughays]

I got a ping from a customer requesting a bump in priority for this ticket.

[bill]

Hi cwalters,

This is a great patch! Thanks for working on this. Interesting approach to implement between checking by seeing if you are near the edge of a node. (Or perhaps that's how the standard DnD code works.)

Anyway, I'm seeing a few issues I'm hoping you can correct though:

• on test_Tree_DnD.html, bottom left tree, if you drag "Cereals" to the bottom of the "Foods" TreeNode (such that Foods is highlighted, and the horizontal line appears), and then drop "Cereals", it disappears altogether.

• similarly, if you drop "Citrus" at the bottom of "Fruits", which should be a no-op, "Citrus" becomes a sibling of "Fruits" rather than remaining a child

• highlighting: when the user is dropping between two nodes, Tree shows a horizontal line between the nodes (which is good) but also it keeps highlighting one of the nodes (whichever node the mouse is actually over), which is confusing.

• IE6: This code works by drawing a horizontal line between nodes whenever you are near the top of a node, or the bottom of a node. However, on IE6 and maybe other versions of IE too, it only seems to work when you are near the bottom of a node.

[bill]

(In [16087]) Initial code to support dropping between tree items. Thanks to cwalters (CLA on file) for this great patch! Checking in now, but there are still some issues (see comments in #6445) so leaving ticket open. After fixing those issues should turn feature on by default. Refs #6445 !strict

[cwalters]

Hi Bill,

I'm glad the approach taken works well for everyone! The second patch contains updates for the to address the issues noted:

• There is now a checkItemAcceptance test for the bottom left tree. I felt it needed to be handled at this level since the ForestStoreModel can have multiple top level items, so in that instance it should be allowed.

• Moving "Citrus" to be after "Fruits" rather than a child is perfectly acceptable, just as "Citrus" can be dropped over "Foods". Either action would take "Citrus" out of the "Fruits" collection and put it in "Foods", making them siblings. The drop between has the advantage of specifying the order rather than adding it to the bottom.

• I agree the highlighting is confusing, but it is handled by the dojoDndItemOver setting in the /dojo/tests/dnd/dndDefault.css and is not standard. If you try dragging an Item within the upper left "Items" to a new position in the list, you will see the same behavior. The standard dojoDndItemOver just changes the cursor.

• This was due to the IE box model bug. The test_Tree_DnD.html update patch has IE specific styles for the dijitTreeLabel and dijitTreeExpando to fix this. I figured it would be best to let others decide if this should be rolled into the Common.css for themes.

[bill]

Hi,

Thanks for the new patch. Hopefully I can find you on IRC, I want to discuss a few things about it. I'm writing them down here, before I forget:

similarly, if you drop "Citrus" at the bottom of "Fruits", which should be a no-op, "Citrus" becomes a sibling of "Fruits" rather than remaining a child

You and I had different ideas about how the feature should work, but seems like you and Mike are both thinking along the same lines, see http://thread.gmane.org/gmane.comp.web.dojo.devel/9627/focus=9636

There is now a checkItemAcceptance test for the bottom left tree.

Actually from Tree's perspective, regardless of the model (ForestModelStore or otherwise), there's a single root node. Sometimes it's hidden (like in the second example in test_Tree.html) but it always exists. So if the user is trying to drop after/before the root node, that should be disallowed. I don't think we should require the user to write a custom method for this case.

That reminds me, need to confirm that checkItemAcceptance() is being called with the right arguments, both onMouseMove and on drop. If the user drags a node into the "after" position for node X, icheckItemAcceptance() should be called with X's parent, not X. Is it?

This was due to the IE box model bug.

We shouldn't be hardcoding heights into the CSS files because that messes things up for users that increase/decrease font size. I'm sure we can find a different way. As per "the IE box model bug" I assume you are talking about IE using border-box sizing rather than content-box sizing, but dojo has code to handle that browser difference. Maybe we are calling dojo.coords() when we should be calling dojo.marginBox().

[bill]

(In [16098]) DnD related documentation, plus removal of DnD horizontal code path, since trees are always vertical (i.e, each TreeNode? is a separate row, not a separate column). Refs #6445 !strict

[bill]

(In [16099]) Fix IE issues w/DnD. The onmousemove event should be making calculating based on the node that we connect to for onmouseenter/onmouseleave events, which is the dijitTreeRow node, not the dijitTreeContentNode.

This change has the unfortunate side effect of removing the indentation from the insert line (the horizontal line that shows that we are inserting before/after a given element). Need to modify CSS etc. to get that behavior back.

Refs #6445 !strict

[bill]

(In [16100]) Bunch of TODO comments, plus adding third parameter to checkItemAcceptance(), to indicate whether we are dropping before, after, or (the default) into the target node. Refs #6445 !strict

[cwalters]

Hi Bill,

Thanks for going over things with me in IRC! The latest patch prevents any drop before or after a root node. It also updates the test_Tree_DnD.html to force the collection tree to only allow itself as the source. I'm a bit unhappy with the IE fix for the before/after, mostly because it breaks the indentation of the line, but also because it introduces a "wiggle" of the node as the between line is drawn. Hopefully all that can be handled via CSS changes.

[bill]

(In [16103]) Prevents any drop before or after the root node. Also updated test_Tree_DnD.html to force the collection tree to only allow itself as the source.

Patch from cwalters (thanks!), refs #6445 !strict

[bill]

Thanks for the patch.

Agreed about the look and feel. Apparently, w/the old code, the reason the border (ie, insert line) didn't make the tree "wiggle" was because the border was added to a <span>, which doesn't have layout.

It's the dojoDndItemAfter rule in tundra/Common.css which adds the border.

[bill]

(In [16108]) Beef up comments, standardizing format and (more importantly) adding documentation about parameter types.

Also removed markupFactory since dndSource/dndTarget are never created in markup (rather, they are created from Tree).

Would like to remove dndTarget altogether as it doesn't seem useful, but will leave in for backwards compatibility.

Refs #6445 !strict

[bill]

(In [16109]) Align insert indicator (for DnD before/after dropping) with content, to make it clearer where the element will be dropped. Refs #6445 !strict

[bill]

Also, big cleanup in [16117] and [16120]. I'm marking this as fixed; it's working pretty well now.