Closed Bug 483776 Opened 11 years ago Closed 9 years ago

When dragging a tab, Mouse cursor does not change.

Categories

(Firefox :: Tabbed Browser, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: alice0775, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 8 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090316 Firefox/3.5.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090316 Minefield/3.2a1pre

When I dragging a tab,  Mouse cursor does not change. 
It is still the normal pointer. It should be change to the pointer with small rectangle.



Reproducible: Always

Steps to Reproduce:
1.Start Minefield with new profile. 
2.Dragging a tab
3.
Actual Results:  
Mouse cursor does not change. 
It is still the normal pointer. 

Expected Results:  
It should be change to the pointer with small rectangle.

Regression range:
Works fine:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090306 Minefield/3.2a1pre

Broken:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090307 Minefield/3.2a1pre

Changesets between the above range.
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-03-06+04%3A01%3A03&enddate=2009-03-07+03%3A53%3A49
Keywords: regression
Version: unspecified → Trunk
Severity: major → normal
Status: UNCONFIRMED → NEW
Component: General → Tabbed Browser
Ever confirmed: true
OS: Windows XP → All
QA Contact: general → tabbed.browser
Sorry, maybe Bug 463088?
Yes that was the intent of that bug, it's better than before (with the no-action cursor) but could probably still be improved.
Flags: wanted-firefox3.1?
also when dragging a tab on the tab bar while pressing ctrl to clone - the "+" indication does not show
Flags: blocking-firefox3.5?
This was part of the tab tear off work. We expose a new setting to provide control over the cursor, currently it supports "default" behavior and a constant arrow setting. For drags that remain in the tab bar we should probably be be using default rather than auto.
the constant cursor gives no feedback whatsoever

i would like to see:
1. indication when the dragging operation is in action
2. "+" sign when using ctrl to clone
3. relevant indications for special drop zones:
   a. disallowed (or no action) zones
   b. when dropping on a button (e.g. home) it should get highlighted
   c. dropping on a windows folder should not highlight it, as it does now

besides usability these issues are also important for discoveability
See also bug 485105 which is about improving the visual feedback when dragging a tab.
Flags: wanted-firefox3.5?
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
Whiteboard: [polish-easy]
(In reply to comment #4)
> For drags that remain in the tab bar we should probably
> be be using default rather than auto.

I assume you mean "auto rather than default".
I tried to set aEvent.dataTransfer.mozCursor to "auto" in dragover and "default" in dragleave, but it doesn't seem to make any difference. That's on Windows XP.
Flags: wanted-firefox3.5?
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
Whiteboard: [polish-easy]
Restoring flags, assuming Dao didn't mean to switch them back.
Flags: wanted-firefox3.5?
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
(In reply to comment #8)
> assuming Dao didn't mean to switch them back.

correct, that wasn't intentional.
(In reply to comment #7)
> (In reply to comment #4)
> > For drags that remain in the tab bar we should probably
> > be be using default rather than auto.
> 
> I assume you mean "auto rather than default".
> I tried to set aEvent.dataTransfer.mozCursor to "auto" in dragover and
> "default" in dragleave, but it doesn't seem to make any difference. That's on
> Windows XP.

The drag leave event handler isn't getting called on trunk builds from today. Will check 1.9.1 next.
(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #4)
> > > For drags that remain in the tab bar we should probably
> > > be be using default rather than auto.
> > 
> > I assume you mean "auto rather than default".
> > I tried to set aEvent.dataTransfer.mozCursor to "auto" in dragover and
> > "default" in dragleave, but it doesn't seem to make any difference. That's on
> > Windows XP.
> 
> The drag leave event handler isn't getting called on trunk builds from today.
> Will check 1.9.1 next.

Actually it is getting called. The problem is that we use a clone of the original data transfer in events, afaict - 

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMDragEvent.cpp#172

In which case settings changes don't migrate back to the original data transfer object, which the native drag object has a reference to.
Looks like we should be able to address this in event state manager, I'll see if I can work something up.
Attached patch m-c data transfer patch v.1 (obsolete) — Splinter Review
Attached patch 1.9.1 data transfer patch v.1 (obsolete) — Splinter Review
Depends on: 493412
Assignee: nobody → jmathies
Attached patch m-c data transfer patch v.2 (obsolete) — Splinter Review
Touched up comment typos and removed reliance on the drag leave event.
Attachment #377882 - Attachment is obsolete: true
Attached patch 1.9.1 data transfer patch v.2 (obsolete) — Splinter Review
ditto.
Attachment #377883 - Attachment is obsolete: true
Comment on attachment 377950 [details] [diff] [review]
m-c data transfer patch v.2

jonas -> event state manager changes
Attachment #377950 - Flags: review?(jonas)
OS: All → Windows Vista
Not sure I fully understand the significance of wanted 3.5+ and blocking 3.5- but is there any chance this patch will be reviewed & checked in to make 3.5 RC code freeze?
(In reply to comment #18)
> Not sure I fully understand the significance of wanted 3.5+ and blocking 3.5-
> but is there any chance this patch will be reviewed & checked in to make 3.5 RC
> code freeze?

Not likely for RC, it needs to finish up reviews, then bake on trunk. We're past the freeze for RC for the most part and the blocking- indicates drivers felt it wasn't worth holding up the release.
It's unlikely that the first RC is also the last one, though, so if this gets reviewed in the near future, there's a chance that it will be approved later.
Flags: blocking-firefox3.6?
Comment on attachment 377950 [details] [diff] [review]
m-c data transfer patch v.2

Changing up reviewers.
Attachment #377950 - Flags: review?(jonas) → review?(bzbarsky)
Comment on attachment 377950 [details] [diff] [review]
m-c data transfer patch v.2

>+++ b/browser/base/content/tabbrowser.xml

Someone else needs to review this part; I'm not sure the comments here make sense to me, but they need to make sense to a toolkit peer.

>+++ b/content/events/src/nsEventStateManager.cpp
>+      nsDragEvent *dragEvent = (nsDragEvent*)aEvent;

So this is non-null, right?

>+      // collect any changes to the moz curor setting stored in the event's

s/curor/cursor/

>+nsEventStateManager::UpdateDragDataTransfer(nsDragEvent* dragEvent)
>+{
>+  if (!dragEvent || !dragEvent->dataTransfer)

dragEvent can't be null here.  Just asssert that instead of checking?

This method should return void, as far as I can tell.  You never check the return value anyway (and it's always NS_OK).

>+  // collect any changes to the moz curor setting stored in the event's

s/curor/cursor/

>+++ b/content/events/src/nsEventStateManager.h
>+   * Update the initial drag session data transfer with any changes that occur
>+   * on cloned data tranfer objects used for events.

s/tranfer/transfer/.  

>+  nsresult UpdateDragDataTransfer(nsDragEvent* dragEvent);

Per above, return void.

r=bzbarsky on the content changes with those nits picked.
Attachment #377950 - Flags: review?(bzbarsky) → review+
Attachment #377951 - Attachment is obsolete: true
Attached patch m-c data transfer patch v.3 (obsolete) — Splinter Review
Attachment #377950 - Attachment is obsolete: true
Attached patch m-c data transfer patch v.3 (obsolete) — Splinter Review
correct patch!
Attachment #389389 - Attachment is obsolete: true
Attached patch m-c data transfer patch v.3 (obsolete) — Splinter Review
bah, must be a sunday. patch with "cursor" spelled correctly.
Attachment #389390 - Attachment is obsolete: true
Comment on attachment 389391 [details] [diff] [review]
m-c data transfer patch v.3

Dão, can you approve the tabbrowser changes?
Attachment #389391 - Flags: review?(dao)
The patch doesn't seem to apply on trunk.
Attached patch m-c data transfer patch v.3 (obsolete) — Splinter Review
Updated!
Attachment #389391 - Attachment is obsolete: true
Attachment #389454 - Flags: review?(dao)
Attachment #389391 - Flags: review?(dao)
When I drag a tab over the location bar, the mouse pointer flickers between the default and the drag states. I've also managed getting the do-not-drop cursor multiple times while dragging over the desktop, although I don't know how to reproduce this. I'm on XP at the moment.

Also note that dragging a tab over a textfield, like the one I'm typing in, doesn't show the normal drag cursor, but based on the patch I guess that's expected.
Duplicate of this bug: 505376
(In reply to comment #29)
> When I drag a tab over the location bar, the mouse pointer flickers between the
> default and the drag states. I've also managed getting the do-not-drop cursor
> multiple times while dragging over the desktop, although I don't know how to
> reproduce this. I'm on XP at the moment.

So, since this patch enables default behavior in the tab bar, you're going to see some cursor changes. For example dragging over the tab you are currently dragging will result in a no drop symbol. You'll also see changes as you move around the rest of the tab bar. This is windows default behavior, we aren't controlling it.

Personally I'm ok with the locked cursor, but some obviously feel they want the default behavior in the tab bar. This is probably pretty minor to tie to a pref, so it's just a ui call we have to make.

> 
> Also note that dragging a tab over a textfield, like the one I'm typing in,
> doesn't show the normal drag cursor, but based on the patch I guess that's
> expected.

That's expected, this patch only updates the tab browser settings.

FYI, you will see the no-drop cursor in some cases on the desktop with/without this patch. Certain windows that don't support drop objects will get these even though we tell windows to lock the cursor. I'm guessing the no drop desktop cursor you're seeing is related to that. 

Even if we don't make the tab browser changes, the moz cursor code in event state manager should still land since we currently don't update the property correctly during drags.
(In reply to comment #31)
> So, since this patch enables default behavior in the tab bar, you're going to
> see some cursor changes. For example dragging over the tab you are currently
> dragging will result in a no drop symbol. You'll also see changes as you move
> around the rest of the tab bar. This is windows default behavior, we aren't
> controlling it.

It flickered while the mouse rested over the location bar.

> FYI, you will see the no-drop cursor in some cases on the desktop with/without
> this patch. Certain windows that don't support drop objects will get these even
> though we tell windows to lock the cursor. I'm guessing the no drop desktop
> cursor you're seeing is related to that. 

Not sure. I don't think there was another window in my way, but I may be wrong.
(In reply to comment #32)
> (In reply to comment #31)
> > So, since this patch enables default behavior in the tab bar, you're going to
> > see some cursor changes. For example dragging over the tab you are currently
> > dragging will result in a no drop symbol. You'll also see changes as you move
> > around the rest of the tab bar. This is windows default behavior, we aren't
> > controlling it.
> 
> It flickered while the mouse rested over the location bar.

Hmm, shouldn't be happening assuming the tab browser changes / events are getting called right. The cursor should be locked as soon as it leaves the tab bar.
Comment on attachment 389454 [details] [diff] [review]
m-c data transfer patch v.3

I can reproduce both problems from comment 32 with a different computer on Vista. There is no other window involved in the second problem, but it seems to depend on where you drag the tab over. For example, I can't reproduce it when dragging a tab up over the search bar or down over a text-only page. But I can reproduce it frequently when dragging a tab up over the location bar, or down over the content area when about:config is loaded.
Attachment #389454 - Flags: review?(dao) → review-
I see no change in mouse cursor when trying to drag a tab in Windows 2000: 

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1) Gecko/20090806 Namoroka/3.6a1

The action takes place, however. I can dran&drop or detach.
Yes, we should restore this behaviour, but no, it doesn't block. Honestly, the little rectangle is laughable as feedback, but better than nothing, I suppose.
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
Split out the event state manager changes to propagate mozCursor state back to the original data transfer object. 

bz, you already r+ this part of the patch, can you propagate that forward so I can get the fix for mozCursor landed? I can revisit the drag cursor stuff for tab browser independently.
Attachment #389454 - Attachment is obsolete: true
Attachment #404543 - Flags: review?(bzbarsky)
Attachment #404543 - Flags: review?(bzbarsky) → review+
Comment on attachment 404543 [details] [diff] [review]
[checked-in] data transfer patch v.3

http://hg.mozilla.org/mozilla-central/rev/5bbfae9a5c7e
Attachment #404543 - Attachment description: data transfer patch v.3 → [checked-in] data transfer patch v.3
(In reply to comment #37)
> I can revisit the drag cursor stuff for tab browser independently.

Is there a bug filed? 

Using XP, I'm also seeing the flickering behavior:

If I just drag a tab and hold the cursor over another tab or itself without letting go, I also see the cursor flicker off and on.
No longer depends on: 521966
Depends on: 524748
Sorry what is the progress of this bug?

A patch has been given "review+". When will the bug closed?
Assignee: jmathies → nobody
Still no one replied after 1 year.

Would anyone mind telling me why the bug hasn't been closed after a patch has been given "review+"?
(In reply to comment #41)
> Still no one replied after 1 year.
> 
> Would anyone mind telling me why the bug hasn't been closed after a patch has
> been given "review+"?

Technically this bug was never fixed, but we did fix a problem with cursor settings while dragging. The original bug was about changing the cursor while dragging a tab. I think the general consensus was the way things work currently is the behavior we want.

closing, resolved won't fix.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
On Windows7 Classic, There is no visual feedback while dragging a tab due to Bug 591941.
No workaround now.
At least, it is necessary changing the cursor while dragging a tab on Windows7 Classic.
jimm, could you say which parts of comment #5, are relevant for opening new bugs?
(In reply to comment #44)
> jimm, could you say which parts of comment #5, are relevant for opening new
> bugs?

We can file bugs on any/all of these, but I think the appropriate way to address this is not to use cursor changes, we should place visual indicators on the drag image itself.

For classic we might be able to fall back on some sort of cursor change if we have to. That's more of a polish issue for that desktop theme since by default, it doesn't support drag images (according to comment 43).
You need to log in before you can comment on or make changes to this bug.