Last Comment Bug 463088 - Tab tear off cursor needs to be constant
: Tab tear off cursor needs to be constant
Status: VERIFIED FIXED
: dev-doc-complete, verified1.9.1
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: mozilla1.9.2a1
Assigned To: Jim Mathies [:jimm]
:
Mentors:
: 465153 (view as bug list)
Depends on: 225680 505376
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-04 12:01 PST by Jim Mathies [:jimm]
Modified: 2010-12-17 06:29 PST (History)
20 users (show)
mbeltzner: blocking1.9.1-
vladimir: wanted1.9.1+
hskupin: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
GiveFeedback patch v.1 (5.65 KB, patch)
2008-11-04 13:39 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
drag cursor behavior patch v.1 (8.67 KB, patch)
2009-01-12 10:31 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
drag cursor behavior patch v.2 (9.42 KB, patch)
2009-01-14 14:27 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
drag cursor behavior patch v.3 (9.34 KB, patch)
2009-01-16 12:37 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
drag cursor behavior patch v.4 (9.52 KB, patch)
2009-03-03 11:13 PST, Jim Mathies [:jimm]
no flags Details | Diff | Review
drag cursor behavior patch final (9.49 KB, patch)
2009-03-04 13:40 PST, Jim Mathies [:jimm]
enndeakin: review+
Details | Diff | Review
checked in (9.49 KB, patch)
2009-03-06 09:56 PST, Jim Mathies [:jimm]
mbeltzner: approval1.9.1-
Details | Diff | Review
touchup patch (2.80 KB, patch)
2009-03-09 13:23 PDT, Jim Mathies [:jimm]
roc: superreview+
Details | Diff | Review
touchup patch (2.79 KB, patch)
2009-03-09 17:03 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Review
branch-safe version (10.26 KB, patch)
2009-04-15 18:05 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
enndeakin: review-
Details | Diff | Review
191 - builds (10.95 KB, patch)
2009-04-16 11:29 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
enndeakin: review+
mbeltzner: approval1.9.1+
Details | Diff | Review

Description Jim Mathies [:jimm] 2008-11-04 12:01:55 PST
nsNativeDragSource can handle setting the cursor in its GiveFeedback method for the special case where we are dragging a tab.
Comment 1 Jim Mathies [:jimm] 2008-11-04 13:39:02 PST
Created attachment 346324 [details] [diff] [review]
GiveFeedback patch v.1
Comment 2 Jim Mathies [:jimm] 2008-11-04 13:43:30 PST
Comment on attachment 346324 [details] [diff] [review]
GiveFeedback patch v.1

