Closed Bug 333791 Opened 14 years ago Closed 14 years ago

Tab drag indicator (arrow) vanishes/flickers when cursor passes over parts of the tab

Categories

(Firefox :: Tabbed Browser, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: waynegwoods, Assigned: moco)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 7 obsolete files)

This minor UI bug seems to have existed for as long as the tab drag arrow has, but I can't find a dupe...

To reproduce:
1. open multiple tabs
2. drag one tab, moving the mouse over another tab in the process
3. note that the indicator arrow flickers or vanishes as you pass over very specific points on the tab, but reappears again

On Mac OS X it's quite obvious, but it also exists on Windows XP, despite being far more subtle (the arrow just flickers slightly at specific points, and people probably expect it's just part of the movement).

It seems to be associated with the borders between buttons, text and icons on the tab... as if each item has an invisible box drawn around it and touching that box causes the tab drag indicator to fail. While dragging a tab, move the mouse around the borders of a tab close button, the tab title, or the site icon. You should notice that the arrow appears normal while directly over the widgets or text, but flickers when passing across a border in any direction.
Do we know this bug is XP? I see it on Mac OS X, firefox 1.5. 

* The arrow flickers especially when you're dragging over the favicon and the close button.
* The arrow is *not shown at all* sometimes when you're dragging the frontmost tab over itself.
* The arrow to the left of the leftmost tab is not shown, because it shows outside the browser window's area.

I can't imagine that none of these cases haven't already been filed somewhere in bugzilla though... Our drag behavior is extremely buggy indeed.
It definitely occurs in XP. It flickers at the same places, but it's not quite as pronounced... On Mac, I can find spots where I can hover the cursor and the arrow will *remain* vanished until I move the mouse again. On XP, I can't seem to find spots where I can keep it hidden... it flickers and comes back.

I'm surprised nobody's reported it before, too. Perhaps there's a dupe I didn't find with the keywords I used. It's not a serious bug, but it certainly makes dragging look a bit cheap and nasty ;) Any idea what might cause it?
Related to bug 315167?
(In reply to comment #3)
> Related to bug 315167?

Definitely.  Perhaps we need a meta-bug for all of these tab drag&drop issues?
I presume some sort of event is triggered whenever the mouse cursor leaves one interface element, and a different event is triggered when it enters the next, say from the tab close button to the tab background. I am also guessing that the drag indicator was designed to vanish at the first trigger, with the assumption that it was signalling the cursor leaving the tab bar, when it really isn't. So it disappears, only to reappear when triggered by the new element. If that's true, then hopefully one could just check whether the new element being hovered over is also part of the tab bar *before* vanishing the arrow. Of course that's a lot of guesses :)
See bug 319244 for the SeaMonkey patches that address this.
Regression bug 325455 needs to be fixed before we can even see the indicator, and verify whether the seamonkey patch can fix this.
Depends on: 325455
Assignee: nobody → mconnor
Flags: blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
Depends on: 319244
(In reply to comment #7)
> Regression bug 325455 needs to be fixed before we can even see the indicator,
> and verify whether the seamonkey patch can fix this.
> 

Bug 325455 is actually Mac-only. This bug is on Windows too, so it can still be worked on without the other bug being fixed first.
load-balancing tabbrowser stuff to sspitzer
Assignee: mconnor → sspitzer
gah.  s/.org/.com/!
Assignee: sspitzer → sspitzer
Wayne: you are mostly correct, sir. (I doubt my previous comment conveyed that as succinctly ;-) )

We were seeing the same flicker in SeaMonkey. |onDragExit| always hides the arrow, and when you move from tab to tab a |dragexit| is triggered, followed by a |dragover| which shows the arrow |if !canDrop etc.|. The fix is to make |onDragExit| hide the arrow only when you exit the tab strip, and make |onDragOver| hide the arrow |if !canDrop etc.| (in addition to showing it |if canDrop etc.|).

(You can't just make |onDragExit| only hide the arrow when |event.relatedTarget| lies outside the tab strip, since the old code depends on the arrow always being hidden for the |!canDrop| case in |onDragOver|).

See attachment 205817 [details] [diff] [review].
Thanks for that! I've adapted your patch to modify a Firefox version of tabbrowser.xml. Some properties etc. have different names in the Firefox version, and there seems to be different ways of doing certain things. I also omitted to add the "canDrop" method, as that seems to do nothing except hide the indicator if it's adjacent to the tab being dragged (I think it's less-confusing if it appears). Was it supposed to do any more than that? I also omitted the "getTabIndex" method, as it wasn't as handy in the Firefox patch.

Anyway, so far the patch I'm working on has fixed the flickering problem. At least on Mac... I haven't tested on Windows or Linux. It also fixes the remaining symptoms of (Mac-only) bug 325455.

The only problem with it that I can see is that the indicator arrow refuses to hide if I drag over the web page... it just sticks in the position where I left it. I assume it's not doing that in SeaMonkey? I can't figure out what's different or wrong yet.

Cheers
OK, so I completely bollocksed the first patch, which was, amongst other things, the reverse of what it should have been.

The new one will hopefully make sense.

I'm still not sure why the arrow remains if you drag over the content window. It vanishes if you move outside the tab strip in other directions. Might aDragSession.canDrop return true for this in Firefox?
Attachment #223593 - Attachment is obsolete: true
wayne, this seems like a dup of #315167, which has some addition info about this issue.
(In reply to comment #14)
> wayne, this seems like a dup of #315167, which has some addition info about
> this issue.
Thanks Seth. I've seen that bug. Forgive me if I'm wrong, but isn't that bug just that if you mouse over the arrow, then it behaves weirdly, and is solved with a simple |mousethrough="always"|? I got the impression that wouldn't fix this bug, which seems to stem from onDragExit hiding the arrow when it shouldn't. For SeaMonkey, at least, the fixes for the two problems seemed to be unrelated. Cheers.

estimate 1 day to figure out wayne's patch, see what is left to do.

thanks to wayne for doing the lion's share of the work already!
Status: NEW → ASSIGNED
Whiteboard: [SWAG: 1d]
(In reply to comment #16)
> thanks to wayne for doing the lion's share of the work already!

It's Peter/Jag's work.. I just changed some element names and such :)

You might want to look directly at his patch (attachment 205817 [details] [diff] [review]) at the same time.  Some methods may behave differently in Firefox, and so a direct translation of the patch may not have been the right solution.

Also, I haven't tested my patch in Windows. There's a chance it works properly in that environment, and the remaining problems I'm seeing are actually related to the Mac bug 325455.
The problem seems to be with this code in |onDragExit|:
  var target = aEvent.relatedTarget;
  while (target && target != this.mStrip)
    target = target.parentNode;

Where "mStrip" is defined as document.getAnonymousElementByAttribute(this, "anonid", "strip");

If you drag the tab into another tab bar element, the while loop iterates about 5 times and |target| is eventually |this.mStrip|. As a result, the code returns later on and the arrow is left.

If you drag the tab into another bar (bookmarks toolbar or location bar, for example), then the loop iterates about 7 times, but |target| never becomes|this.mStrip|. It eventually becomes 0, and so the arrow is hidden later in the code. Something similar happens if you drag it outside the browser window entirely.

The problem is that if you drag the tab into the content frame (page contents or whatever you call it), it behaves exactly as if you dragged it to another tab bar element, i.e. the while loop iterates about 5 times and |target| eventually becomes |this.mStrip|. So the code seems to think the mouse cursor is still in the tab strip. Is this supposed to be the case? If so, is there any other property we could check for that would differentiate the contents from the tab bar?
I know what I'm about to describe isn't directly this bug, but it's something that I think is blocking the patch from working correctly...

It looks like the xul dragexit event isn't being triggered *at all* when the drag enters the content window, so there's no call to the code that hides the drag indicator.

I added debug messages to onDragExit in tabbrowser.xml and dragExit in nsDragAndDrop.js. Just to be sure that some weird timing thing wasn't allowing the browser to hijack the ondragexit event listener, I also added debug messages to the various dragExit-like functions in browser.js and browser-places.js. None are triggered when the drag enters the content window. The last target for the ondragexit is the thin strip at the bottom of the tab bar. Also, I'm pretty certain that the same is true if you try to drag a bookmark toolbar button over the content window (with or without a tab bar in between). Maybe it's true for dragging any widget over the content window.

Can anyone confirm this is the case with Windows or Linux? Maybe it's just a Mac bug. A simple test would be to apply my patch and see if the drag indicator still vanishes when you drag over the content window, or if it now stays where you left it. If the latter, then the bug is the same.

Maybe it's a more basic bug with xul?
Blocks: 318181
> Can anyone confirm this is the case with Windows or Linux? Maybe it's just a Mac bug.

I can look into what happens on Windows.  

An additional data point about the mac, see bug 318168 comment 43.

I'm seeing differences between mac and windows and when ondragenter is called.
> It looks like the xul dragexit event isn't being triggered *at all* when the
> drag enters the content window, so there's no call to the code that hides the
> drag indicator.

wayne, I see this on the mac, too.  See also bug #341100.
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
Attached patch Proposed fix (obsolete) — Splinter Review
Reduced patch - only the onDragExit changes are required to fix this bug. Other code in the SeaMonkey patch was about indicator behaviour in circumstances which aren't pertinent to this bug anyway (plus I happen to prefer it as it is).

It'd be really great to get this in soon: On Mac it combines with bug 325455 to make the indicator all but useless. At least fixing this bug will make working on the other bug easier!

Couple of questions:

- Who should I set as reviewer (if you're happy for it to go to review, Seth)?

- Is there no easier way to determine if one node is a descendent of another except by iteration of parentNode (as the patch does)?

Thanks :)
Attachment #223921 - Attachment is obsolete: true
Seth, can you test/review the patch here, and I'll rs= if it's good to go?

Wayne, thanks for splitting this out!
Whiteboard: [SWAG: 1d] → [SWAG: 1d] [at risk]
> Seth, can you test/review the patch here, and I'll rs= if it's good to go?

I'll go test and review it now, and report back.

Thanks for the patch Wayne, and sorry for the delay.
> I'll go test and review it now, and report back.

this patch fixes this bug, but I think I've noticed an issue with it, but I'm not 100% sure (yet) if it is cased by this patch.

the issue I'm seeing (on mac, haven't tested windows) is that the drop arrow can stay around after I stop dragging.

looking into it...
note, my build has mark's fix for #342361 (Drop indicator remains visible after a tab is dragged into the content area)
Your last patch should fix the flicker going from tab to tab.

Our canDrop code makes it so no-op drags (dragging a tab right before, on top of, or after itself) give the no-drop feedback and (in onDragOver) hide the arrow as a hint that nothing will happen. So yes, if you like your current behaviour, then the canDrop and additional dragging="false" in onDragOver aren't needed.
>this patch fixes this bug, but I think I've noticed an issue with it, but I'm
>not 100% sure (yet) if it is cased by this patch.
>
>the issue I'm seeing (on mac, haven't tested windows) is that the drop arrow
>can stay around after I stop dragging.
>
>looking into it...

I'm not seeing this problem on windows.  I think I'm getting a ondragexit on windows in a scenario where I'm not on mac, I'm double checking to make sure.
Thanks for looking at this. Seth, I presume the arrow-sticking problem is only when you drop over the active tab (i.e. dropping a tab on itself)? I now see this problem as well.

onDragExit IS being called in that situation. The problem, however, it's breaking on the first line:
    if (aDragSession.sourceNode.parentNode == this.mTabContainer)
because |aDragSession.sourceNode| is null. So it never executes any hiding code.

That's simple enough to fix. I can add a check for it, and also move the |'dragging','false'| call to the very end of the method, so it's called in more situations.

However, if you report that it IS working on Windows, that suggests a lack of parity. Can you confirm in that situation that |aDragSession.sourceNode| has a value in that situation on Windows? If so, I wonder what the difference is. I'm afraid I can't compile on Windows to test this myself.

This would never have appeared on SeaMonkey, as Jag's other code hides the indicator when the tab is approaching its original position. On second thoughts, while I like the arrow to still appear when the drag is over adjacent tabs (even though its pointing back to the tab's original position), I think it should be hidden when the drag is directly over the active/original tab. This is easy to do and would alleviate this problem as well. But it still doesn't explain the lack of platform parity...

Cheers
sorry for the delay.

> Thanks for looking at this. 

Thank you for coming up with a patch.

> Seth, I presume the arrow-sticking problem is only
> when you drop over the active tab (i.e. dropping a tab on itself)? I now see
> this problem as well.

yes, that is the scenario where I see it.

> onDragExit IS being called in that situation. The problem, however, it's
> breaking on the first line:
>    if (aDragSession.sourceNode.parentNode == this.mTabContainer)
>because |aDragSession.sourceNode| is null. So it never executes any hiding
>code.

You're right.  it is being called, but sourceNode is null.

> That's simple enough to fix. I can add a check for it, and also move the
>|'dragging','false'| call to the very end of the method, so it's called in 
> more situations.

I like this suggestion.  Let me attach a patch that does this.

> However, if you report that it IS working on Windows, that suggests a lack of
> parity. Can you confirm in that situation that |aDragSession.sourceNode| 
> has a value in that situation on Windows? 
> If so, I wonder what the difference is. I'm
> afraid I can't compile on Windows to test this myself.

I'll check win32 and report back.  but I think that on win32, I am not able to drop the tab on itsself.  so perhaps the parity is in the "canDrop" code.

>This would never have appeared on SeaMonkey, as Jag's other code hides the
>indicator when the tab is approaching its original position. On second
>thoughts, while I like the arrow to still appear when the drag is over 
>adjacent
>tabs (even though its pointing back to the tab's original position), I think it
>should be hidden when the drag is directly over the active/original tab. This
>is easy to do and would alleviate this problem as well. 

I think that is what win32 appears to do, but I'll double check (once my build is ready.)

> But it still doesn't explain the lack of platform parity...

true.  once we figure that out, and if we determine it is a bug on the mac side, we can ping mento.

new patch coming...
update:

1)  windows is calling onDragExit
2)  aDragSession.sourceNode is not null on win32

