319244 - Tab drop indicator icon flickers

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

onDragExit isn't very smart about picking when to hide the drop indicator icon - if you move the mouse over the icon, it flickers.  Also, if you drag a tab and move the mouse left of the tab, then right, when it moves to the right the indicator appears briefly at the left of the tab before moving.

Comment 1

[zug_treno]

Related to Firefox bug 315167?

Comment 2

[jag (Peter Annema)]

I believe so.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

That looks like the same problem, and the fix will likely be the same.  Don't dup this bug to that one though (different product, forked code).

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch for one of the flicker issues

By not showing the indicator bar until the position has been changed, one of the flickers will no longer happen.

Comment 5

[jag (Peter Annema)]

Attachment: Only show and hide the indicator when entering/leaving the tab strip or leaving/entering the no-drop zone

The only thing I haven't fixed yet is the flicker you get when you position your mouse cursor over the indicator image.

We should be able to do something similar for Firefox, if they're interested.

Comment 6

[[e-mail]]

Comment on attachment 205271 [details] [diff] [review]
Only show and hide the indicator when entering/leaving the tab strip or leaving/entering the no-drop zone

>+            if (aDragSession.sourceNode.parentNode == this.mTabContainer) {
>+              var newIndex = this.getDropIndex(aEvent);
>+              var tabIndex = this.getTabIndex(aDragSession.sourceNode);
If we do this in onDrop we can get rid of the x-moz-tab flavour!

Comment 7

[[e-mail]]

Comment on attachment 205271 [details] [diff] [review]
Only show and hide the indicator when entering/leaving the tab strip or leaving/entering the no-drop zone

>+            for (var i = 0; i < this.mTabs.length; ++i)
>+              if (this.mTabs[i] == aTab) 
>+                return i;
>+
>+            throw Components.results.NS_ERROR_ILLEGAL_VALUE;
jag, we have JS 1.6 now, use this.mTabs.indexOf(aTab) ;-)

Comment 8

[[e-mail]]

(In reply to comment #7)
>(From update of attachment 205271 [details] [diff] [review])
>>+            for (var i = 0; i < this.mTabs.length; ++i)
>>+              if (this.mTabs[i] == aTab) 
>>+                return i;
>>+
>>+            throw Components.results.NS_ERROR_ILLEGAL_VALUE;
>jag, we have JS 1.6 now, use this.mTabs.indexOf(aTab) ;-)
Whoops, I thought mTabs was an array, sorry :-[

Comment 9

[jag (Peter Annema)]

Would be nice if we could actually do stuff like that. You had my hopes up there for a minute.

And yep, you're right. We've got the sourceNode, no need to actually pass the index number around, I think. New patch tomorrow if I still think that when I'm actually awake.

Any ideas on how to make it so dragging over the indicator image doesn't turn it into the evil <blinkenimg/>? I tried mousethrough="always" but that seems to only affect clicks (perhaps something in need of fixing?).

Comment 10

[jag (Peter Annema)]

Attachment: Previous patch + remove tab index as transfer flavour

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 205340 [details] [diff] [review]
Previous patch + remove tab index as transfer flavour

A couple things I don't like:
1. The area that gives the no-drop indicator is big - two full tab widths.  I don't know if this is a valid complaint, but it would seem to me that you'd have to drag pretty far the first time to realize you can actually do something with the tab once you've started dragging it.  If our UI czar (Neil) is ok with this behavior, I won't r- for it though.

2. You continue showing the drop indicator if you go somewhere you could drop to, and then back over the original tab.  At this point, the indicator shows the tab will move somewhere, but releasing the mouse button doesn't actually drop the tab where the indicator is.  I think we should hide the indicator when you can't drop the tab.

+              if (newIndex != tabIndex) 
+                this.moveTabTo(tabIndex, newIndex);
Is that check still necessary?  You shouldn't be able to drop there any more due to canDrop(), right?

Comment 12

[jag (Peter Annema)]

Attachment: Oops, I hand-edited the patch and messed up, try this one

Comment 13

[jag (Peter Annema)]

When you review this patch, assume I've made this change (like CTho pointed out, no need for that check any longer):

-              if (newIndex != aXferData.data) 
-                this.moveTabTo(aXferData.data, newIndex);
+              this.moveTabTo(tabIndex, newIndex);

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 205530 [details] [diff] [review]
Oops, I hand-edited the patch and messed up, try this one

I still think I prefer showing the drag indicator for a useless drag and allowing the drop (even though it ends up being ignored), but the code looks good and does what jag seems to have meant for it to do ;).

I noticed that the mouse cursor switches from the arrow to the background-busy cursor when the indicator moves, but this actually occurs even without this patch (it's only noticeable in my debug build unless I look very carefully).  We might want to see if we can fix that somehow too (doesn't have to be here though).

Comment 15

[Serge Gautherie (:sgautherie)]

*** Bug 319876 has been marked as a duplicate of this bug. ***

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 205530 [details] [diff] [review]
Oops, I hand-edited the patch and messed up, try this one

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml

>@@ -1151,6 +1148,19 @@
>+      <method name="getTabIndex">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+            for (var i = 0; i < this.mTabs.length; ++i)

Nit: This could be |for (var i = this.mTabs.length; --i >= 0; )|.

>+              if (this.mTabs[i] == aTab) 
>+                return i;

*****

Can bug 309452 attachment 196902 [details] [diff] [review] removal of ondragover and ondragdrop be ported too, or are they still needed in SeaMonkey ?

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 205530 [details] [diff] [review]
Oops, I hand-edited the patch and messed up, try this one

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml

>@@ -1300,9 +1329,19 @@
>         <parameter name="aDragSession"/>
>         <body>
>           <![CDATA[
>+            if (aDragSession.sourceNode.parentNode == this.mTabContainer) {
>+              var target = aEvent.relatedTarget;
>+              while (target) {
>+                if (target == this.mStrip)
>+                  return;
>+
>+                target = target.parentNode;
>+              }

Nit: This could be |for(var target = aEvent.relatedTarget; target; target = target.parentNode;)|

>+              document.getAnonymousElementByAttribute(this, "class",
>+                                                      "tab-drop-indicator-bar")
>+                      .hidden = true;
>+            }

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> (From update of attachment 205530 [details] [diff] [review] [edit])
> I still think I prefer showing the drag indicator for a useless drag and
> allowing the drop (even though it ends up being ignored)

I don't like the useless case: once I knew DnD was working, I wondered why it let me think it would do something, while it would not. But I can understand your point.

In that particular case, would it be too hard to get the cannot-drop mouse icon while still displaying the insert arrow ? (Would it make (more) sense ??)
Or display the/a cannot-drop icon where the insert-arrow should be ? (or may be a cannot-drop arrow icon too !?)

Note: In current (20051207) build, DnD'ing a tab onto its own page "reloads" it ... We may want to block that behaviour too (but that may be another bug !?).

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #16)
> (From update of attachment 205530 [details] [diff] [review] [edit])
> >Index: xpfe/global/resources/content/bindings/tabbrowser.xml
> 
> >@@ -1151,6 +1148,19 @@
> >+      <method name="getTabIndex">
> >+        <parameter name="aTab"/>
> >+        <body>
> >+          <![CDATA[
> >+            for (var i = 0; i < this.mTabs.length; ++i)
> 
> Nit: This could be |for (var i = this.mTabs.length; --i >= 0; )|.
What's the benefit?  Seems more intuitive to me to start at 0 and work up, unless 1) it's an expensive loop and 2) we know users tend to drag tabs to the end.

> Can bug 309452 attachment 196902 [details] [diff] [review] [edit] removal of ondragover and ondragdrop be ported
> too, or are they still needed in SeaMonkey ?
No, Neil saw that it was unnecessary originally when I was working on bug 105885.

(In reply to comment #18)
> Note: In current (20051207) build, DnD'ing a tab onto its own page "reloads" it
> ... We may want to block that behaviour too (but that may be another bug !?).
> 

Why?  If you don't want to do the drag, just drag the tab back to itself... I think it's better to be consistent that dropping any tab to the content area loads that tab's URL.

Comment 20

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

*** Bug 319246 has been marked as a duplicate of this bug. ***

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> > >+            for (var i = 0; i < this.mTabs.length; ++i)
> > 
> > Nit: This could be |for (var i = this.mTabs.length; --i >= 0; )|.
> What's the benefit?  Seems more intuitive to me to start at 0 and work up,
> unless 1) it's an expensive loop

(As you wish; I thought it could be a small resource gain on the average.)

> > Can bug 309452 attachment 196902 [details] [diff] [review] [edit] [edit] removal of ondragover and ondragdrop be ported
> > too, or are they still needed in SeaMonkey ?
> No, Neil saw that it was unnecessary originally when I was working on bug
> 105885.

(I'm not sure if you mean to keep/remove it, but I trust you to have considered it (right), which was my only point.)

> Why?  If you don't want to do the drag, just drag the tab back to itself... I
> think it's better to be consistent that dropping any tab to the content area
> loads that tab's URL.

(As you wish; I thought it might be good to be consistent with the new DnD_moveTab feature (= "not to itself")...)

Comment 22

[jag (Peter Annema)]

Some thoughts... If (for the first time) someone wants to drag a tab somewhere, they'll drag it to the new place they want it, and at that point see the indicator arrow, and know what to look for next time. I don't think they'll drag left or right a little bit, get the no-drop icon, decide that they can't drag tabs and give up. I guess I should find some users to test this theory on.

I'll have to think about showing the indicator while at the same time showing the no-drop icon. I think, while correct if you think of the arrow as "if you are able to drop it, this is where it'll go", the combination is a bit cryptic, and most people will probably see the arrow as "drop it here". On my Linux system (but I suspect this is a Gnome thing) at work the no-drop icon is the same as the drop icon except for a small arrow. Having the extra feedback from not displaying our indicator helps a lot there.

For loops, I prefer idiomatic form if performance is not much of an issue. If need be I'd stuff the length in a var outside the loop, or do |for (var i = foo.length - 1; i >= 0; --i)| which I find more readable than your compressed form.

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> I don't think they'll
> drag left or right a little bit, get the no-drop icon, decide that they can't
> drag tabs and give up. I guess I should find some users to test this theory on.

(I support this statement: furthermore, if they move it a little and see the cannot-drop, they could think "then, there should be somewhere else where they can drop" :->)

> Having the extra feedback from
> not displaying our indicator helps a lot there.

(As I just wrote, no indicator ... or maybe some kind of cannot-drop indicator (as a feature hint) too ;-))

> |for (var i = foo.length - 1; i >= 0; --i)|

(That one would be close enough :-))

Comment 24

[jag (Peter Annema)]

I think doing the "can't drop" feedback twice, once in the drop indicator, once in the mouse cursor, would look ugly.

Neil has the final say in this, though.

Comment 25

[[e-mail]]

Comment on attachment 205530 [details] [diff] [review]
Oops, I hand-edited the patch and messed up, try this one

I'd like to see that while -> for conversion please.

Comment 26

[jag (Peter Annema)]

              var target = aEvent.relatedTarget;
              while (target) {
                if (target == this.mStrip)
                  return;
                                                                                
                target = target.parentNode;
              }

vs.

              for (var target = aEvent.relatedTarget; target;
                   target = target.parentNode) {
                if (target == this.mStrip)
                  return;
              }

vs.

              for (var target = aEvent.relatedTarget;
                   target;
                   target = target.parentNode) {
                if (target == this.mStrip)
                  return;
              }

vs.

              for (var target = aEvent.relatedTarget;
                   target && target != this.mStrip;
                   target = target.parentNode) {
                // nothing to do here
              }

              if (target)
                return;

vs.

              var target = aEvent.relatedTarget;
              while (target && target != this.mStrip)
                target = target.parentNode;

              if (target)
                return;

I'm visually not liking any of the for loops much, but I like the last while loop better than the first one.

Comment 27

[[e-mail]]

Note that the first for loop doesn't need {}s, although I'm not claiming that that 's enough to make it look nicer than the last while loop.

Does if (document.getBindingParent(event.relatedTarget) == this) return; work?

Comment 28

[jag (Peter Annema)]

That seems to work. Every now and then the relatedTarget seems to be for a <toolbar> (bug?), but that's only when I move up really quickly when the indicator isn't showing, in which case we won't have to worry about hiding it.

Has anyone noticed that in the attached patch when you actually drop the tab the indicator stays around? I don't recall seeing this when I was testing my patch, but it makes sense. The only place we hide the bar is when we onDragExit the tab strip, which we don't do when we drop the tab. Easily fixed with another ib.hidden = true right before we move the tab in onDrop.

Comment 29

[jag (Peter Annema)]

Never mind, I'm smoking something. The attached patch is fine, it's my slightly modified patch that I thought I had made functionally equivalent to the old patch that was still exhibiting this problem. When you drop, there is a onDragExit, with relatedTarget == null. I saw another problem when I was using |getBindingParent()|, let me play with it a little bit more :-)

Comment 30

[jag (Peter Annema)]

|getBindingParent()| gives me the tabbrowser, while I really want the tab strip. The suggested comparison by itself means when I move up, over the indicator strip, it doesn't go away, and at this point we're outside the content that gets calls for our onDragExit, so the strip never goes away. I could add an extra check to make sure we didn't just enter the indicator strip, but it's rather fragile. We could move the ondrag stuff up to <tabbrowser/>, but then we need to filter out additional unwanted drag events. We could just check if the relatedTarget is inside the tab strip, but then we're back to the loop that we tried to compress into a function call.

How about that last while loop?

And yeah, the first for loop doesn't technically need {}s, but I put them around statements that span more than one line to make it really clear where the body of the (if|for|while|...) ends.

Comment 31

[jag (Peter Annema)]

Attachment: This is what I checked in

Comment 32

[jag (Peter Annema)]

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.135; previous revision: 1.134
done

Comment 33

[Ian Neal]

Comment on attachment 205817 [details] [diff] [review]
This is what I checked in

Do we want this on the branch too?

Comment 34

[Robert Kaiser]

Comment on attachment 205817 [details] [diff] [review]
This is what I checked in

a=me, makes 2 with IanN :)

