Closed Bug 338302 Opened 18 years ago Closed 17 years ago

Tab drag and drop indicator (arrow) only works after first drag of a tab

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3 alpha7

People

(Reporter: u88484, Assigned: Gavin)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

patch
4.52 KB, patch
mconnor
: review+
Details | Diff | Splinter Review 
1) Open Firefox, open a new tab or two, drag a tab around the bar - no drop arrow indicator
2) drop that tab and try again - now you have a drop arrow indicator

The drag and drop arrow indicator only shows up after first tab drop.
This is happening on Mac as well. Any idea when this regression occurred? Unfortunately the tab drag indicator was broken on Mac OS X up till the 20060510 build, so I can't test this on older builds.
OS: Windows XP → All
Hardware: PC → All
Summary: Tab drag and drop indicator only works after first drop of a tab → Tab drag and drop indicator (arrow) only works after first drag of a tab
2006-05-10-02 trunk works
2006-05-10-13 trunk broken

bug 326273 is pretty much the only check in during that time, can anyone confirm the regression range?
CCin'g sspitzer since I just seen he fixed a lot of bugs dealing with drag and drop. 

Will try to reproduce this after tomorrows build is release to see if it was someone fixed in the improve tab browsing patch.
(In reply to comment #2)
> 2006-05-10-02 trunk works
> 2006-05-10-13 trunk broken
> 
> bug 326273 is pretty much the only check in during that time, can anyone
> confirm the regression range?

I can now confirm that regression range. So it was definitely the nsIThreadManager patch you mentioned in bug 326273. Funny... that check-in actually caused the tab dnd indicator to start working again in Mac OS X (except for the first use, as per this bug).

is this trunk only?  since we are pointing the finger at darin's change, cc'd him.
-> me, but i'd appreciate any help tracking this down
Assignee: nobody → darin
Target Milestone: --- → Firefox 3 alpha1
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P1
> is this trunk only?

I'm seeing it on trunk only (and on Windows XP)
(In reply to comment #6)
> -> me, but i'd appreciate any help tracking this down

Even when the indicator isn't being shown, I can say that onDragOver ( toolkit/ content/ widgets/ tabbrowser.xml ) is still being called correctly. In it, the tab-drop-indicator-bar 'dragging' property is still being set to true, and the tab-drop-indicator is being set to the correct marginLeft.

I assume (only guessing) the arrow is supposed to be shown or hidden by setting 'dragging' on the indicator bar, which seems to be handled by the theme code, where 'display' is set to -moz-box ( e.g. http://lxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/global/browser.css#186 ). At least that's the only reference to it that I could find.
darin / wayne, another data point, this one on windows.

For bug #343019, on trunk then "underflow" event handler in scrollbox.xml doesn't get called for the first "underflow" event, whereas on the branch, it does.

the underflow in event handler in scrollbox.xml will start getting called, but only after I manual scroll the scrollbox.

this may aid in tracking down the problem on the trunk.  to experience the bug, I don't think you need my patch in bug #343019, but you do need a recent trunk build (with tabs in a arrowscrollbox) and you need to open just enough tabs so that you get the scrollbox arrows to appear, and then make the window "wider" so that underflow occurs.
So if this bug is analogous, what could the event be in this situation? I know the correct tab drag code is still being called. So some sort of painting event?
Attached file testcase (obsolete) — 
So this bug happens for me with this testcase when when I have it opened for the first time in a browser window.
I wonder if there is some relation to bug 203573.
Keywords: testcase
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/2007022704 Minefield/3.0a3pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070227 SeaMonkey/1.5a] (nightly) (W2Ksp4)

The arrow/indicator bug is still there:
confirming with SeaMonkey too.

But the testcase does not seem to apply (anymore ?):
I get the green color immediately. (Martijn, could you check that ?)
Flags: blocking-firefox3?
I'm still seeing the bug with my testcase in current trunk build of Firefox (windowsxp).
Remember, you only see the bug with the testcase the first time you open it in an unique browser window.
Yes, I tried again with new (FF/SM) profiles:
as soon as I have dragged a few pixels, I get the "forbidden" indicator and the background text color does change to green.

Maybe a different behaviour of (our) WXP vs W2K ?
Could someone else give it a try and/or maybe on other OSes too ?
Strange, my experiences agree with Serge. I can't reproduce the testcase bug in the latest Fx trunk builds for either Win XP or Mac OS X. I always quit the browser and restart with a freshly-loaded page in between testing, and the link turns green every time, including the first. I wonder what's different?

I did some testing with old Mac builds and discovered that the testcase stopped bugging between the 20061024-04 and 20061025-(unsure which hour, either 04 or 17) nightlies. Nothing in Bonsai jumps out at my untrained eye.

However, when I tried out your testcase in the buggy builds, I discovered that the link background *never* turned green, no matter how many times I tried to drag it. So maybe we're seeing different bugs with a similar result?
Well, I'm with Martijn on this one. I can still reproduce no indicator the first time and the testcase doesn't turn green on a drag until I've gotten the indicator arrow to show.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070224 Firefox/3.0a3pre
Maybe Mac builds aren't a good comparison with Windows ones for the regression time(s). The reported regression time for the described bug in Windows is 20060510 (comment 2). But on Mac builds, the regression time for the *testcase* bug is much earlier - between 20051007 and 20051008, which corresponds with a different Mac drag problem (bug 325455). On Mac, the testcase bug then followed the described symptoms until 20060926 or so. By 20060930, the green background stopped appearing altogether, until it was fixed entirely between the times mentioned in comment 15.

In Windows, does the testcase bug begin on 20060510?
(In reply to comment #16)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070224
> Firefox/3.0a3pre

I'm wondering why yours is not branded "Minefield" ?

FWIW, I'm testing from Zip nightlies.
Because I do my own builds
This might sound silly, but have you tried reproducing with the official nightlies?
(Not silly: my point, exactly ... Interesting to know, whatever the result.)
Interestingly, the link turns green (is also does on my own builds), but the indicator still doesn't show until the second time.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070228 Minefield/3.0a3pre
I've experimented with the testcase on Windows XP now, and can't actually get a reliable bug out of it:

I discovered that if you click the link at the very bottom and drag quickly down (or at the top and drag quickly up), that the background doesn't turn green, but it seems that's the case in *every* build I tried, even going back into 2005 (earlier than the Mac regression dates in comment 17). So maybe it always behaved that way.

But if you click on the link and drag back over the link as you drag, then the background seems to always turn green, in older and newer builds.

So it seems the testcase symptoms have been this way for a long time. I certainly can't see differences in its behaviour within the regression dates of the described bug. Can anyone else confirm this?
Yes, I can reproduce the behavior you note on both Windows and FC6.

Also, I just confirmed with the current trunk that indeed, this bug is fixed there as well. I guess it is Windows-only at this point.
Wow, I somehow totally left off the most important part of that last sentence. I confirmed *on FC6* that the indicator always shows now.
(In reply to comment #24)
> Also, I just confirmed with the current trunk that indeed, this bug is fixed
> there as well. I guess it is Windows-only at this point.

In case there's any misunderstanding, it's only the testcase which seems fixed on Mac OS X. The actual described bug here is still a problem (unless it's magically fixed in the latest nightly!)


Wayne,

(In reply to comment #23)
> I've experimented with the testcase on Windows XP now, and can't actually get a
> reliable bug out of it:
> {...}

Let me try to make it "reliable": (and I confirm all this on my SM-070228/W2K)
*Hovering the link, you get the color: this bug, or rather lack of (any) bug.
*A few pixel movement needed before entering drag mode: this may be a D&D feature !?
*Not getting the color "as" the drag mode activates while not over the link: this would seem a bug to me, yet a different (to be filled.!?.) one, about targeting/handling the 'dragenter' event probably. (Or is that the intended behaviour of that event ?)

***

Ryan,

(In reply to comment #22)
> Interestingly, the link turns green (is also does on my own builds)

This would seem to contradict your comment 16.
Could you clarify how your own build does behave ?

(In reply to comment #24)
> Yes, I can reproduce the behavior you note on both Windows and FC6.

Which Windows ?

> Also, I just confirmed with the current trunk that indeed, this bug is fixed
> there as well. I guess it is Windows-only at this point.

Is not this bug on MacOSX too ?

(In reply to comment #25)
> Wow, I somehow totally left off the most important part of that last sentence.
> I confirmed *on FC6* that the indicator always shows now.

Interesting: would you have a(ny) timeframe for when the indicator bug was "solved" on Linux ?
Currently the visibility of the dnd indicator is changed by toggling the "display" styling. If the "visibility" styling is toggled instead, then
(a) the indicator displays on the very first drag, and:
(b) it also gets rid of another issue where if you drag away from the tab bar and then back on at a different point, the indicator briefly appears in its original position before updating to the new one, creating a "flicker"

I've only tested this in Pinstripe. I assume it would also work for Winstripe.

I know it doesn't address the underlying issues with whatever is stopping the image from being displayed when "display" is toggled. But given that it also fixes (b), might it be a better long term solution anyway?
Attachment #246022 - Attachment is obsolete: true
my guess is that ind.boxObject.width is 0, because the style sheet isn't synced yet with the updated "dragging" attribute:

> 1775               var ib = this.mTabDropIndicatorBar;
> 1776               var ind = ib.firstChild;
> 1777               ib.setAttribute('dragging',
> 1778                  aDragSession.canDrop ? 'true' : 'false');
> 1779 
> 1780               var tabStripBoxObject = tabStrip.scrollBoxObject;
> 1781               var halfIndWidth = Math.floor((ind.boxObject.width + 1) / 2);

In that case, you could also show/remove the indicator directly in the script (i.e. set the "hidden" property) instead of using "visibility" in the style sheet.
... on the other hand, ind.boxObject.width shouldn't really matter for the whole calculation; the indicator should only be misaligned slightly.
Keywords: testcase
I'm pretty sure that ind.boxObject.width and ind.boxObject.x are both normal when the indicator is supposed to appear, even though it's not being drawn. So I don't think it's that, at all.
(In reply to comment #29)
> In that case, you could also show/remove the indicator directly in the script
> (i.e. set the "hidden" property) instead of using "visibility" in the style
> sheet.

I considered this, but I wondered if whoever created the 'dragging' attribute wanted it for other purposes as well. Maybe not.
Severity: major → normal
Flags: blocking-firefox3? → blocking-firefox3+
I totally hope this isn't a useless comment as I think it might be but could it be that maybe the icon isn't preloaded in memory on startup like other icons?

If you visit about:cache and check the memory cache, I see some of the icons there and they look core related at times w/ chrome:// & resource:// beginnings. I used the CacheViewer extension to look @ them; best to clear your cache if looking for memory icons. Very funny I was not able to view them thru about:cache itself.

So maybe as someone said before, it's trying to be painted? Instead, why don't we just preload it @ startup so its ready to be called?

Sorry if this has already been mentioned & tried.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
When moving items on the bookmarks toolbar (since Places landed) I can see the same effect: The drag and drop indicator only shows up after the first bookmark drop.

Is the problem's root the same or should I file a new bug?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070521 Minefield/3.0a5pre ID:2007052104 [cairo]
If you believe the cause is:
*bug 326273 too, it seems enough to leave it here for the time being;
*some "Places landing", then do file a separate bug.
Well, before Places was enabled on trunk (I didn't use places-experimental builds) there was no indicator at all.

I think for now I'll not file a new bug because the problem is identical and I don't think the places-team re-invented the wheel for their indicator.

I guess the fix for this bug will show wether the problems are related to each other or not.
I haven't tried it yet, but I'm pretty sure that adapting the logic from my patch (attachment 258162 [details] [diff] [review]) to the bookmarks toolbar dnd indicator, i.e. around:
http://mxr.mozilla.org/seamonkey/source/browser/themes/pinstripe/browser/browser.css#1246
would fix that problem, too.

I just want to know if it's the right approach before proceeding. It doesn't actually fix any underlying bug with whatever is wrong with toggling 'display'. it just avoids the issue by using visibility instead.
Assignee: darin.moz → gavin.sharp
Status: ASSIGNED → NEW
Target Milestone: Firefox 3 alpha1 → Firefox 3 beta1
Blocks: 381699
about noah's comment #33, if I load chrome://browser/skin/places/toolbarDropMarker.png in the url bar before I attempt a dnd, I don't get this bug.
Same with chrome://global/skin/tabDragDrop/tabDragIndicator.png for tab dnd.

So if the image is cached already, display can be toggled fine. And if it's set by default to display: -moz-box (but isn't initially visible), then it's also fine.

Is there something in the initial caching that interrupts the display?
My best guess is that the drag-and-drop operation is somehow interfering with the image load operation but as I don't know how either works that's not very helpful :-\
Wayne's patch looks like the best approach to me. Using "hidden" doesn't solve the problem, it boils down to the same thing (display: none). "collapsed" (visibility: collapsed) does, but in my testing that doesn't solve the flicker problem because the element's width when it's first displayed is 0 (because it's collapsed). Using visibility: hidden means that we'll load the image immediately, avoiding this bug, and the boxObject will always represent the "real" width, avoiding the flicker.

This also fixes some indentation in tabbrowser.xml::onDragOver that I noticed while debugging this bug, hence the diff -w.
Attachment #258162 - Attachment is obsolete: true
Attachment #273125 - Flags: review?(mano)
Status: NEW → ASSIGNED
Priority: P1 → P2
Whiteboard: [patch-r?]
Comment on attachment 273125 [details] [diff] [review]
use visibility instead of display

r=mano assuming the Index: toolkit/themes/pinstripe/global/textbox.css is incidentally included.
Attachment #273125 - Flags: review?(mano) → review+
Wouldn't be worth fixing the bookmarks toolbar dnd indicator in this bug too?
Comment on attachment 273125 [details] [diff] [review]
use visibility instead of display

>RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/global/browser.css,v
.
.
.
> .tab-drop-indicator-bar[dragging="true"] {
>-    display: -moz-box;
>+    visibility: hidden;
> }

Shouldn't this be |visible|, not |hidden|?
Attached patch fix for bookmarks toolbar dnd (obsolete) — — Splinter Review 
Gavin, this will likewise fix the dnd indicator for the bookmarks toolbar, in case you want to incorporate it into the next version of your patch.
(In reply to comment #44)
> Shouldn't this be |visible|, not |hidden|?

Indeed it should. This and the inclusion of textbox.xml means I should have double checked this patch again before posting it, I think! I'll take a look at your other changes and look into combining them with my patch, as well.
Whiteboard: [patch-r?]
Attached patch patch — — Splinter Review 
Fix the problem mentioned in comment 44, and incorporate the changes from Wayne for bookmark DnD.
Attachment #273125 - Attachment is obsolete: true
Attachment #273306 - Attachment is obsolete: true
Attachment #273459 - Flags: review?(mano)
Whiteboard: [patch-r?][needs review mano]
Comment on attachment 273459 [details] [diff] [review]
patch

r=me on this, since Mano's not around right now
Attachment #273459 - Flags: review?(mano) → review+
mozilla/toolkit/themes/pinstripe/global/browser.css 	1.21
mozilla/toolkit/themes/winstripe/global/browser.css 	1.32
mozilla/browser/themes/pinstripe/browser/browser.css 	1.59
mozilla/browser/themes/winstripe/browser/browser.css 	1.69
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [patch-r?][needs review mano]
Afaict, my testcase (from comment 11), https://bugzilla.mozilla.org/attachment.cgi?id=246022 , still doesn't work.
And for some reason I don't know, it has been marked obsolete.
(In reply to comment #51)
> Afaict, my testcase (from comment 11),
> https://bugzilla.mozilla.org/attachment.cgi?id=246022 , still doesn't work.
> And for some reason I don't know, it has been marked obsolete.

I can't reproduce the bug using that testcase (trunk, Mac), but if you can, could you file a new bug on it? You're right that it shouldn't have been marked obsolete, we just worked around the thread manager regression here.
(In reply to comment #51)
> Afaict, my testcase (from comment 11),
> https://bugzilla.mozilla.org/attachment.cgi?id=246022 , still doesn't work.
> And for some reason I don't know, it has been marked obsolete.

I think the evidence was stacking up against that attachment being related to this particular bug. I can still only reproduce that bug with the situation described in comment 23, which seems to be related to speed of the drag, and has existed for years. (s/can't/can at the top of that comment, btw).

If you file a new bug, could you please post it here? I'd still be interested in seeing what's going on there. :)
Depends on: 389615
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/200707251717 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bug still there, in SeaMonkey.

I wonder if the following file(s) should be fixed too ?
{{
/themes/classic/navigator/mac/tabbrowser.css,
    * line 92 -- .tab-drop-indicator-bar {
/themes/classic/navigator/win/tabbrowser.css,
    * line 54 -- .tab-drop-indicator-bar {
/themes/modern/navigator/tabbrowser.css,
    * line 108 -- .tab-drop-indicator-bar {
}}
(In reply to comment #54)
> I wonder if the following file(s) should be fixed too ?

Seems likely! Want to file a SeaMonkey bug for that?
I filed
Bug 389675 – [SeaMonkey port] Tab drag and drop indicator (arrow) only works after first drag of a tab
Status: RESOLVED → VERIFIED
No longer depends on: 389615
Should this patch be backed out, now that bug 389931 (which fixed bug 326273 regression) is fixed ?
Yes.
Can you look into this ?
I'll try, but I'm hopelessly behind all kinds of things I would like to do.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: