Closed Bug 229224 Opened 16 years ago Closed 2 years ago

Implement Drag & Drop method for reordering attachments during composition

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: mozilla-bugs, Assigned: bugzilla2007)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 14 obsolete files)

20.41 KB, patch
Paenglab
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
Currently there does not seem to be any way of reordering attachments in the
Composition window (short of deleting and re-attaching). It would be nice if it
was possible to reorder attachments by dragging them across the attachement
window (and dropping into the right spot of the attachements list).

Another alternative, I guess, would be to add "Move Up/Move Down" to the context
menu for the attachments - not as natural/easy to use, but could be easier to
implement.
Summary: RFE: ability to reorder attachments (by D&D, or context menu, or both). → Ability to reorder attachments (by D&D, or context menu, or both).
related : bug 2920 (some 'edit attachment' bugs were duped to that, despite the
title)
Product: MailNews → Core
Blocks: 295747
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: esther → composition
Product: Core → MailNews Core
Duplicate of this bug: 295747
OS: Linux → All
Hardware: x86 → All
Summary: Ability to reorder attachments (by D&D, or context menu, or both). → Ability to reorder attachments (by Drag&Drop, or context menu, or both)
Summary: Ability to reorder attachments (by Drag&Drop, or context menu, or both) → Ability to reorder attachments during composition (by Drag&Drop, or context menu, or both)
Duplicate of this bug: 618011
Blocks: attachUXtracker
No longer blocks: 295747
The behavior with this patch is that dragging and dropping from other windows always puts the attachment at the end, but dragging and dropping *within* the attachment pane reorders them. Reordering picks the nearest insertion point (usually between two list items) and shows a little arrow where the object will get dropped. I suppose I could make the first case drop into the nearest insertion point too, but I'm not sure if people actually want that to happen.

:clarkbw, what do you think about the behavior I described?
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #501175 - Flags: ui-review?(clarkbw)
fwiw, the behaviour in comment 6 is exactly what I would expect when dragging attachments inside or into the attachment box.

Jim, this is great!!!!! Thank you so much for writing this patch, which will kill another major and longstanding nuisance in composer's attachment pane.

Since your patch handles composer's attachment list items, I am wondering if it would be possible for you to add the following tiny improvement in the very same area (probably about 5 lines of code for a noticeable usability improvement):
Bug 526998 - Implement F2 keyboard shortcut for renaming focused attachments when composing (on Windows)?
Ugh, this was entirely too painful, but I finally managed to get drag-and-drop of multiple selected attachments to get reordered sanely. Basically, when you select multiple attachments and drag-and-drop them, they all get moved to that insertion point and are placed there in the same relative order they were originally.

Pulling ui-review request forward, since this obsoletes the other patch.
Attachment #501175 - Attachment is obsolete: true
Attachment #501463 - Flags: ui-review?(clarkbw)
Attachment #501175 - Flags: ui-review?(clarkbw)
(In reply to comment #7)
> Since your patch handles composer's attachment list items, I am wondering if it
> would be possible for you to add the following tiny improvement in the very
> same area (probably about 5 lines of code for a noticeable usability
> improvement):

I'm not going to do it in this bug (there's probably half a dozen simple things I could do in this part of the code, and they add up fast), but I can look at it.

While I'm thinking of it, there's still one problem in attachment 501463 [details] [diff] [review]: if you drag to the edge of the listbox, it doesn't automatically scroll. I'm not sure how to do that at the moment, but I will look into it. Anyone with brilliant ideas in this department is of course free to point me in the right direction. :)
Blocks: 623346
(In reply to comment #7)
> fwiw, the behaviour in comment 6 is exactly what I would expect when dragging
> attachments inside or into the attachment box.

Another way to do this is to add attachments to the end when you drop onto the address pane, and add attachments at the nearest insertion point (i.e. the middle) when actually dragging over the attachment pane.
(In reply to comment #10)
> Another way to do this is to add attachments to the end when you drop onto the
> address pane, and add attachments at the nearest insertion point (i.e. the
> middle) when actually dragging over the attachment pane.

Theoretically yes, but practically such behaviour would be undiscoverable and unexpected and therefore not very useful.
Jim, your comment 6 looks like the best behaviour (position dragging inside attachment panel only; for dropping files from outside attachment panel always add them to the end of the list):

- we can expect that most people just want to add one file after the other when they drop files on attachment pane
- we can't expect everyone do do precise dragging for each attachment they want do drop from outside
- file managers like windows 7 explorer will drop files into the correct position of the current order of the list (i.e. not actual the drop position). when you have files a, b, d, and you drop c between a and b, it will end up in correct order between b and d.
otherwise (e.g. when pasting with ctrl+v), files seem to end up at the end of the list most of the time.
(In reply to comment #11)
> - file managers like windows 7 explorer will drop files into the correct
> position of the current order of the list (i.e. not actual the drop position).
> when you have files a, b, d, and you drop c between a and b, it will end up in
> correct order between b and d.

At least in XP, there's only one explorer view mode that lets you actually pick the order of objects like in this patch: thumbnail view. I know in that view when you drag-and-drop, it puts the dropped file in the drop position, not at the end or in sorted order.

We'll see what clarkbw thinks, though, since he's probably got some ideas too.

On the subject of scrolling the listbox when you drag to the edge of it (like windows explorer, etc), I think it might be best if I did that in a followup bug, since I think we'd want the listbox to scroll smoothly while doing that, and that's impossible unless I go in and edit core toolkit code, which will be complicated and probably delay this patch from getting committed for quite a while.
CC'ing :clarkbw just in case.
Comment on attachment 501463 [details] [diff] [review]
Allow drag-and-drop of multiple items

looks good, thanks!
Attachment #501463 - Flags: ui-review?(clarkbw) → ui-review+
This just changes a single line to use screenY instead of pageY, since that's what I used everywhere else. Pulling ui-review+ forward.

:dmose, could you take a look at this? I fixed the indentation in the patch, so there are a bunch of whitespace changes, but other than those, most of the changes are near the beginning and end of the diff.
Attachment #501463 - Attachment is obsolete: true
Attachment #502170 - Flags: ui-review+
Attachment #502170 - Flags: review?(dmose)
Comment on attachment 502170 [details] [diff] [review]
Be more consistent with pageY vs. screenY

I don't expect to have bandwidth to look at this soon, but my hope is that bwinton might, so I'm going to start by redirecting to bwinton.  Blake, if you don't, let me know, and we'll see what we can figure out...
Attachment #502170 - Flags: review?(dmose) → review?(bwinton)
Comment on attachment 502170 [details] [diff] [review]
Be more consistent with pageY vs. screenY

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -3458,136 +3458,243 @@ function AttachmentBucketClicked(event)
>+  _adjustDropTarget: function (aEvent)
>+  {
>+    if (aEvent.originalTarget.tagName == 'listbox')
>+    {

The {s for if-statements (and functions) should be on the same line.
(But the {s for functions defined at the top level should be on a new line, oddly enough. :)

>+      // Abort, abort! We hit the border of the listbox!
>+      return null;
>+    }

I can’t seem to drag files out of the attachment box to remove them from the list of attachments.  But I guess I can’t do that previously either, so, uh, maybe that’s a feature request…  :)

Oh, also, if I drag the files quickly out of the attachment box into the compose window, the dropMarker never gets hidden, so you might want to do that in here.

>+  }, 

There’s a trailing space here that should be removed.

>+  _showDropMarker: function(target, offset)
>+  {
>+    var marker = document.getElementById('attachmentDropMarker');
>+    var stack = document.getElementById('attachmentStack');

I think we prefer "s to 's for strings.  At least, that’s what the other strings in the file use.

>+    marker.left = target.boxObject.screenX - stack.boxObject.screenX +
>+      ((offset && offset.x) || 0);
>+    marker.top = target.boxObject.screenY - stack.boxObject.screenY -
>+      marker.boxObject.height/2 + ((offset && offset.y) || 0);

You could also line these up with the "target.boxObject", instead of indenting them two spaces, but I think this way is also fine.

>   onDrop: function (aEvent, aData, aDragSession)
>+  {
>+    var dataList = aData.dataList;
>+    var dataListLength = dataList.length;
>+    var errorTitle;
>+    var attachment;
>+    var errorMsg;
>+
>+    var bucket = document.getElementById('attachmentBucket');

I think you should put this variable declaration up with the others.

>+    if (aDragSession.sourceNode.parentNode == bucket)
>     {
>+      // We dragged from the attachment pane onto itself, so instead of
>+      // attaching a new object, we're just reordering them.
>+
>+      var target = this._adjustDropTarget(aEvent);
>+
>+      // NodeLists are "live" (and unsorted in this case), but we want a "dead",
>+      // sorted list, so massage things a bit.
>+      var sources = Array.prototype.slice.call(bucket.selectedItems);
>+      sources.sort(function(a, b) bucket.getIndexOfItem(a) - 

And another trailing space.
I would also prefer it if you put ()s around the "bucket.getIndexOfItem(a) - bucket.getIndexOfItem(b)" expression, for clarity.

>+      if (target == bucket)
>       {
>+        // We dragged to the end of the list
>+        for (let i = 0; i < sources.length; i++)
>+          bucket.appendChild(sources[i]);
>+      }
>+      else if (target.parentNode == bucket)
>+      {
>+        // We dragged into the middle (or beginning)
>+        for (let i = 0; i < sources.length; i++)
>+          bucket.insertBefore(sources[i], target);
>+      }

So, I’ve noticed that if I select multiple items, and drag them so that the drop marker is at the top of the selection, they re-order themselves, and I can’t quite tell why, but I suspect it’s because of this second case…  Perhaps it would be worth a check to see if the target is one of the things you’re dragging.

>   onDragOver: function (aEvent, aFlavour, aDragSession)
>+  {
>+    var bucket = document.getElementById('attachmentBucket');
>+    if (aDragSession.sourceNode.parentNode == bucket)

This line gives me an error of "aDragSession.sourceNode is null" when I try to drag a file from the desktop to the compose window.

>     {
>+      // If we're dragging from the attachment bucket onto itself, we need to
>+      // draw a drop marker.
>+
>+      var target = this._adjustDropTarget(aEvent);
>+
>+      if (target && target.parentNode == bucket)
>       {
>+        this._showDropMarker(target);
>+      }
>+      else if (target == bucket)
>+      {
>+        var last = bucket.getItemAtIndex(bucket.itemCount-1);
>+        this._showDropMarker(last, {y: last.boxObject.height});
>       }

I wonder if that logic should be wrapped up into _showDropMarker, so that it will show the marker after the last item if you, say, call it with an argument of null or undefined…

>+    else if (aFlavour.contentType != "text/x-moz-address")
>+    {
>+      // make sure the attachment box is visible during drag over
>+      var attachmentBox = document.getElementById("attachments-box");
>+      if (attachmentBox.collapsed)
>+        ChangeAttachmentBucketVisibility(false);
>+    }
>+    else
>+    {
>+      DragAddressOverTargetControl(aEvent);
>+    }

And I can’t test these two cases, because of the null error above.  ;)

I’m also seeing a lot of "NS_ENSURE_TRUE(typeSupported) failed:" errors when I try to drag attachments out of the attachment box which I think we should try to prevent somehow.

Finally, since I ran across two bugs, I really think we need some tests before I can accept this patch.

Thanks,
Blake.
Attachment #502170 - Flags: review?(bwinton) → review-
(In reply to comment #17)
> Finally, since I ran across two bugs, I really think we need some tests before
> I can accept this patch.

I'll take a look at the bugs you found later (I'm not at my dev box), but I'm not sure we *can* test this yet, since it requires bug 515776, and that's only fixed in Mozmill 1.5.2+. As I recall, we're still stuck on 1.4.2-something and upgrading causes failures elsewhere.
Wellll, I suppose I could be evil and just copy the code from bug 515776 and put it somewhere and then file a bug for us to remove that code when we upgrade Mozmill.
Ok, it's becoming clear to me that I'm doing these attachment UX patches in the wrong order. I'm going to put this one on the backburner, since the code will probably have to change when I fix the XBL bindings for attachment panes anyway. Here's the plan:

1) Add "all attachments" info to compose/reading attachment panes (bug 282068, 
   bug 623346)
2) Fix XBL bindings for attachment panes and use them for compose/reading 
   (currently only reading uses the XBL binding; no bug yet)
3) Reorder attachments when composing (this bug)
Blocks: 636617
Depends on: 630759
Blocks: 663695
(In reply to Jim Porter (:squib) from comment #20)
> panes anyway. Here's the plan:
> 
> 1) Add "all attachments" info to compose/reading attachment panes (bug
> 282068, 
>    bug 623346)
> 2) Fix XBL bindings for attachment panes and use them for compose/reading 
>    (currently only reading uses the XBL binding; no bug yet)
> 3) Reorder attachments when composing (this bug)

Now that 1) and 2) are fixed (thank you!), what are the chances for 3)?
(In reply to Jim Porter (:squib) from comment #20)
Jim, according to your plan, and appreciating the advanced age of this very useful idea, it would be great to see this land before the next release cycle on Dec 20th. Please...!
(In reply to Thomas D. from comment #22)
> (In reply to Jim Porter (:squib) from comment #20)
> Jim, according to your plan, and appreciating the advanced age of this very
> useful idea, it would be great to see this land before the next release
> cycle on Dec 20th. Please...!

This definitely isn't happening for 11.0, since I'm already swamped with stuff. It might make it into 12.0, but I can't make any promises.
Whiteboard: [has wip patch]
Whiteboard: [has wip patch] → [has wip patch][patchlove]
Attached file orderatt-1.4-tb.xpi (obsolete) —
Hi Jim,

I still have high hopes in you that you might be able to finish off your advanced (albeit bitrotted) WIP patch on this highly desirable feature (potentially useful every day for all sorts of users, private, business, beginner, advanced - it's just so easy to get attachment order wrong, or wanting to replace/add just one in the list...).

Blake's review from Comment 17 is mostly about nits, real issues were few afasics.

Perhaps the attached addon code which works on TB24 might have some easier starting points in current code / inspiration for you?

Things that don't work with the addon (but worked with Jim's WIP patch):
- dragging multiple attachment items
- indication of attachment drop location is nicer with Jim's patch afasics from the code

Things that are nice about the addon:
- comes with a nice full-flegded context menu, too.
- dragging attachment into text input boxes drops the file name as text; useful e.g. for copying the file name into the editor when talking about specific attachment

Things that would be nice to have (for both):
- dragging attachment out of attachment pane (and dropping anywhere outside text inputs) should remove that attachment (RFE as desired by Blake)

Tia for anything you might offer here.
Flags: needinfo?(squibblyflabbetydoo)
I might work on this one day, but it's definitely not going to happen for TB31. I have other things I'd rather try to land for that.
Flags: needinfo?(squibblyflabbetydoo)
Assignee: squibblyflabbetydoo → nobody
Status: ASSIGNED → NEW
I'm implementing full-fledged "Reorder Attachments" functionality with context-menu and keyboard access in bug 663695, so we're left with adding drag and drop method.
Summary: Ability to reorder attachments during composition (by Drag&Drop, or context menu, or both) → Implement Drag & Drop method for reordering attachments during composition
I've updated Jim's patch (with some purely cosmetic changes) to apply on top of my patch attachment 8920633 [details] [diff] [review] in bug 663695. It's still basically working, with all the little issues mentioned by Blake in comment 17. I also see some flickering when dragging attachments downwards, which is because of the same focus/selection styles problem discussed in my bug 663695, comment 89, namely the 1px horizontal space between attachment list items, which here causes the drop marker to jump down to the end of the list temporarily, which then causes the flickering.

I'm reasonably confident that this could be revived with reasonable effort, but not taking yet.

I imagine that instead of the little arrow dropmarker at left side of list, we could have a simple line drop marker between items. Maybe we could realise this in a better way:
- set a property like dropmarker="top|bottom" on the item where drop marker should appear
- use css based on property/value selector to dymanically show top-border|bottom-border as a drop marker.
Richard, per bug 663695, comment 89, and my comment 27 here, could you please normalise/get rid of the small horizontal spacing between attachment list items, so that we avoid artificial problems caused by that? Of course we could also code around that here, but I don't think that's useful, that space must go anyway as it can only cause trouble in many ways.
Flags: needinfo?(richard.marti)
Attached patch attachmentitem-style.patch (obsolete) — Splinter Review
This fixes the issue with the margin between the attachmentitems. Now, when two selected items touch each other, the border look more strong.

Do you want use this patch here or in bug 663695? I could also file a new one and try to fix the border issue.
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #29)
> Created attachment 8920887 [details] [diff] [review]
> attachmentitem-style.patch
> 
> This fixes the issue with the margin between the attachmentitems. Now, when
> two selected items touch each other, the border look more strong.
> 
> Do you want use this patch here or in bug 663695? I could also file a new
> one and try to fix the border issue.

I think separate bug to fix spacing and border issue is best. If at all, I'll definitely do drag n drop here, not in bug 663695, but it'll take time especially to allow removing attachments via d n d.
Filed bug 1410745 for the spacing.
Here we go! :)

This WIP patch (on top of my other patch for reordering attachments) has been completely rewritten and now features fully fine-tuned drag & drop functionality, also eliminating all problems mentioned in Blake's initial review of comment 17. I replaced Jim's stack-dropmarker-image approach with a css-based approach.

Dropping non-coherent multiple selections of attachment list items into their right places without changing their order turned out to be pretty nasty, especially when dropping disjoint selections at their own outer/inner edges, but I benefited from my experience gained in bug 663695.

Richard, apart from testing the behaviour, could you please help me out with the css? I've tried in vain to show the dropmarker on selected and/or focused items. I included my windows working copy of attachmentList.css to show what I'm trying to do.
Btw this is also the right corner to fine-tune the focus/selection behaviour to be ux-consistent with Windows 10 Explorer, and apart from just eliminating some side borders, we'll probably want to use css + selector to get the selection top/bottom edges right (border-free).

It would also be cool if we could find a way that avoids the slight shifting of items around the dropmarker; if there's no other way, we might have to split the two-px-border-top-dropmarker into 1px border-bottom of previousSibling and 1px border-top. But I think I saw 2px top-borders already, so I'm not sure why this is moving at all.
Assignee: nobody → bugzilla2007
Attachment #8920862 - Attachment is obsolete: true
Attachment #8920887 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8924354 - Flags: review?(acelists)
Attachment #8924354 - Flags: feedback?(richard.marti)
Attachment #8924354 - Attachment description: Patch V.2.2 (WIP, on top of attachment 8920633): Implement drag & drop functionality for reordering attachments → Patch V.3 (WIP, on top of attachment 8920633): Implement drag & drop functionality for reordering attachments
Attachment #8924354 - Attachment description: Patch V.3 (WIP, on top of attachment 8920633): Implement drag & drop functionality for reordering attachments → Patch V.3 (WIP, on top of attachment 8923082): Implement drag & drop functionality for reordering attachments
Comment on attachment 8924354 [details] [diff] [review]
Patch V.3 (WIP, on top of attachment 8923082 [details] [diff] [review]): Implement drag & drop functionality for reordering attachments

Works quiet good.

(In reply to Thomas D. from comment #32)
> Created attachment 8924354 [details] [diff] [review]
> Patch V.3 (WIP, on top of attachment 8923082 [details] [diff] [review]):
> Implement drag & drop functionality for reordering attachments
> 
> Dropping non-coherent multiple selections of attachment list items into
> their right places without changing their order turned out to be pretty
> nasty, especially when dropping disjoint selections at their own outer/inner
> edges, but I benefited from my experience gained in bug 663695.

When multiple items are selected and and I begin to drag, only the item directly under the mouse is dragged. When multiple items are selected all of them should be dragged as one block. Explorer, and also all file manager, drags all together.

> Richard, apart from testing the behaviour, could you please help me out with
> the css? I've tried in vain to show the dropmarker on selected and/or
> focused items. I included my windows working copy of attachmentList.css to
> show what I'm trying to do.

As I wrote above, dragging above the selected items is a no go as you can't drag something above itself.

> Btw this is also the right corner to fine-tune the focus/selection behaviour
> to be ux-consistent with Windows 10 Explorer, and apart from just
> eliminating some side borders, we'll probably want to use css + selector to
> get the selection top/bottom edges right (border-free).

No, it isn't. We have bug 1412758 for this.

> It would also be cool if we could find a way that avoids the slight shifting
> of items around the dropmarker; if there's no other way, we might have to
> split the two-px-border-top-dropmarker into 1px border-bottom of
> previousSibling and 1px border-top. But I think I saw 2px top-borders
> already, so I'm not sure why this is moving at all.

Please don't use "data-dropmarker" but "dropOn" like it's used everythere in c-c and m-c. And for the Windows CSS you can use this:

  #attachmentBucket attachmentitem[dropOn="top"] {
    border-top-color: Highlight;
  }

  #attachmentBucket attachmentitem[dropOn="bottom"] {
    border-bottom-color: Highlight;
  }

For the other platforms you can use the colors from here:

https://dxr.mozilla.org/comm-central/search?q=treechildren%3A%3A-moz-tree-cell-text(primary%2C+dropOn)+-path%3Aobj-x86&redirect=false
and
https://dxr.mozilla.org/comm-central/search?q=treechildren%3A%3A-moz-tree-cell(primary%2C+dropOn)+-path%3Aobj-x86&redirect=false

So for Linux it's 'Highlight' too. and for OSX '#a1a1a1'.
Attachment #8924354 - Flags: feedback?(richard.marti) → feedback+
(In reply to Richard Marti (:Paenglab) from comment #33)
> Comment on attachment 8924354 [details] [diff] [review]
> Patch V.3 (WIP, on top of attachment 8923082 [details] [diff] [review]):
> Implement drag & drop functionality for reordering attachments
> 
> Works quiet good.

*quite* (de: "ziemlich") good is not good enough, but let's see...
*quiet* (de: "ruhig") is ok...

> (In reply to Thomas D. from comment #32)
> > Created attachment 8924354 [details] [diff] [review]
> > Patch V.3 (WIP, on top of attachment 8923082 [details] [diff] [review]):
> > Implement drag & drop functionality for reordering attachments
> > 
> > Dropping non-coherent multiple selections of attachment list items into
> > their right places without changing their order turned out to be pretty
> > nasty, especially when dropping disjoint selections at their own outer/inner
> > edges, but I benefited from my experience gained in bug 663695.
> 
> When multiple items are selected and and I begin to drag, only the item
> directly under the mouse is dragged. When multiple items are selected all of
> them should be dragged as one block. Explorer, and also all file manager,
> drags all together.

Yes, Explorer shows a bundle of files (even for disjoint selections), and the number of files dragged.
But I'm afraid that there's nothing I can do about that, it's not me who defines the drag behaviour and images shown while dragging. That might even be an XUL bug, otherwise our own existing implementation of dragging. Technically, even though drag image shows only one item, all selected items are dragged and dropped correctly (on my Windows install), unless there's something odd with your OS again. On windows, dragging multiple items (disjoint or not) works exactly as it should (except for the misleading drag image while dragging). Can you please confirm that multiple items are moving correctly, or maybe they don't for you?

> > Richard, apart from testing the behaviour, could you please help me out with
> > the css? I've tried in vain to show the dropmarker on selected and/or
> > focused items. I included my windows working copy of attachmentList.css to
> > show what I'm trying to do.
> 
> As I wrote above, dragging above the selected items is a no go as you can't
> drag something above itself.

Hmmm, there's a misunderstanding here.
You can't drag a *single* selected *block* onto itself because there would be no changes in item positioning at all, so *that* does not make sense, and my code has been designed to prevent that.
However, dragging *multiple*, *disjoint* selected blocks is another story.
It's totally ok, intuitive and consistent to do this (# indicates selected items, ----- indicating drop marker):

A.txt
------
B.txt#
C.txt
D.txt#
E.txt
F.txt#
G.txt

Then after dropping at dropmarker directly above a selected block (e.g. above B.txt), you'll get this:


A.txt
B.txt#
D.txt#
F.txt#
C.txt
E.txt
G.txt

And it doesn't matter above or below which of the smaller selected blocks you drop, there'll always be predictable movement.
We need to understand the meaning of dragging multiple disjoint selected (blocks of) items into a new position:
- first, group all disjoint selected (blocks of) items together
- then, place the entire single selected block at dropmarker position.
That's exactly what happens.

There's no need to limit user's choice as to where he wants to drop the re-grouped/collapsed block.
That may imply that some items don't move, but the net result is predictable and useful as long as there are other items which move and are recombined into a single block in the new position.
The only thing which does not make sense is to drop multiple selected items as a block *between two selected items*, because the result would be unpredictable and self-contradictory.

We already offer this function from the menu, in a more fine-grained way:
- "Move together" (up or down) works exactly the same way, the top (or bottom) selected item(s) won't move, but we'll just collapse any remote (non-adjacent) items into a single block. Which makes perfect sense for combining items which belong together as a group. It actually reduces your cognitive burden because you can just select any items which should be grouped together and then they'll be grouped, without cracking your head that maybe the first two are already in the right position, so you should only drag the remote items (but if that's your preferred way, you're also free to do that). So the behaviour which I implemented for drag and drop is consistent with that. Almost anything goes, in a predictable and intuitive manner.

> > Btw this is also the right corner to fine-tune the focus/selection behaviour
> > to be ux-consistent with Windows 10 Explorer, and apart from just
> > eliminating some side borders, we'll probably want to use css + selector to
> > get the selection top/bottom edges right (border-free).
> 
> No, it isn't. We have bug 1412758 for this.

Yes. I was just trying to point out the corner in code, not suggesting that we should fix it here.
Unfortunately adjacent (next) sibling selector ( + ) only helps us half-way, as you said, because there's no previous-sibling selector in CSS (yet).

> > It would also be cool if we could find a way that avoids the slight shifting
> > of items around the dropmarker; if there's no other way, we might have to
> > split the two-px-border-top-dropmarker into 1px border-bottom of
> > previousSibling and 1px border-top. But I think I saw 2px top-borders
> > already, so I'm not sure why this is moving at all.
> 
> Please don't use "data-dropmarker" but "dropOn" like it's used everythere in
> c-c and m-c. And for the Windows CSS you can use this:

ok

>   #attachmentBucket attachmentitem[dropOn="top"] {
>     border-top-color: Highlight;
>   }
> 
>   #attachmentBucket attachmentitem[dropOn="bottom"] {
>     border-bottom-color: Highlight;
>   }

Great, thanks, that looks a lot better.

> For the other platforms you can use the colors from here:
> 
> https://dxr.mozilla.org/comm-central/search?q=treechildren%3A%3A-moz-tree-
> cell-text(primary%2C+dropOn)+-path%3Aobj-x86&redirect=false
> and
> https://dxr.mozilla.org/comm-central/search?q=treechildren%3A%3A-moz-tree-
> cell(primary%2C+dropOn)+-path%3Aobj-x86&redirect=false
> 
> So for Linux it's 'Highlight' too. and for OSX '#a1a1a1'.

I might have to come back to you for that, I'll happily leave things CSS to you... ;)
(In reply to Thomas D. from comment #34)
> The only thing which does not make sense is to drop multiple selected items
> as a block *between two selected items*, because the result would be
> unpredictable and self-contradictory.

For the avoidance of doubt, what does not make sense is to drop multiple selected items as a block *between two /directly adjacent/ selected items* (as that's really dropping the selection into itself, which doesn't work). But there's nothing wrong with wanting do drop the entire collapsed block at a free space in the center of the original selection, like this:

A.txt
B.txt#
C.txt
------
D.txt#
E.txt
F.txt#
G.txt

Then after dropping at dropmarker directly above a middle selected block (e.g. above selected D.txt = below unselected C.txt), you'll get this:

A.txt
C.txt
B.txt#
D.txt#
F.txt#
E.txt
G.txt

It just works. You don't have to do it if you don't need it. It's totally ok and works fine to just select a single block of adjacent items and move it to another position outside of the block (which then requires skipping at least one non-selected item, otherwise the whole block won't move).
(In reply to Thomas D. from comment #35)
> (In reply to Thomas D. from comment #34)
> > The only thing which does not make sense is to drop multiple selected items
> > as a block *between two selected items*, because the result would be
> > unpredictable and self-contradictory.
> 
> For the avoidance of doubt, what does not make sense is to drop multiple
> selected items as a block *between two /directly adjacent/ selected items*
> (as that's really dropping the selection into itself, which doesn't work).

So the case which we can't allow (and which is prevented by my code, showing "can't drop here" image) is this:

A.txt
B.txt#
C.txt#
--/--/--
D.txt#
E.txt
F.txt
G.txt
(In reply to Thomas D. from comment #34)
> However, dragging *multiple*, *disjoint* selected blocks is another story.
> It's totally ok, intuitive and consistent to do this (# indicates selected
> items, ----- indicating drop marker):
> 
> A.txt
> ------
> B.txt#
> C.txt
> D.txt#
> E.txt
> F.txt#
> G.txt

...which means I still need Richard's help how to draw a top-border (as a dropmarker) on a selected (and possibly also focused) item. It must be possible, with the right CSS specificity and/or !important. Maybe my proposal of having selectedFirstOfBlock and selectedLastOfBlock attributes also helps to increase specificity (see bug 1412758, attachment 8925253 [details] [diff] [review]).
Flags: needinfo?(richard.marti)
(In reply to Thomas D. from comment #37)
> ...which means I still need Richard's help how to draw a top-border (as a
> dropmarker) on a selected (and possibly also focused) item. It must be
> possible, with the right CSS specificity and/or !important. Maybe my
> proposal of having selectedFirstOfBlock and selectedLastOfBlock attributes
> also helps to increase specificity (see bug 1412758, attachment 8925253 [details] [diff] [review]
> [details] [diff] [review]).

With this CSS patch on top of Bug 1412758 (and remove of the CSS part in your patch) the drop-on marker are always shown.

What I saw today was, that the drop-on marker stays on the last position, also when the mouse was released. Also does no reordering happen.
Flags: needinfo?(richard.marti)
So here's an updated patch (now with the little CSS tweak by Richard from attachment 8927651 [details] [diff] [review]), to be applied on top of the following:

attachment 8928714 [details] [diff] [review] (to get the markers for styling)
attachment 8927645 [details] [diff] [review] (to get the new styling)

Caveat: Dropmarker styling only works on Windows, functionality I guess should work elsewhere but you won't see the dropmarker.

Richard, on my Windows install this works correctly as it should, and exactly as described in my previous comments. Can you try again on Windows?
If you look at the code, I am cleaning up dropmarkers, and I'm also moving items correctly. So if it completely doesn't work, that looks like a problem on your side.
Attachment #8924354 - Attachment is obsolete: true
Attachment #8927651 - Attachment is obsolete: true
Attachment #8924354 - Flags: review?(acelists)
Attachment #8928927 - Flags: review?(acelists)
Attachment #8928927 - Flags: feedback?(richard.marti)
Attachment #502170 - Attachment is obsolete: true
Attachment #8928927 - Attachment description: Patch V.3.1 (WIP, Windows-only, on top of markers attachment 8928714 & styles attachment 8927645): (updated, add Richard's dropOn css) Implement d&d reordering → Patch V.3.1 (WIP, Windows-only, on top of markers attachment 8928714 & styles attachment 8927645): (updated, add Richard's dropOn css) Implement d&d reordering (Caveat: many whitespace changes!)
Comment on attachment 8928927 [details] [diff] [review]
Patch V.3.1 (WIP, Windows-only, on top of markers attachment 8928714 [details] [diff] [review] & styles attachment 8927645 [details] [diff] [review]): (updated, add Richard's dropOn css) Implement d&d reordering (Caveat: many whitespace changes!)

This works now. :)
Attachment #8928927 - Flags: feedback?(richard.marti) → feedback+
Attached patch Bug229224-CSS.patch (obsolete) — Splinter Review
This are the missing rules for Linux and Mac.
We also forgot classic Windows.

Thomas, when you have to make a new patch, you can combine this patch with yours. I need no credit for this on submit. :)
Attachment #8929759 - Flags: review?(acelists)
This patch requires the current patches of Bug 1412758.

Aceman, let's make sure we land this in time so that it will ship together with Bug 663695 in the next release with a bit of exposure before.

Richard, CSS still looks as if there's something missing for Windows?
And why attachmentlist.css only changed in Windows?
I didn't look at the details, as you're the expert...
Attachment #8928927 - Attachment is obsolete: true
Attachment #8928927 - Flags: review?(acelists)
Flags: needinfo?(richard.marti)
Attachment #8938756 - Flags: review?(acelists)
(In reply to Thomas D. from comment #42)
> Created attachment 8938756 [details] [diff] [review]

> Richard, CSS still looks as if there's something missing for Windows?
> And why attachmentlist.css only changed in Windows?
> I didn't look at the details, as you're the expert...

I'd guess that attachmentlist.css is the more appropriate place for attachmentBucket-specific CSS...
(In reply to Thomas D. from comment #43)
> (In reply to Thomas D. from comment #42)
> > Created attachment 8938756 [details] [diff] [review]
> 
> > Richard, CSS still looks as if there's something missing for Windows?
> > And why attachmentlist.css only changed in Windows?
> > I didn't look at the details, as you're the expert...
> 
> I'd guess that attachmentlist.css is the more appropriate place for
> attachmentBucket-specific CSS...

This file is used for the attachment list in main window's message pane. The code I used until now on Windows applied without issues also there. But now you want in bug 1412758 that I work with the adjacent sibling combinator (... + ...). This needs now to be done in messageheaders.css to not affect the message pane attachment items.

Linux and Mac do this already to not affect the message pane attachment items.

But I need to check, if it would be better to move the Windows changes here also to messageheaders,css (especially the @media (-moz-windows-default-theme: 0) {} ) part. The dropOn part doesn't harm as it it only used in composer.
Flags: needinfo?(richard.marti)
Moved the Windows styles to messengercompose.css to do the same on all platforms.
Attachment #8938763 - Flags: review?(acelists)
Attachment #8929759 - Attachment is obsolete: true
Attachment #8929759 - Flags: review?(acelists)
Comment on attachment 8938756 [details] [diff] [review]
Patch V.3.2 (WIP, needs attachment 8938752 [details] [diff] [review], attachment 8929613 [details] [diff] [review]): (updated, add Richard's Linux/MAC css) Implement d&d reordering

Obsoleting this patch because of the CSS changes in the new patch.
Attachment #8938756 - Attachment is obsolete: true
Attachment #8938756 - Flags: review?(acelists)
Comment on attachment 8938763 [details] [diff] [review]
229224_reorder_attachments_d&d.patch

Review of attachment 8938763 [details] [diff] [review]:
-----------------------------------------------------------------

What a code drop :-P
Yes, this seems to work nicely (also applies without bug 1412758 with a little fuzz).

This still sometimes produces an error when dropping a block of attachments onto itself:
19:31:32.165 TypeError: aDragSession.sourceNode is null 1 MsgComposeCommands.js:5912:1
	onDragOver chrome://messenger/content/messengercompose/MsgComposeCommands.js:5912:1
	dragOver chrome://global/content/nsDragAndDrop.js:418:15
	ondragover chrome://messenger/content/messengercompose/messengercompose.xul:1:1

It also happens when dragging an attachment from a file manager over the recipient field and holding it without dropping. Then the errors appear periodically at a rate of several per second. This needs to be fixed.
But dropping the attachment there properly adds it to the bucket.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5680,5 @@
> +   *                                 - "none" if this isn't a valid insertion point
> +   *                                 - "afterLastItem" for appending at the
> +   *                                   bottom of the list.
> +   */
> +  _adjustDropTarget: function (aEvent)

I think there does not need to be a space before '('.

@@ +5690,5 @@
> +
> +    if (target == bucket) {
> +      // Dragging or dropping at top/bottom border of the listbox
> +      let box = target.boxObject;
> +      if((aEvent.screenY - box.screenY) / box.height < 0.5) {

Space after 'if'.

@@ +5718,5 @@
> +      console.log ('mouse targetItem =' + target.attachment.name);
> +      // If we're dragging/dropping in bottom half of attachmentitem,
> +      // adjust target to target.nextSibling (to show dropmarker above that).
> +      let box = target.boxObject;
> +      if((aEvent.screenY - box.screenY) / box.height >= 0.5) {

Space after 'if'.

@@ +5784,3 @@
>    onDrop: function (aEvent, aData, aDragSession)
> +  {
> +

Unneeded empty line?

@@ +5860,5 @@
> +        // We dragged outside the list.
> +       console.log('Removing selected attachments');
> +       RemoveSelectedAttachment();
> +      }
> +*/

To be removed?

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +25,5 @@
>  
> +#attachmentBucket attachmentitem {
> +  border-top: 1px solid transparent;
> +  border-bottom: 1px solid transparent;
> +}

Something here now makes gaps in the selection outline of the attachments, that weren't there before. It is for the line that shows the intended drop target of the dragged items? But the target can't be inside the block or at the edges of it.
Attachment #8938763 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #47)
> Comment on attachment 8938763 [details] [diff] [review]
> 229224_reorder_attachments_d&d.patch
> 
> Review of attachment 8938763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What a code drop :-P
> Yes, this seems to work nicely (also applies without bug 1412758 with a
> little fuzz).

Applies now without fuzz.

> This still sometimes produces an error when dropping a block of attachments
> onto itself:
> 19:31:32.165 TypeError: aDragSession.sourceNode is null 1
> MsgComposeCommands.js:5912:1
> 	onDragOver
> chrome://messenger/content/messengercompose/MsgComposeCommands.js:5912:1
> 	dragOver chrome://global/content/nsDragAndDrop.js:418:15
> 	ondragover
> chrome://messenger/content/messengercompose/messengercompose.xul:1:1
> 
> It also happens when dragging an attachment from a file manager over the
> recipient field and holding it without dropping. Then the errors appear
> periodically at a rate of several per second. This needs to be fixed.
> But dropping the attachment there properly adds it to the bucket.

I'll let Thomas do the needed fixes here.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +5680,5 @@
> > +   *                                 - "none" if this isn't a valid insertion point
> > +   *                                 - "afterLastItem" for appending at the
> > +   *                                   bottom of the list.
> > +   */
> > +  _adjustDropTarget: function (aEvent)
> 
> I think there does not need to be a space before '('.

Removed

> @@ +5690,5 @@
> > +
> > +    if (target == bucket) {
> > +      // Dragging or dropping at top/bottom border of the listbox
> > +      let box = target.boxObject;
> > +      if((aEvent.screenY - box.screenY) / box.height < 0.5) {
> 
> Space after 'if'.

Done

> @@ +5718,5 @@
> > +      console.log ('mouse targetItem =' + target.attachment.name);
> > +      // If we're dragging/dropping in bottom half of attachmentitem,
> > +      // adjust target to target.nextSibling (to show dropmarker above that).
> > +      let box = target.boxObject;
> > +      if((aEvent.screenY - box.screenY) / box.height >= 0.5) {
> 
> Space after 'if'.

Done

> @@ +5784,3 @@
> >    onDrop: function (aEvent, aData, aDragSession)
> > +  {
> > +
> 
> Unneeded empty line?

I think, yes -> removed.

> @@ +5860,5 @@
> > +        // We dragged outside the list.
> > +       console.log('Removing selected attachments');
> > +       RemoveSelectedAttachment();
> > +      }
> > +*/
> 
> To be removed?
> 
> ::: mail/themes/linux/mail/compose/messengercompose.css
> @@ +25,5 @@
> >  
> > +#attachmentBucket attachmentitem {
> > +  border-top: 1px solid transparent;
> > +  border-bottom: 1px solid transparent;
> > +}

I removed it, but Thomas need to decide if the removal is correct.

> Something here now makes gaps in the selection outline of the attachments,
> that weren't there before. It is for the line that shows the intended drop
> target of the dragged items? But the target can't be inside the block or at
> the edges of it.

Fixed. The border bottom on Linux is only needed on the last item. Also fixed the Mac dropOn colors and a forgotten test style.

Thomas, can you fix the first comment? Then it should be ready to land.
Attachment #8938763 - Attachment is obsolete: true
Flags: needinfo?(bugzilla2007)
Attachment #8944191 - Flags: review+
Attachment #8944191 - Attachment description: 229224_reorder_attachments_d&d.patch → 229224_reorder_attachments_d&d V.3.4.patch
Flags: needinfo?(bugzilla2007)
(In reply to :aceman from comment #47)
> Comment on attachment 8938763 [details] [diff] [review]
> This still sometimes produces an error when dropping a block of attachments
> onto itself:

Are you sure it was a block of attachments dropped upon itself which created the error? I doubt that, because attachments must always have a source node.
Can you re-check with this patch applied?

> 19:31:32.165 TypeError: aDragSession.sourceNode is null 1
> MsgComposeCommands.js:5912:1
> 	onDragOver
> chrome://messenger/content/messengercompose/MsgComposeCommands.js:5912:1
> 	dragOver chrome://global/content/nsDragAndDrop.js:418:15
> 	ondragover
> chrome://messenger/content/messengercompose/messengercompose.xul:1:1
> 
> It also happens when dragging an attachment from a file manager over the
> recipient field and holding it without dropping. Then the errors appear
> periodically at a rate of several per second. This needs to be fixed.

Yes, I forgot to check in onDragOver if dragsession.sourceNode really exists, which it doesn't for files from outside. So this should be fixed.

> Something here now makes gaps in the selection outline of the attachments,
> that weren't there before. It is for the line that shows the intended drop
> target of the dragged items? But the target can't be inside the block or at
> the edges of it.

Block edges are allowed as targets for dragging disjunct multiple selections, which get bundled first and then relocated to the edge position.
Attachment #8378409 - Attachment is obsolete: true
Attachment #8944191 - Attachment is obsolete: true
Attachment #8944204 - Flags: ui-review?(richard.marti)
Attachment #8944204 - Flags: review?(acelists)
(In reply to Thomas D. (currently busy elsewhere) from comment #49)
> Are you sure it was a block of attachments dropped upon itself which created
> the error? I doubt that, because attachments must always have a source node.
> Can you re-check with this patch applied?

I am not sure, it may be I hovered outside of the bucket for a moment when trying the block drag.
I can't reproduce it anymore with this patch, it could be the same problem as hovering over recipient field.
Implemented for TB, SM can port the code in a new bug.
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
Whiteboard: [has wip patch][patchlove]
Comment on attachment 8944204 [details] [diff] [review]
Patch V.4: (1 sourceNode error fixed) Implement d&d reordering

Review of attachment 8944204 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this works great now.
No more errors produced and the space between selections is reduced.
Also dragging a file from an external file manager drops it into the bucket. The file is added even when dropping onto a recipient field or subject field. In those cases, the local URL to the file is also pasted into the respective field.

Would it be possible to also add the attachment when dropping the file onto the message body (editor element)? I think Outlook does that. You could try that in a followup bug.
Attachment #8944204 - Flags: review?(acelists) → review+
Attachment #8944204 - Flags: ui-review?(richard.marti) → ui-review+
Forgot to do the Linux gap fix also for Windows Classic.
Attachment #8944204 - Attachment is obsolete: true
Attachment #8944211 - Flags: ui-review+
Attachment #8944211 - Flags: review+
Keywords: checkin-needed
(In reply to :aceman from comment #52)
> Comment on attachment 8944204 [details] [diff] [review]
> Patch V.4: (1 sourceNode error fixed) Implement d&d reordering
> 
> Review of attachment 8944204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes this works great now.
> No more errors produced and the space between selections is reduced.
> Also dragging a file from an external file manager drops it into the bucket.
> The file is added even when dropping onto a recipient field or subject
> field. In those cases, the local URL to the file is also pasted into the
> respective field.

That's a pre-existing bug, local URLs aren't helpful and shouldn't be exposed.
This does NOT happen for me on Windows, where do you see that?

For dragging attached files into subject or body, we could just drop the name of the file as text (followup bug).

> Would it be possible to also add the attachment when dropping the file onto
> the message body (editor element)? I think Outlook does that. You could try
> that in a followup bug.

For image files, I guess my expectation would be to find the image inline after dropping on body. Or to be asked if I want to drop inline or as attachment. But yes, sounds like a nice idea which could be tried, for bigger drop target (which is currently a bit clumsy).
(In reply to Thomas D. (currently busy elsewhere) from comment #54)
> > The file is added even when dropping onto a recipient field or subject
> > field. In those cases, the local URL to the file is also pasted into the
> > respective field.
> 
> That's a pre-existing bug, local URLs aren't helpful and shouldn't be
> exposed.
> This does NOT happen for me on Windows, where do you see that?

On Linux.

> For image files, I guess my expectation would be to find the image inline
> after dropping on body. Or to be asked if I want to drop inline or as
> attachment. But yes, sounds like a nice idea which could be tried, for
> bigger drop target (which is currently a bit clumsy).

If you drag a file, Outlook adds an attachment. So users could expect that. If you are dragging image data (not a file), e.g. copied an area of pixels from an image editing application, you can paste it inline with Ctrl-V.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/26ee90059507
Implement Drag & Drop method for reordering attachments during composition. r=aceman, ui-r=Paenglab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
You need to log in before you can comment on or make changes to this bug.