273800 - Can't drag favicons of 3rd and following tabs to create bookmarks, weblocs in recent nightlies

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(Not positive if this should go in tabs or drag and drop)

This is reproducible always beginning with the 20041202 0.8+ nightly.  

If you have multiple tabs open, you can normally drag the favicon (or globe on
sites without favicons) in the tab title to the bookmarks bar to create a
bookmark or the desktop to create a webloc.  Beginning in the 02-Dec build, you
can only drag the weblocs of the first *two* tabs; tabs 3-n do not respond to
attempts to drag.

There were a couple of Camino-specific checkins to the trunk on 01-Dec (bug
272189, bug 262815, and bug 160348; my layman's guess would be the last of the
three).

This is not a big bug, but it is a regression from earlier 0.8+ and I believe
0.8.2, but it's a bit annoying since dragging favicons from tabs was a
work-around for the multiple bookmarks/weblocs bug. :-)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Hmm, at some point tabs two and one stop working, too (after browsing for a
while), no clear cause.  

However, at every start-up--even with a fresh profile, which is what I used when
tracking down the build in which it first appeared--the third and following
tabs/favicons don't allow drags.

Comment 2

[Julian Pellico]

Confirming. This sounds bad. So you think it's the third bug? Aside from that
being my fix, that code only affected the drag destination -- looks like the
drag isn't even starting at the source on the tab.

Comment 3

[Julian Pellico]

It appears that the call to
dragImage:at:offset:event:pasteboard:source:slideback: near
BrowserTabViewItem.mm:293 (not exact line number, tossed in some NSLogs) fails
to start a drag.

Looking at the variable iconRect, I get values like
((202, 5), (15, 15))
when you drag from the 1st 2 tabs.
I see values like
((377, 740), (15, 15))
when you drag from the 3rd and later tab. Notice the wildly different y-origin.

I tried setting iconRect.origin.y = 5; by hand right before |dragImage|, but
that wasn't a quick fix. Something weird is afoot! 

I found that the [mLabelCell imageFrame] has a high y-origin for all the tabs,
so |convertRect| is doing something funky for the later tabs...

Comment 4

[Julian Pellico]

Confirmed: the final patch for bug 262815is the cause. It doesn't look like too
much code, so I'll take a gander

Comment 5

[Julian Pellico]

takey takey

Comment 6

[Julian Pellico]

Attachment: fix drag on favicons

Reviewers, please test carefully to see if there are any nuances with the
change.

Comment 7

[Simon Fraser [no longer active]]

Comment on attachment 170204 [details] [diff] [review]
fix drag on favicons

>+  // the new tab view item needs to have its tab visibility synchronized to the tab bar so that
>+  // its content view will be hooked up correctly
>+  if ([tabViewItem respondsToSelector:@selector(updateTabVisibility:)])
>+    [tabViewItem updateTabVisibility:[mTabBar isVisible]];
>   [self showOrHideTabsAsAppropriate];
> }


Nit: in another place in the file, the same check is done thusly:

  if ([tabItem isMemberOfClass:[BrowserTabViewItem class]])
    [(BrowserTabViewItem*)tabItem updateTabVisibility:tabsVisible];

so it would be nice to standardize.
r/sr=sfraser either way.

Comment 8

[Ludovic Hirlimann [:Usul]]

Julian : with 2005010808 (v0.8+) I can't repro. It's seem WORKSFORME. Can you
confirm ?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ludovic, I still have the problem in 2005011308 (v0.8+), the latest build I
have.  I even tested with a fresh profile.  

Make sure you're not trying to drag the globe/favicon on the first two tabs
(which work most of the time); it's only on tabs >2 where this is an issue all
the time.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ludovic: Also, Wevah posted a tip
http://forums.mozillazine.org/viewtopic.php?p=1150672#1150672 where invoking the
bookmarks manager seems to restore favicon-drag ability, so if you had opened
the bookmarks manager recently, maybe that is why it was WFM when you tried it.  

However, it seems Wevah's tip only works for tabs that were present when the BM
was activated; new tabs are still unable to have their favicons dragged in
existing builds.

Comment 11

[Julian Pellico]

The patch works, just waiting for sr/commit.

Comment 12

[Julian Pellico]

(In reply to comment #7)
> Nit: in another place in the file, the same check is done thusly:
> 
>   if ([tabItem isMemberOfClass:[BrowserTabViewItem class]])
>     [(BrowserTabViewItem*)tabItem updateTabVisibility:tabsVisible];
> 
> so it would be nice to standardize.
> r/sr=sfraser either way.
> 

Good point, consistency for that would be good.

Comment 13

[Julian Pellico]

Attachment: patch deux

r?'ing Geoff and seeing if he responds...

Comment 14

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 171563 [details] [diff] [review]
patch deux

This is the right thing to do and works as described.

r=[e-mail]

Comment 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 171563 [details] [diff] [review]
patch deux

sr=pink, but i'm curious to know why this works for the first 2 tabs and the
patch fixes 3+?

Comment 16

[Julian Pellico]

Something special happens when the tab bar becomes visible (the 2nd tab is created).

http://lxr.mozilla.org/mozilla/source/camino/src/browser/BrowserTabView.mm#217

In particular, this code gets run when the 2nd tab is created:
if (tabVisibilityChanged) {
237       // tell the tabs that visibility changed
238       NSArray* tabViewItems = [self tabViewItems];
239       for (int i = 0; i < numItems; i ++) {
240         NSTabViewItem* tabItem = [tabViewItems objectAtIndex:i];
241         if ([tabItem isMemberOfClass:[BrowserTabViewItem class]])
242           [(BrowserTabViewItem*)tabItem updateTabVisibility:tabsVisible];
243       }

See how this code doesn't (currently) get run on subsequent tabs.

Comment 17

[Josh Aas]

landed on trunk