I'll attach a patch for this bug, based on wayne's patch and suggestions, and then spin off a new bug about the platform parity.
Attachment #229092 - Attachment is obsolete: true
Attachment #230051 - Flags: review?(bugs.mano)
Attachment #230051 - Flags: review?(w.woods)
Attachment #230050 - Attachment is obsolete: true
Attachment #230050 - Flags: review?
Attachment #230051 - Flags: superreview?(mconnor)
Whiteboard: [SWAG: 1d] [at risk] → [fix in hand, awaiting reviews]
> I'll attach a patch for this bug, based on wayne's patch and suggestions, and
> then spin off a new bug about the platform parity.

see bug #345425 for the dnd platform parity issue.
No longer blocks: 345425
(In reply to comment #34)
> Created an attachment (id=230051) [edit]
> patch based on wayne's fix and his comments

That patch is identical to what I was just trying out as well, so I know it works.

It would be nice to also include the code for hiding the indicator when it's over the original tab, but I'm still tinkering with that (it's easy enough to do for the most part, but the arrow still appears briefly if you drag over the tab "guttering" under the original tab, and I'm just trying to get rid of that too). Plus that's not really what this bug was about, so I'm happy to spin off a new bug for it if it's not done in time.
> That patch is identical to what I was just trying out as well, so I know it
> works.

thanks wayne.  (can you mark the patch as r=wayne?)

> It would be nice to also include the code for hiding the indicator when it's
> over the original tab, but I'm still tinkering with that (it's easy enough to
> do for the most part, but the arrow still appears briefly if you drag over the
> tab "guttering" under the original tab, and I'm just trying to get rid of that
> too). Plus that's not really what this bug was about, so I'm happy to spin off
> a new bug for it if it's not done in time.

yes, please spin that off to a new bug (and you might want to seek an ui review from beltzner.)
Attachment #230051 - Flags: review?(w.woods) → review+
I completed the code for hiding the indicator when you drag over (or just under) the position of the active tab. Here 'tis, bound up with the code from the previous patch, in case you're happy to check it in at the same time. I haven't superseded the previous patch, in case you don't want this patch checked in here.

Essentially, canDrop (the original, inherited version) already returns false when dragging over the active tab, so simply adding a check for that in onDragExit fixes 90% of the problem.

However, it's unfortunately not enough. Even with that fix, if you have the drag indicator showing, and you drag VERY rapidly from an adjacent tab to the active tab, then sometimes the indicator fails to vanish. I assume this is due to the same onDragExit skip that I noted in bug 342361 comment 12, which Mark suggests is normal (bug 342361 comment 16). At least it does that on Mac OS X... haven't tested on anythign else. Anyway, as a result, I also had to stick a check in onDragOver, which is similar to what Jag did for SeaMonkey.

Finally, even though the indicator vanishes when dragging over the active tab, it still appears if you drag onto the thin band of tab "guttering" underneath that tab, which can make it flicker if you cross that area while dragging. So to get rid of that, I added a canDrop method to the file. All it does is check the current screenX value of the drag and determine if it falls between the screenX boundaries of the active tab. If so, the drag must be just under the active tab, and so it should not be possible to drop.

