256362 - bookmarks: drag and drop service doesn't keep track of action consistently

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Turns out, we can't rely on the drag service to keep track of the action
specified (LINK/COPY/MOVE) and do sane things with it.  This is the underlying
cause of a bunch of bugs -- we're expecting that a MOVE implies that we're to
remove the old element, which we can only do for bookmarks in this case.

The solution here is to ignore LINK/COPY/MOVE unless the drag source is a
bookmark (because we set it correctly and it gets through the OS munging
correctly), and assume everything else is a copy.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is blocking a handful of bugs; will nail this ASAP.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: bm-fix-dnd-0.patch

This patch fixes (at least on linux, for me) the issues with D&D:

1) It only enforces MOVE semantics if the source is a bookmark (determined by
whether it has a source rdf resource ID)

2) Doesn't allow dragging into live bookmark containers

3) Sets MOVE/COPY/LINK on drags from the proxy icon, which should make dragging
to the desktop from there work again.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix-bm-dnd-0.patch

Er, this patch file I meant.. same comments as above.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 156816 [details] [diff] [review]
fix-bm-dnd-0.patch

pch, can you take a look at this patch?  It seems to do the right thing, though
as I mentioned, I can't test on mac atm but will try to do so later this
afternoon.

Comment 5

[Pierre Chanial]

Comment on attachment 156816 [details] [diff] [review]
fix-bm-dnd-0.patch

