418359 - Start drag of background tabs with one click

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Jeff Dlouhy]

Moved from bug 160720 comment #62:
We only begin a drag if the tab button is the active tab.  Dragging tabs
does not feel as natural this way - if I decide to move a background one I have
to first select it, release the mouse, then click again and start dragging.  I
think a better approach would be to active the tab immediately and begin a drag
all under the same mouseDown: event.

Comment 1

[Sean Murphy]

Reassigning, working on fixing this asap :).

Comment 2

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

Blocking b1, per the meeting; Sean should have a patch for it soon.

Comment 3

[Sean Murphy]

Attachment: Patch

When a background TabButtonView is clicked on and dragged, we are not receiving any mouseDragged: events on either the button itself or the BrowserTabBarView.  

The reason for this is because the TabButtonView that first received the event is removed from the view hierarchy when the tab bar is rebuilt (which needs to be done on selection change).  It seems that, when the view that first received the |mouseDown:| event is removed from the view hierarchy, AppKit will not follow with any further |mouseDragged:| events.  I created a test project to exclude any other factors, such as Gecko issues, and can reproduce this assumption.

As sort of a workaround, the only way I can think of us solving this is to simulate another mouseDown: event after the bar is rebuilt, which will allow the |mouseDragged:| events to be processed.  I put some checks in to make sure we do not simulate that event during any other situation, and I could not find any negative side-effects from doing it.  Hopefully it's a suitable approach :).

Comment 4

[Stuart Morgan]

Comment on attachment 351473 [details] [diff] [review]
Patch

Good find on the cause here!

I'm not wild about the idea of hacking around this though; alternate approach coming in a second.

Comment 5

[Stuart Morgan]

Attachment: don't mess with the views

When we are doing nothing but changing the selection (which is, as it turns out, always the case when one is starting a drag), there's no need to tear out and re-add the views--we've just never had any real reason not to before. This is more plumbing, but the the effect is minor: we just do somewhat less work in the simple case, and the same thing we've always done the rest of the time.

Comment 6

[Sean Murphy]

Comment on attachment 351700 [details] [diff] [review]
don't mess with the views

Thanks for stepping in Stuart with the alternate approach.  I do not see any issues at all, everything looks great to me.  r=murph.

Comment 7

[Mike Pinkerton (not reading bugmail)]

+typedef enum {
+  kTabRebuildForSelectionChange,
+  kTabRebuildForStructureChange
+} ETabRebuildReason;

in c++, i don't think you need the |typedef| keyword.

sr=pink

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 351700 [details] [diff] [review]
don't mess with the views

sr=pink

Comment 9

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

Landed on cvs trunk for Stuart with

enum ETabRebuildReason {
  kTabRebuildForSelectionChange,
  kTabRebuildForStructureChange
};

I think we can freeze for b1 now!