Hope that all makes sense. I've tried to break it and can't find a way :)
Attachment #230116 - Flags: review?(sspitzer)
Comment on attachment 230116 [details] [diff] [review]
Patch with addded code to hide the arrow when dragging on/under the active tab

>+      <method name="canDrop">
>+        <parameter name="aEvent"/>
>+        <parameter name="aDragSession"/>
>+        <body>
>+          <![CDATA[
>+            if (aDragSession.sourceNode.parentNode == this.mTabContainer)
>+              if (aEvent.screenX >= aDragSession.sourceNode.boxObject.screenX &&
>+                  aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
>+                                     aDragSession.sourceNode.boxObject.width))
>+                return false;
>+            return true;
>+          ]]>
>+        </body>
>+      </method>

Why nest the if statements?  Just do this.

           if (aDragSession.sourceNode.parentNode == this.mTabContainer && 
               aEvent.screenX >= aDragSession.sourceNode.boxObject.screenX &&
               aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
                                  aDragSession.sourceNode.boxObject.width))
             return false;

           return true;

This looks good.  Seth, can you test this and land it, it should work fine but I don't have time right now to verify.
Attachment #230116 - Flags: review?(sspitzer) → review+
Attachment #230051 - Attachment is obsolete: true
Attachment #230051 - Flags: superreview?(mconnor)
Attachment #230051 - Flags: review?(bugs.mano)
> Seth, can you test this and land it, it should work fine but
> I don't have time right now to verify.

yes, I'll test and drive it in.
wayne, thanks for that addition patch.  the one issue I saw was this:  the drop indicator were still drawn if I was in the neighboring tabs and dropping would still not result in a tab reorder.

I've got a new canDrop() that leverages getNewIndex() which uses the midpoint calculation.  I like relying on the existing getNewIndex().

with this patch, you will only see the indicator if dropping will actually move the patch.

I'll go test on mac.

wayne/mconnor:  what do you think?
Comment on attachment 230157 [details] [diff] [review]
new patch, based on wayne's last patch