Comment 35

[jag (Peter Annema)]

And checked in on the branch.

Comment 36

[Benoît]

-> VERIFIED

Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.8) Gecko/20060104 SeaMonkey/1.0b

Comment 37

[jag (Peter Annema)]

Attachment: Don't flicker while we're over the arrow

Comment 38

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 218595 [details] [diff] [review]
Don't flicker while we're over the arrow

Can you indent the second line to match the "this" above it?  We already exceed 80 characters anyway on other lines, right?

Comment 39

[jag (Peter Annema)]

Attachment: How about this instead?

Comment 40

[[e-mail]]

Comment on attachment 218676 [details] [diff] [review]
How about this instead?

I can drag the mouse over the arrow and out of the strip, then when I release the mouse the arrow never disappears...

Comment 41

[[e-mail]]

Comment on attachment 218676 [details] [diff] [review]
How about this instead?

This is the wrong fix for the trunk, as we can just use mousethrough="always" on the image. We don't get the right drag exit events with this patch anyway.

Comment 42

[jag (Peter Annema)]

I thought I'd tried mousethrough. It only seems to work for mouse clicks, not drag events. Let me look into that again though.

What do you mean by "We don't get the right drag exit events with this patch anyway."?

Comment 43

[jag (Peter Annema)]

Comment on attachment 218676 [details] [diff] [review]
How about this instead?

Bad patch, no cookie.

Comment 44

[jag (Peter Annema)]

Attachment: mousethrough="always"

Hrm, so simple. I could've sworn I'd tried this and it didn't work. It appears to work just fine though.

Comment 45

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 219589 [details] [diff] [review]
mousethrough="always"

r=me

Comment 46

[jag (Peter Annema)]

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.154; previous revision: 1.153
done

Comment 47

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060522 SeaMonkey/1.1a] (nightly) (W98SE)

Now that it's fixed on Trunk,
what's the plan to fix it on the branch(es) ?

Comment 48

[[e-mail]]

(In reply to comment #47)
>Now that it's fixed on Trunk, what's the plan to fix it on the branch(es) ?
The trunk fix relies on other trunk fixes. There's no equivalent branch fix.

Comment 49

[Serge Gautherie (:sgautherie)]

(In reply to comment #48)
> (In reply to comment #47)
> >Now that it's fixed on Trunk, what's the plan to fix it on the branch(es) ?
> The trunk fix relies on other trunk fixes. There's no equivalent branch fix.

That's what I +/- understood from comment 41 to 43...
Then, does it mean these other fixes would need to be ported,
or that this part of the bug is left as WONTFIX for branches ?

Comment 50

[[e-mail]]

(In reply to comment #49)
>this part of the bug is left as WONTFIX for branches ?
Probably, sorry.