463088 - Tab tear off cursor needs to be constant

Status: VERIFIED FIXED

(Core :: Widget, defect, P1)

Description

[Jim Mathies [:jimm]]

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]]

Attachment: GiveFeedback patch v.1

Comment 2

[Jim Mathies [:jimm]]

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]]

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]

What special behaviour is this allowing? It doesn't seem like we should be hardcoding special cases like this.

Comment 5

[Jim Mathies [:jimm]]

(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)]

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]]

(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]

(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]]

Comment on attachment 346324 [details] [diff] [review]
GiveFeedback patch v.1

update coming up..

Comment 10

[Jim Mathies [:jimm]]

(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 12

[Neil Deakin]

(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]]

(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]]

A duplicate bug had request for blocking release, this bug may as well inherit the request.

Comment 15

[Asaf Romano (gone)]

Jim, any update

Comment 16

[Jim Mathies [:jimm]]

(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]]

Attachment: drag cursor behavior patch v.1

Comment 18

[Jim Mathies [:jimm]]

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]

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]]

(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]]

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]]

(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]]

Attachment: drag cursor behavior patch v.2

Comment 24

[Jim Mathies [:jimm]]

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) (email my personal email if necessary)]

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

[Asaf Romano (gone)]

So, fwiw, ideally I would like to be able to _alter_ the cursor on dragover and on dragleave.

Comment 27

[Jim Mathies [:jimm]]

(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]]

Attachment: drag cursor behavior patch v.3

Comment 29

[Neil Deakin]

(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]]

(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]]

Hey Mano, any feedback on this?

Comment 32

[Asaf Romano (gone)]

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]]

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

[Asaf Romano (gone)]

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]]

(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

[Asaf Romano (gone)]

As long as you can set mozCursor within both dragover and dragleave, it seems fine.

Comment 37

[Jim Mathies [:jimm]]

(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

[Asaf Romano (gone)]

Jim, what's up?

Comment 39

[Jim Mathies [:jimm]]

(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

[Asaf Romano (gone)]

If we're going to do this in 1.9.1, the api must land before b3.

Comment 41

[Jim Mathies [:jimm]]

(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]]

Attachment: drag cursor behavior patch v.4

Ok, updated.

Comment 43

[Neil Deakin]

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]]

(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]

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]]

Attachment: drag cursor behavior patch final

Comment 47

[Neil Deakin]

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]]

(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]]

Attachment: checked in

mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/37207402f983

Comment 50

[Asaf Romano (gone)]

Don't you need super-review for most of this patch?

Comment 51

[Asaf Romano (gone)]

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]

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]]

(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: [e-mail]]]

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]]

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]]

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) (email my personal email if necessary)]

+  // 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]]

Attachment: touchup patch

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 366370 [details] [diff] [review]
touchup patch

+  }
+  else {

One line.

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Reopening while it is not completely fixed...

Comment 61

[Jim Mathies [:jimm]]

Attachment: touchup patch

Comment 62

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/aac514561b0b

Comment 63

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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]]

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) (email my personal email if necessary)]

That interface is new in 3.5, does that make a difference?

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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: [e-mail]]]

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]]

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]]

Comment on attachment 365928 [details] [diff] [review]
checked in

a191=beltzner

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

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

[Asaf Romano (gone)]

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]]

And to be clearer yet: Mano's working up a branch patch, which Enn will review.

Comment 73

[Asaf Romano (gone)]

Attachment: branch-safe version

Comment 74

[Asaf Romano (gone)]

Comment on attachment 373031 [details] [diff] [review]
branch-safe version

hrm, this doesn't seem to build. Investigating

Comment 75

[Neil Deakin]

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

[Asaf Romano (gone)]

No, that won't do due to comment 26 (Yes, I need to file a follow up for the tabbrowser.xml part).

Comment 77

[Asaf Romano (gone)]

Attachment: 191 - builds

see previous comment

Comment 78

[Mike Beltzner [:beltzner, not reading bugmail]]

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]]

Comment on attachment 373172 [details] [diff] [review]
191 - builds

a191=beltzner

Comment 80

[Asaf Romano (gone)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1dd773e64a93

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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][⌚️UTC+2]]

https://litmus.mozilla.org/show_test.cgi?id=7578 has been updated to cover this regression.

Comment 83

[Eric Shepherd [:sheppy]]

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]]

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