311110 - Dragging behaviour of bookmarks into empty area in tab bar is inconsistent

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Chris Lawson (gone)]

When a single bookmark is dragged from the bookmarks bar to an empty area of the
tab bar, it opens a new tab in the background with the contents of that bookmark.

When a single bookmark is dragged from the bookmarks bar to an already-open tab,
it replaces the contents of that tab with the contents of the bookmark.

However, when a tab group is dragged into the empty area of the tab bar, it
destroys all open tabs in that window and opens only the tabs in the tab group
(like you had simply clicked on it in the bookmark bar).

Dragging something to the empty space on the tab bar seems to imply "open this
in addition to what I've already got open." Therefore, our behaviour with tab
groups is inconsistent and counterintuitive here, and should be amended to open
tab groups in new (background) tabs.

(I haven't looked into whether the drag behaviour obeys the preference to always
open new links in the background, but it seems logical that it should.)

Comment 1

[Chris Lawson (gone)]

Attachment: Appends the contents of a tab group in new tabs, rather than replacing current tabs

Also contains some stylistic fixes to BrowserTabView.mm for spurious spaces and
the like.

Comment 2

[Chris Lawson (gone)]

Attachment: New version, fixes bug where dragging to an extant tab would append rather than replace

Oops. This one's better. :)

cl

Comment 3

[L. H.]

>if (!overTabViewItem) // if not over a tabViewItem, append rather than replace
(bug 311110)
>     [[[self window] windowController] openURLArray:[aBookmark childURLs]
tabOpenPolicy:eAppendTabs allowPopups:NO];
>   else // user dropped a tab group on an existing tab, so replace instead of
append
>     [[[self window] windowController] openURLArray:[aBookmark childURLs]
tabOpenPolicy:eReplaceTabs allowPopups:NO];

The difference is more clear with just
[[[self window] windowController] openURLArray:[aBookmark childURLs]
tabOpenPolicy:(overTabViewItem ? eReplaceTabs : eAppendTabs) allowPopups:NO];

Comment 4

[Chris Lawson (gone)]

Attachment: New version incorporating LH's suggestion

Yeah, I was thinking I could do that somehow, but I didn't make the leap to
nesting inside the method call. Man, I love Obj-C.

cl

Comment 5

[Chris Lawson (gone)]

Comment on attachment 198541 [details] [diff] [review]
New version incorporating LH's suggestion

asking for r from pink, add pink to cc list (was never on it as I created and
assigned myself this one in a single step)

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 198541 [details] [diff] [review]
New version incorporating LH's suggestion

sr=pink. smfr, you agree here?

Comment 7

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

Simon, will you be able to get to this review before 1.0?  (Do we want this fix for 1.0?)  The current behavior is kind-of nasty.

Comment 8

[Simon Fraser [no longer active]]

Comment on attachment 198541 [details] [diff] [review]
New version incorporating LH's suggestion

>Index: BrowserTabView.mm
>===================================================================

>         if ([aBookmark isKindOfClass:[Bookmark class]])
>           return [self handleDropOnTab:overTabViewItem overContent:overContentArea withURL:[aBookmark url]];
>+          
>         else if ([aBookmark isKindOfClass:[BookmarkFolder class]]) {

Remove the 'else', or remove the blank line, but don't have both.

>-          [[[self window] windowController] openURLArray:[aBookmark childURLs] tabOpenPolicy:eReplaceTabs allowPopups:NO];
>-          return YES;
>+          // if the drag terminates over a TabViewItem, replace. Otherwise, user dragged to empty space in tab bar and we should append as new tabs (bug 311110).
>+          [[[self window] windowController] openURLArray:[aBookmark childURLs] tabOpenPolicy:(overTabViewItem ? eReplaceTabs : eAppendTabs) allowPopups:NO];
>+        return YES;

I don't think the comment is necessary, and please fix the indentation.

Comment 9

[Chris Lawson (gone)]

Attachment: Fixed per Simon's suggestions

That should fix it; not sure how that whitespace mixup got in there to begin with.

cl

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 201282 [details] [diff] [review]
Fixed per Simon's suggestions

I checked this in. I also fixed the > 1 drag item clause too.