seeking asaf's review as well.
Attachment #230157 - Flags: review?(bugs.mano)
(In reply to comment #42)
> wayne, thanks for that addition patch.  the one issue I saw was this:  the drop
> indicator were still drawn if I was in the neighboring tabs and dropping would
> still not result in a tab reorder.

That's intentional behaviour, actually.  Pretty sure the bug that asked to suppress the dropmarker in this case was wontfixed.  See any other dnd impl (bookmarks, windows start, etc) and it will indicate where the drop will be, even if nothing will change.
> That's intentional behaviour, actually.  Pretty sure the bug that asked to
> suppress the dropmarker in this case was wontfixed.  See any other dnd impl
> (bookmarks, windows start, etc) and it will indicate where the drop will be,
> even if nothing will change.

thanks for the info.  

I'll attach a new patch, based on wayne's v 3 of the patch, without the nested if statements.  I've already tested that, and it works as expected.
this has r=sspitzer, sr=mconnor (I've just removed the nested if)
Attachment #230116 - Attachment is obsolete: true
Attachment #230157 - Attachment is obsolete: true
Attachment #230187 - Flags: superreview+
Attachment #230187 - Flags: review+
Attachment #230157 - Flags: review?(mconnor)
Attachment #230157 - Flags: review?(bugs.mano)
fixed on trunk.

thanks again, wayne!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [fix in hand, awaiting reviews] → [fix landed on trunk]
Comment on attachment 230187 [details] [diff] [review]
Patch with addded code to hide the arrow when dragging on/under the active tab (v2)

seeking approval
Attachment #230187 - Flags: approval1.8.1?
(In reply to comment #44)
> That's intentional behaviour, actually.  Pretty sure the bug that asked to
> suppress the dropmarker in this case was wontfixed.

Thanks for that! I wasn't actually aware of that wontfixed bug, but I deliberately wanted the marker to still appear in adjacent tabs... it's just easier to follow as a user. Otherwise I would have followed more closely the SeaMonkey patch, which I believe did hide it in that case. Thanks for fixing my the nested ifs and glad it's in. Even if most of it was adapted from someone else's code, I'm just happy to finally have a patch in Firefox :)
Depends on: 345425
Comment on attachment 230187 [details] [diff] [review]
Patch with addded code to hide the arrow when dragging on/under the active tab (v2)

a=mconnor on behalf of drivers
Attachment #230187 - Flags: approval1.8.1? → approval1.8.1+
fixed on branch.

thanks again, wayne!
Keywords: fixed1.8.1
Whiteboard: [fix landed on trunk]
No worries. Now that bug 345425 is fixed, is it worth removing the extra aDragSession.sourceNode check?
> No worries. Now that bug 345425 is fixed, is it worth removing the extra
> aDragSession.sourceNode check?

yes.  but let's log a new bug on that code cleanup.

(note, I don't think that sort of fix would not block FF 2.0)

(In reply to comment #53)
> > No worries. Now that bug 345425 is fixed, is it worth removing the extra
> > aDragSession.sourceNode check? 
> yes.  but let's log a new bug on that code cleanup.

Done. Bug 346082.
see bug #346172 for a regression.

we are missing a check in canDrop() for aDragSession.sourceNode.
In the process of checking this, I discovered two things about the DnD indicator on the Mac that I thought were weird. I'm pretty sure they're not due to the fix here... probably much older... but I was hoping you guys could tell me if you think I'm seeing a real bug or not (If so, I'll open a new one). I also can't test if the same thing happens on Windows:
- In onDragOver, if I make any reference to ind.boxObject.x, then the indicator arrow refuses to be drawn at all, ever. ind.boxObject.x returns the correct value (I can print it out in a dump()), however, so it's not like it's breaking.  I can't see any reason why this would be intentional behaviour, or what would be blocking the arrow from drawing...
- Also in onDragOver: normally |ib.setAttribute('dragging','true')| is called early in the method. If, however, I move it to the end (after the margin has been set) and then drag the tab around, then half the time the arrow is drawn where it's supposed to be, and the other half it's drawn to the far left, just before the first tab. This behaviour is erratic, so I can only assume this is also a bug.
fyi, the canDrop() from tabbrowser.xml which was added for the "hide the arrow when dragging on/under the active tab" part of this fix is the cause of a regression.  

see bug #346314 for details.
You need to log in before you can comment on or make changes to this bug.