Opened 11 years ago

Closed 10 years ago

#6445 closed task (fixed)

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

Reported by: bill Owned by: bill
Priority: high Milestone: 1.3
Component: Dijit Version: 1.1.0
Keywords: alex Cc: Chris Walters
Blocked By: Blocking:

Description

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

Taken from Alex's email.

Attachments (3)

dndBetween.patch (9.7 KB) - added by Chris Walters 11 years ago.
dndBetweenUpdate.patch (1.8 KB) - added by Chris Walters 10 years ago.
dndBetweenNoRoot.patch (2.6 KB) - added by Chris Walters 10 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 11 years ago by Matt Sgarlata

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

comment:2 Changed 11 years ago by 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 =/

comment:3 Changed 11 years ago by Chris Walters

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. :-)

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

Changed 11 years ago by Chris Walters

Attachment: dndBetween.patch added

comment:5 Changed 11 years ago by Chris Walters

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.

comment:6 Changed 11 years ago by bill

Milestone: 1.41.3

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

comment:7 Changed 11 years ago by Chris Walters

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

comment:8 Changed 11 years ago by bill

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

Great, OK, thanks for the patch!

comment:9 Changed 11 years ago by bill

Milestone: 1.31.4

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

comment:10 Changed 11 years ago by scottoreilly

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

comment:11 Changed 11 years ago by bill

Milestone: 1.41.3

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

comment:12 Changed 10 years ago by Douglas Hays

Priority: normalhigh

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

comment:13 Changed 10 years ago by bill

Cc: Chris Walters added

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.

comment:14 Changed 10 years ago by 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

Changed 10 years ago by Chris Walters

Attachment: dndBetweenUpdate.patch added

comment:15 Changed 10 years ago by Chris Walters

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.

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

comment:17 Changed 10 years ago by 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

comment:18 Changed 10 years ago by 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

comment:19 Changed 10 years ago by 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

Changed 10 years ago by Chris Walters

Attachment: dndBetweenNoRoot.patch added

comment:20 Changed 10 years ago by Chris Walters

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.

comment:21 Changed 10 years ago by 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

comment:22 Changed 10 years ago by 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.

comment:23 Changed 10 years ago by 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

comment:24 Changed 10 years ago by 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

comment:25 Changed 10 years ago by bill

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.