Pretty straight forward, the patch passes in a flag when the nsNativeDragSource object is created. Within that, we set our own cursor when dragging a tab and do the default for everything else.
Comment 3 Jim Mathies [:jimm] 2008-11-04 15:21:47 PST
One update, the delete object call isn't needed for shared cursors, so in the final I'll be plucking that out of the patch.
Comment 4 Neil Deakin 2008-11-05 07:20:15 PST
What special behaviour is this allowing? It doesn't seem like we should be hardcoding special cases like this.
Comment 5 Jim Mathies [:jimm] 2008-11-05 09:33:19 PST
(In reply to comment #4)
> What special behaviour is this allowing? It doesn't seem like we should be
> hardcoding special cases like this.

Sorry I should have clarified a bit more. There was a 3.1 feature request for drag tab to new window functionality. (bug 225680) Mano got that working using some script wizardry, but there was a problem on win32 with cursors, since the cursor displayed is usually controlled by the system based on the underlying window and the contents of the drag.  The idea was that when dragging a tab, the cursor should remain an arrow at all times so as not to confuse users. The only way to get that to work was to detect a tab drag in this code, and control the cursors for that special case manually. Hence this patch.
Comment 6 Ere Maijala (slow) 2008-11-09 11:44:39 PST
Comment on attachment 346324 [details] [diff] [review]
GiveFeedback patch v.1

Is it ok to drop a tab anywhere? I don't think showing a cursor is appropriate if the drop target doesn't actually accept the tab. Do I need to apply the patch from bug 225680 to test this or what? If so, I'd rather have that committed before letting this in.
Comment 7 Jim Mathies [:jimm] 2008-11-10 07:46:23 PST
(In reply to comment #6)
> (From update of attachment 346324 [details] [diff] [review])
> Is it ok to drop a tab anywhere? I don't think showing a cursor is appropriate
> if the drop target doesn't actually accept the tab. Do I need to apply the

Yes, you can drop it anywhere and it'll create a new window. This was supposed to mimic functionality on osx. 

> patch from bug 225680 to test this or what? If so, I'd rather have that
> committed before letting this in.

Yes, it requires 225680.
Comment 8 Neil Deakin 2008-11-10 12:25:01 PST
(In reply to comment #5)
> The idea was that when dragging a tab, the cursor should remain an arrow at all times so as not to confuse users. The only way to get that to work was to detect a tab drag in this code, and control the cursors for that special case manually. 

So why does it require hardcoding? I'd think there might be other cases where this would be useful, so I'd go with adding an API to disable the cursor changing.
Comment 9 Jim Mathies [:jimm] 2008-11-11 09:17:37 PST
Comment on attachment 346324 [details] [diff] [review]
GiveFeedback patch v.1

update coming up..
Comment 10 Jim Mathies [:jimm] 2008-11-11 12:22:35 PST
(In reply to comment #8)
> (In reply to comment #5)
> > The idea was that when dragging a tab, the cursor should remain an arrow at all times so as not to confuse users. The only way to get that to work was to detect a tab drag in this code, and control the cursors for that special case manually. 
> 
> So why does it require hardcoding? I'd think there might be other cases where
> this would be useful, so I'd go with adding an API to disable the cursor
> changing.

When you suggest an api, were you seeing it on something like the native data object or all the way up on something like nsIDOMDataTransfer? I'm curious if you were thinking we need to expose something people can access from script.
Comment 11 Jim Mathies [:jimm] 2008-11-17 08:00:54 PST
*** Bug 465153 has been marked as a duplicate of this bug. ***
Comment 12 Neil Deakin 2008-11-17 08:13:13 PST
(In reply to comment #10)

> When you suggest an api, were you seeing it on something like the native data
> object or all the way up on something like nsIDOMDataTransfer? I'm curious if
> you were thinking we need to expose something people can access from script.

The data transfer. In the future, this may be the only public api that is available.

Is the idea here to prevent the cursor feedback always, or just when the cursor is over something that isn't Firefox, or just on the desktop, or any area that doesn't accept drops?

Also, the patch here is Windows specific, whereas this would need to be implemented all platforms. (The duplicate is actually for all platforms).

For Cocoa, there's a draggingSourceOperationMaskForLocal(sameApp) method that can set the allowed drop operations differently for same-vs-different app, so we should find an api that meets the needs here plus can work on all platforms.
Comment 13 Jim Mathies [:jimm] 2008-11-17 08:27:15 PST
(In reply to comment #12)
> (In reply to comment #10)
> 
> > When you suggest an api, were you seeing it on something like the native data
> > object or all the way up on something like nsIDOMDataTransfer? I'm curious if
> > you were thinking we need to expose something people can access from script.
> 
> The data transfer. In the future, this may be the only public api that is
> available.
> 
> Is the idea here to prevent the cursor feedback always, or just when the cursor
> is over something that isn't Firefox, or just on the desktop, or any area that
> doesn't accept drops?
> 

It should be constant - an arrow for example, throughout the entire drag operation.
Comment 14 Damian Shaw [Quan] 2008-11-19 10:08:32 PST
A duplicate bug had request for blocking release, this bug may as well inherit the request.
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-12-16 09:09:06 PST
Jim, any update
Comment 16 Jim Mathies [:jimm] 2008-12-16 14:12:36 PST
(In reply to comment #15)
> Jim, any update

Yes I was derailed from this for a bit but am back on it this week. Hope to have something by mon.
Comment 17 Jim Mathies [:jimm] 2009-01-12 10:31:15 PST
Created attachment 356554 [details] [diff] [review]
drag cursor behavior patch v.1
Comment 18 Jim Mathies [:jimm] 2009-01-12 10:33:45 PST
Comment on attachment 356554 [details] [diff] [review]
drag cursor behavior patch v.1

Hopfully we can get this in before the freeze.
Comment 19 Neil Deakin 2009-01-14 07:22:17 PST
Comment on attachment 356554 [details] [diff] [review]
drag cursor behavior patch v.1

>+  const short DRAG_CURSOR_NATIVE = 0;
>+  const short DRAG_CURSOR_ARROW = 1;
>+
>+  attribute short mozDragCursorBehavior;

I think this would be a better as a string than an integer, allowing the same values as the css cursor property. This would make it more forward-compatible in that we could allow other cursors in the future, and not have a different means of specifying cursors.

Also, the UUID defined for the interface should be changed.

Otherwise, this is OK.
Comment 20 Jim Mathies [:jimm] 2009-01-14 11:59:56 PST
(In reply to comment #19)
> (From update of attachment 356554 [details] [diff] [review])
> >+  const short DRAG_CURSOR_NATIVE = 0;
> >+  const short DRAG_CURSOR_ARROW = 1;
> >+
> >+  attribute short mozDragCursorBehavior;
> 
> I think this would be a better as a string than an integer, allowing the same
> values as the css cursor property. This would make it more forward-compatible
> in that we could allow other cursors in the future, and not have a different
> means of specifying cursors.
> 
> Also, the UUID defined for the interface should be changed.
> 
> Otherwise, this is OK.

nsIDOMNSDataTransfer though also provides int versions of drop effect. I can work entirely with strings but them I'll need copies of those in the native drag source code. Would it be alright to create the string attribute but also keep the int version (marked as no script as with the others), and move the constants into nsIDragService?
Comment 21 Jim Mathies [:jimm] 2009-01-14 12:20:57 PST
Ok, after some discussion on irc with Neil, we're going with a simple binary switch, a string attribute, and no drag constants. Strings are still up in the air a bit but for now it will be 'auto' & 'arrow'.
Comment 22 Jim Mathies [:jimm] 2009-01-14 12:57:30 PST
(In reply to comment #21)
> Ok, after some discussion on irc with Neil, we're going with a simple binary
> switch, a string attribute, and no drag constants. Strings are still up in the
> air a bit but for now it will be 'auto' & 'arrow'.

Actually, I think I'll go with 'auto' and 'locked', and touch up the method names to reflect the more generic switch.
Comment 23 Jim Mathies [:jimm] 2009-01-14 14:27:04 PST
Created attachment 357031 [details] [diff] [review]
drag cursor behavior patch v.2
Comment 24 Jim Mathies [:jimm] 2009-01-14 14:32:32 PST
Comment on attachment 357031 [details] [diff] [review]
drag cursor behavior patch v.2

requesting an sr on the way this attrib. was implemented.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-14 14:42:50 PST
Why don't we just call it mozCursor and let it take a CSS3 cursor value, as Neil suggested? For now we could just support "auto" (not locked) and "default" (locked). I don't think we need "locked" as a value.
Comment 26 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-01-15 02:00:01 PST
So, fwiw, ideally I would like to be able to _alter_ the cursor on dragover and on dragleave.
Comment 27 Jim Mathies [:jimm] 2009-01-15 10:48:04 PST
(In reply to comment #26)
> So, fwiw, ideally I would like to be able to _alter_ the cursor on dragover and
> on dragleave.

Hmm, that's not the way this has shaped up. You'll be able to choose between keeping the cursor set (the set cursor is dependent on the os, on windows this will be an arrow) or falling back on default behavior where you don't have any control over it's state.
Comment 28 Jim Mathies [:jimm] 2009-01-16 12:37:06 PST
Created attachment 357407 [details] [diff] [review]
drag cursor behavior patch v.3
Comment 29 Neil Deakin 2009-01-20 08:50:01 PST
(In reply to comment #26)
> So, fwiw, ideally I would like to be able to _alter_ the cursor on dragover and
> on dragleave.

What do you want to change it to? If you mean just change to 'copy' or 'move', etc, then that should be happening automatically (although there's a bug on Mac for it not working there) 

Can we get some requirements here? If this patch doesn't do what is desired, we shouldn't be doing it.
Comment 30 Jim Mathies [:jimm] 2009-01-26 10:04:13 PST
(In reply to comment #29)
> (In reply to comment #26)
> > So, fwiw, ideally I would like to be able to _alter_ the cursor on dragover and
> > on dragleave.
> 
> What do you want to change it to? If you mean just change to 'copy' or 'move',
> etc, then that should be happening automatically (although there's a bug on Mac
> for it not working there) 
> 
> Can we get some requirements here? If this patch doesn't do what is desired, we
> shouldn't be doing it.

The original requirement I had was to give us the ability enable/disable the native cursor changes for drag and drop. I think down the road, if there's a need, we could expand that to allow the setting of different cursors. I'm not aware of a need for that functionality at this point.

This came out of the tab drag stuff - on Windows when you drag a tab the cursor changes based on what the underlying window accepts based on the data in the drag object. Each window accepts different types of data so there were a lot of cursor changes as you moved the tab across the desktop. Originally we just wanted to stop that, and keep the cursor constant. Hence this patch and the first quick fix I posted a while back.

As far as I know we don't need to control the type of cursor, we just wanted to clean up the native cursor changes which are confusing to the user.
Comment 31 Jim Mathies [:jimm] 2009-02-03 15:14:40 PST
Hey Mano, any feedback on this?
Comment 32 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-04 05:43:00 PST
Let me explain first the way things are implement right now

Case 1: Drop on "plan" browser chrome -> nothing happens, dropaction is none.
Case 2: Drop on text fields within the browser window, on the bookmarks toolbar and on other chrome elements -> as in Fx3 (not detach).
Case 3: Drop on content area -> detach - happens in the DROP event. dropaction is probably set to move (maybe link, but not to _none_)
Case 4: Drop outside the browser window -> If the dropaction is none (and that will always be the case once I remove the text/plain data), deatach happens on DRAGEND.

For first couple of cases, the automated behavior is desired, meaning either the move, copy, link or no-drop icon. For the other cases, It would be nice to set a custom icon for "detach" and not just the normal pointer.

Differentiating between the first case to the last case in the back-end seems wrong to me, and so is differentiating between the two other case. Thus, ideally, I would be able to set the cursor to either "auto" or a custom image in both the dragover and dragleave events.
Comment 33 Jim Mathies [:jimm] 2009-02-04 07:59:15 PST
Are we sure we want to be implementing custom drag cursors this close to 3.1? Maybe a better solution would be to stick with arrows for the 3.1 release and spin a mozCursor="custom" enhancement off for 3.2?
Comment 34 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-04 08:14:39 PST
That's why I said "it would be nice". There will still be two modes: "auto" and "default-pointer", which may be set anytime during the drag operation.
Comment 35 Jim Mathies [:jimm] 2009-02-04 08:22:02 PST
(In reply to comment #34)
> That's why I said "it would be nice". There will still be two modes: "auto" and
> "default-pointer", which may be set anytime during the drag operation.

Cool, I can split that off. Do you see any issues with the "drag cursor behavior patch v.3" patch as it stands now based on what you want to do?
Comment 36 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-04 08:28:20 PST
As long as you can set mozCursor within both dragover and dragleave, it seems fine.
Comment 37 Jim Mathies [:jimm] 2009-02-04 09:07:34 PST
(In reply to comment #36)
> As long as you can set mozCursor within both dragover and dragleave, it seems
> fine.

Hmm, then this isn't going to work then. I'll need to see if there's a way to get information to the nsNativeDragSource object during the drag operation. Currently this patch sets the value at the beginning of the drag.
Comment 38 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-03-02 03:18:47 PST
Jim, what's up?
Comment 39 Jim Mathies [:jimm] 2009-03-02 06:53:21 PST
(In reply to comment #38)
> Jim, what's up?

I haven't worked on this since that last patch I posted. I'll see what I can do this week.
Comment 40 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-03-02 14:37:19 PST
If we're going to do this in 1.9.1, the api must land before b3.
Comment 41 Jim Mathies [:jimm] 2009-03-02 16:52:35 PST
(In reply to comment #40)
> If we're going to do this in 1.9.1, the api must land before b3.

What is the rush for beta 3 if we are planning a beta 4 with other fixes 6 weeks out? I might be able to land this this week as I'm looking at it starting tomorrow, but if I don't make that I would think beta 4 would work just as well?
Comment 42 Jim Mathies [:jimm] 2009-03-03 11:13:30 PST
Created attachment 365250 [details] [diff] [review]
 drag cursor behavior patch v.4

Ok, updated.
Comment 43 Neil Deakin 2009-03-04 11:40:45 PST
Comment on attachment 365250 [details] [diff] [review]
 drag cursor behavior patch v.4

>+NS_IMETHODIMP
>+nsDOMDataTransfer::GetMozCursor(nsAString& aCursorState)
>+{
>+  if (!mCursorState)
>+    aCursorState.AssignASCII("auto");
>+  else
>+    aCursorState.AssignASCII("default");

Flip the two lines so you don't have to negate the condition.
 
>+NS_IMETHODIMP
>+nsDOMDataTransfer::SetMozCursor(const nsAString& aCursorState)
>+{
>+  if (aCursorState.EqualsASCII("default")) {
>+    // Lock the cursor to an arrow during the drag.
>+    mCursorState = PR_TRUE;
>+  }
>+

Here, you really want:

mCursorState = aCursorState.EqualsASCII("default");


+   *  auto - use default sytem behavior.

'system behaviour'


Is GiveFeedback called repeatedly during a drag? Does it matter that the cursor is being set every time?

Also, do we want to be able to change mozCursor during a drag, or just during dragstart?
Comment 44 Jim Mathies [:jimm] 2009-03-04 11:55:36 PST
(In reply to comment #43)
> (From update of attachment 365250 [details] [diff] [review])
> >+NS_IMETHODIMP
> >+nsDOMDataTransfer::GetMozCursor(nsAString& aCursorState)
> >+{
> >+  if (!mCursorState)
> >+    aCursorState.AssignASCII("auto");
> >+  else
> >+    aCursorState.AssignASCII("default");
> 
> Flip the two lines so you don't have to negate the condition.
> 
> >+NS_IMETHODIMP
> >+nsDOMDataTransfer::SetMozCursor(const nsAString& aCursorState)
> >+{
> >+  if (aCursorState.EqualsASCII("default")) {
> >+    // Lock the cursor to an arrow during the drag.
> >+    mCursorState = PR_TRUE;
> >+  }
> >+
> 
> Here, you really want:
> 
> mCursorState = aCursorState.EqualsASCII("default");
> 
> 
> +   *  auto - use default sytem behavior.
> 
> 'system behaviour'
> 

will update.

> 
> Is GiveFeedback called repeatedly during a drag?

Yes, although I didn't notice any performance issues. We could resurrect the idea of an int property though to be safe.

> Does it matter that the cursor
> is being set every time?

That's the way you're supposed to do it.

> Also, do we want to be able to change mozCursor during a drag, or just during
> dragstart?

Asaf asked for dragover and dragleave so I think covering everything makes good sense.
Comment 45 Neil Deakin 2009-03-04 12:02:30 PST
Yes, although I didn't notice any performance issues. We could resurrect the
idea of an int property though to be safe.

No, if setting the cursor every time is correct, then just leave it as is.
Comment 46 Jim Mathies [:jimm] 2009-03-04 13:40:27 PST
Created attachment 365527 [details] [diff] [review]
drag cursor behavior patch final
Comment 47 Neil Deakin 2009-03-06 08:51:18 PST
Comment on attachment 365527 [details] [diff] [review]
drag cursor behavior patch final

>+   *  auto - use default sytem behaviour.

Actually, I meant to correct the word 'sytem'. But if you want to be more Canadian/British, go right ahead ;)
Comment 48 Jim Mathies [:jimm] 2009-03-06 09:44:40 PST
(In reply to comment #47)
> (From update of attachment 365527 [details] [diff] [review])
> >+   *  auto - use default sytem behaviour.
> 
> Actually, I meant to correct the word 'sytem'. But if you want to be more
> Canadian/British, go right ahead ;)

Heh, I'll update it before I land.  :)
Comment 49 Jim Mathies [:jimm] 2009-03-06 09:56:21 PST
Created attachment 365928 [details] [diff] [review]
checked in

mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/37207402f983
Comment 50 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-03-07 07:24:37 PST
Don't you need super-review for most of this patch?
Comment 51 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-03-07 07:28:59 PST
SetMozCursor should probably throw on toolkit-widgets in which this behavior is not implemented. "probably", because I have no idea how are similar cases handled.
Comment 52 Neil Deakin 2009-03-07 12:46:08 PST
SetMozCursor isn't in platform specific code. The documentation for mozCursor should probably just say the actual cursor used is platform dependant, or that the property has no effect on some platforms.
Comment 53 Jim Mathies [:jimm] 2009-03-07 15:05:56 PST
(In reply to comment #50)
> Don't you need super-review for most of this patch?

I wasn't aware that I did but I can certainly get one and deal with it if I've miss stepped.
Comment 54 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-07 18:34:43 PST
Yeah, this patch should have been super-reviewed. Perhaps roc can sr it after the fact?

Also, this should be marked FIXED now that it's checked in on trunk, no?
Comment 55 Jim Mathies [:jimm] 2009-03-08 09:08:19 PDT
Comment on attachment 365527 [details] [diff] [review]
drag cursor behavior patch final

Roc, this landed early which was was my fault. I can pull it back out if need be (although it went through quite a bit of discussion before neil gave it the ok). Can you look over what landed and let me know if I need to pull it and fix something or just update what's already landed?
Comment 56 Jim Mathies [:jimm] 2009-03-08 09:14:51 PDT
Also, I need to figure out if this needs to be split off into other platform bugs. From my understanding the cursor issue isn't present on osx but as mentioned previously we might want some sort of functionality, even a simple throw, on osx. As for linux I'm unsure of what's needed there. Both bugs related to this were opened due to windows issues.
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-08 12:44:36 PDT
+  // Indicates the behavior of the cursor during drag operations
+  PRPackedBool mCursorState;

This comment, and the variable name, aren't very helpful. It needs to say what the possible states actually mean.

+  if (mCursorState)
+    aCursorState.AssignASCII("default");
+  else
+    aCursorState.AssignASCII("auto");

These should be AssignLiteral, and also have {} around them.

+  mCursorState = aCursorState.EqualsASCII("default");

EqualsLiteral

+  /**
+   * Sets the drag cursor state. Primarily used to control the cursor during
+   * tab drags, but could be expanded to other uses.
+   *
+   * Possible values:
+   *  auto - use default system behavior.
+   *  default - set the cursor to an arrow during the drag operation.
+   */

Document that other values are treated as "auto".

+    if (cursor.EqualsASCII("default")) {

EqualsLiteral

You can fix these as a separate checkin on top of what you already landed.
Comment 58 Jim Mathies [:jimm] 2009-03-09 13:23:15 PDT
Created attachment 366370 [details] [diff] [review]
touchup patch
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-09 14:02:52 PDT
Comment on attachment 366370 [details] [diff] [review]
touchup patch

+  }
+  else {

One line.
Comment 60 Henrik Skupin (:whimboo) 2009-03-09 15:54:05 PDT
Reopening while it is not completely fixed...
Comment 61 Jim Mathies [:jimm] 2009-03-09 17:03:52 PDT
Created attachment 366434 [details] [diff] [review]
touchup patch
Comment 62 Jim Mathies [:jimm] 2009-03-09 17:04:34 PDT
http://hg.mozilla.org/mozilla-central/rev/aac514561b0b
Comment 63 Henrik Skupin (:whimboo) 2009-03-11 17:12:48 PDT
Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090311 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090311051703
Comment 64 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-07 10:38:02 PDT
Comment on attachment 365928 [details] [diff] [review]
checked in

Changes a scriptable interface, so we can't take this for 3.5
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-07 12:52:59 PDT
That interface is new in 3.5, does that make a difference?
Comment 66 Henrik Skupin (:whimboo) 2009-04-07 13:09:42 PDT
(In reply to comment #65)
> That interface is new in 3.5, does that make a difference?

Beltzner, I believe you haven't gotten this question.
Comment 67 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-04-08 15:50:18 PDT
Well, it has shipped in b3, after which we promised no extension-breaking compat changes... Probably unlikely to pose a problem in practice, but I'm not sure I really understand the benefits well enough to make that call.
Comment 68 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-15 06:09:47 PDT
Marking late-compat just so we can add this to the list, but the fact that it's new for 191 and unlikely to be something that add-ons are targeting makes me feel like it's worth the user experience win here
Comment 69 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-15 06:09:50 PDT
Comment on attachment 365928 [details] [diff] [review]
checked in

a191=beltzner
Comment 70 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-15 06:15:26 PDT
Comment on attachment 365928 [details] [diff] [review]
checked in

Sorry for the churn - Mano and I are going back and forth trying to figure out if it's worth him making a branch-safe (non changing API) patch for this or not.
Comment 71 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-15 06:20:56 PDT
Just to clarify - we still consider the scriptable usage unlikely to break due to this patch. The potential issue is with binary addons and the like.
Comment 72 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-15 06:55:05 PDT
And to be clearer yet: Mano's working up a branch patch, which Enn will review.
Comment 73 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-15 18:05:27 PDT
Created attachment 373031 [details] [diff] [review]
branch-safe version
Comment 74 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-15 18:07:30 PDT
Comment on attachment 373031 [details] [diff] [review]
branch-safe version

hrm, this doesn't seem to build. Investigating
Comment 75 Neil Deakin 2009-04-16 09:15:56 PDT
Comment on attachment 373031 [details] [diff] [review]
branch-safe version

Well, no then, if it doesn't build.

Personally, if I didn't want to change the interface, I would have just used a hard-coded hacky patch like the very first patch for 1.9.1.
Comment 76 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-16 09:48:12 PDT
No, that won't do due to comment 26 (Yes, I need to file a follow up for the tabbrowser.xml part).
Comment 77 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-16 11:29:43 PDT
Created attachment 373172 [details] [diff] [review]
191 - builds

see previous comment
Comment 78 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-16 15:52:06 PDT
Removing late-compat keyword as we now have a 191 patch that modifies something that was added only in b3.
Comment 79 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-16 15:54:28 PDT
Comment on attachment 373172 [details] [diff] [review]
191 - builds

a191=beltzner
Comment 80 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-16 23:54:07 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1dd773e64a93
Comment 81 Henrik Skupin (:whimboo) 2009-04-21 02:03:17 PDT
Verified on 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre ID:20090420044915
Comment 82 Henrik Skupin (:whimboo) 2009-04-21 02:44:59 PDT
https://litmus.mozilla.org/show_test.cgi?id=7578 has been updated to cover this regression.
Comment 83 Eric Shepherd [:sheppy] 2010-03-09 14:15:44 PST
I'm not clear on what if anything really requires documentation here; this appears to be pretty much an internal change to an undocumented interface.
Comment 84 Eric Shepherd [:sheppy] 2010-11-05 05:56:07 PDT
Dunno what I was smoking when I wrote comment 83, but it must have been good. I've updated the article about DataTransfer now:

https://developer.mozilla.org/En/DragDrop/DataTransfer#mozCursor

Note You need to log in before you can comment on or make changes to this bug.