>-    var isHorizontal = (target.localName == "toolbarbutton");
>    if ((PBStyle.direction == 'rtl') && isHorizontal) {
>      if (orientation == BookmarksUtils.DROP_AFTER)
>        orientation = BookmarksUtils.DROP_BEFORE;
>      else if (orientation == BookmarksUtils.DROP_BEFORE)
>        orientation = BookmarksUtils.DROP_AFTER;
>+    var isHorizontal = (target.localName == "toolbarbutton");
>+    if ((PBStyle.direction == 'rtl') && isHorizontal) {
>+      if (orientation == BookmarksUtils.DROP_AFTER)
>+        orientation = BookmarksUtils.DROP_BEFORE;
>+      else if (orientation == BookmarksUtils.DROP_BEFORE)
>+        orientation = BookmarksUtils.DROP_AFTER;

seems like there has been problem when updating this file.

for canDropOn and canDropBeforeAfter, it would be better to rely on
BookmarksUtils.isSelectionValidForInsertion and update it for livemarks. That
way, the paste command disable state in the bookmarks context menus would be
consistently updated.

For the history, long time ago, if an item in a selection was not deletable,
but if there were other items that could be moved, then the whole transaction
was not prevented, but	only performed on the mutable items. At that time, the
immutable items (also the not bookmark ones, such as text selection, url bar
proxy dragging) had selection.parent[i] == null and a check on it was performed
during the transaction. 

I changed this behaviour, and a move transaction now is supposed to be
performed only if all the selection items can be moved, but during that change,
I removed the selection.parent==null check that leads to problems vlad's
experiencing. However, there are still some inconsistent remnants in
checkSelection() parent==null for 'find:' and 'file:' bookmarks.
Now, let's take a selection that has a regular bookmark, and a 'file:' or
'find:' bookmark. What is the expected behaviour when the user drag the
selection?

I think that the regular bookmark should be moved and the 'file:' or 'find:'
bookmarks should be copied and not be considered as immutable (flagging it so
would cancel the whole transaction). We should then reintroduce the
selection.parent==null check.
Thoughts?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #5)
> (From update of attachment 156816 [details] [diff] [review])
> >-    var isHorizontal = (target.localName == "toolbarbutton");
[...]
> 
> seems like there has been problem when updating this file.

Yeah, the file had ^M's in it originally, this just got rid of the ^M's and
expanded out the lines.

> for canDropOn and canDropBeforeAfter, it would be better to rely on
> BookmarksUtils.isSelectionValidForInsertion and update it for livemarks. That
> way, the paste command disable state in the bookmarks context menus would be
> consistently updated.

Good point.  I'll move the parent find/test check there.

> For the history, long time ago, if an item in a selection was not deletable,
> but if there were other items that could be moved, then the whole transaction
> was not prevented, but only performed on the mutable items. At that time, the
> immutable items (also the not bookmark ones, such as text selection, url bar
> proxy dragging) had selection.parent[i] == null and a check on it was performed
> during the transaction. 
> 
> I changed this behaviour, and a move transaction now is supposed to be
> performed only if all the selection items can be moved, but during that change,
> I removed the selection.parent==null check that leads to problems vlad's
> experiencing. However, there are still some inconsistent remnants in
> checkSelection() parent==null for 'find:' and 'file:' bookmarks.
> Now, let's take a selection that has a regular bookmark, and a 'file:' or
> 'find:' bookmark. What is the expected behaviour when the user drag the
> selection?

What do you consider a regular bookmark?  Anything that's a bookmark (i.e.
already is in the bookmarks datasource, has a rdf ID, etc.) should just be
treated as a bookmark -- if the user is dragging three bookmarks, they should
all be moved, regardless of their URI.

I don't think it's possible to come up with a selection that has 1 bookmark item
and one "link" (drag from a page or something).  I think the current behaviour
for this is fine though; if there are immutable things mixed in, we should
probably disallow the move and force the user to explicitly copy (otherwise they
could be expecting a move and get confused.. both options aren't great).

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix-bm-dnd-1.patch

Updated patch; uses isValidTargetContainer instead of doing explicit type
checks itself.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 156902 [details] [diff] [review]
fix-bm-dnd-1.patch

Looking for some quick review to get this landed.. would like at least 2-3
nightly cycles before PR freeze.

Comment 9

[Pierre Chanial]

Comment on attachment 156902 [details] [diff] [review]
fix-bm-dnd-1.patch

>diff -u -8 -p -r1.29.4.12 bookmarksMenu.js
>--- components/bookmarks/content/bookmarksMenu.js	19 Aug 2004 17:59:06 -0000	1.29.4.12
>+++ components/bookmarks/content/bookmarksMenu.js	23 Aug 2004 18:22:35 -0000
>@@ -599,21 +599,29 @@ var BookmarksMenuDNDObserver = {
>     this.onDragRemoveFeedBack(target);
> 
>     var selection = BookmarksUtils.getSelectionFromXferData(aDragSession);
> 
>     var orientation = BookmarksMenu.getBTOrientation(aEvent);
> 
>     // For RTL PersonalBar bookmarks buttons, orientation should be inverted (only in drop case)
>     var PBStyle = window.getComputedStyle(document.getElementById("PersonalToolbar"),'');
>-    var isHorizontal = (target.localName == "toolbarbutton");
>    if ((PBStyle.direction == 'rtl') && isHorizontal) {
>      if (orientation == BookmarksUtils.DROP_AFTER)
>        orientation = BookmarksUtils.DROP_BEFORE;
>      else if (orientation == BookmarksUtils.DROP_BEFORE)
>        orientation = BookmarksUtils.DROP_AFTER;
>+    var isHorizontal = (target.localName == "toolbarbutton");
>+    if ((PBStyle.direction == 'rtl') && isHorizontal) {
>+      if (orientation == BookmarksUtils.DROP_AFTER)
>+        orientation = BookmarksUtils.DROP_BEFORE;
>+      else if (orientation == BookmarksUtils.DROP_BEFORE)
>+        orientation = BookmarksUtils.DROP_AFTER;
>     }
still there.


>         canDropOn: function(index)
>         {
>           var dragSession = DS.getCurrentSession();
>           if (!dragSession)
>             return false;
>-          var selection = BookmarksUtils.getSelectionFromXferData(dragSession);
>-          return !selection.containsImmutable;
>+
>+          // we can only test for kCopyAction if the source is a bookmark
>+          var checkCopy = dragSession.isDataFlavorSupported("moz/rdfitem");
>+          const kCopyAction = kDSIID.DRAGDROP_ACTION_COPY + kDSIID.DRAGDROP_ACTION_LINK;
>+          if (!checkCopy || dragSession.dragAction & kCopyAction) {
>+              return true;
>+          } else {
>+              var selection = BookmarksUtils.getSelectionFromXferData(dragSession);
>+              return !selection.containsImmutable;
>+          }
>         },

should use BookmarksUtils.isValidContainer

>         canDropBeforeAfter: function(index, before)
>         {
>           var dragSession = DS.getCurrentSession();
>           if (!dragSession)
>             return false;
>+
>+          // we can only test for kCopyAction if the source is a bookmark
>+          var checkCopy = dragSession.isDataFlavorSupported("moz/rdfitem");
>+          const kCopyAction = kDSIID.DRAGDROP_ACTION_COPY + kDSIID.DRAGDROP_ACTION_LINK;
>+          var isCopy = dragSession.dragAction & kCopyAction;
>           var selection = BookmarksUtils.getSelectionFromXferData(dragSession);
>-          if (selection.containsImmutable)
>+          if (checkCopy && !isCopy && selection.containsImmutable)
>             return false;
> 
>+          var rsrc = this.mOuter.getRowResource(index);
>+          var rsrcParents = BookmarksUtils.getParents(rsrc, this.mOuter.db);
>+          for (var i = 0; i < rsrcParents.length; i++) {
>+              if (!BookmarksUtils.isValidTargetContainer (rsrcParents[i], selection))
>+                  return false;
>+          }
>+
>           if (index != 0)
>             return true;
>-          if (this.mOuter.getRowResource(index).Value != "NC:BookmarksRoot")
>+          if (rsrc.Value != "NC:BookmarksRoot")
>             return true;
>           return before? false:this.mOuter.treeBoxObject.view.isContainerOpen(0)
>         },

the other way around, you should update isValidTargetContainer to deal with
livemarks

And concerning the the DND action type part, I am still unconvinced we need to
discriminate between bookmarks and the other flavours at this level.
My last comment was suggesting to add back the selection.parent (==null for non
regular bookmarks) check in the transactions because we will need it for some
types of bookmarks.

Comment 10

[Pierre Chanial]

oops, I wasn't checked and I didn't see you last comment.
Sorry for the irrelevant part of my review.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix-bm-dnd-2.patch

Updated patch, after IRC discussion with pch.

With this, if the selection contains any items that can't be moved, the default
operation becomes to copy (instead of just aborting).

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 156997 [details] [diff] [review]
fix-bm-dnd-2.patch

a=asa for aviary checkin.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, in on aviary; I'll check with the next sweetlou build to make sure that
everything works like it should on windows.

Comment 14

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 15

[Shawn Wilsher :sdwilsh]

I'm pretty sure this bug is no longer valid