Last Comment Bug 336216 - Tabs don't do mouse tracking correctly across key window changes
: Tabs don't do mouse tracking correctly across key window changes
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Tabbed Browsing (show other bugs)
: Trunk
: PowerPC Mac OS X
-- minor (vote)
: Camino1.5
Assigned To: Aaron Schulman
:
:
Mentors:
: 339010 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-01 22:44 PDT by Stuart Morgan
Modified: 2006-07-13 08:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch: Makes tabs respond to key window changes (5.97 KB, patch)
2006-06-04 22:11 PDT, Aaron Schulman
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (Fixed: added condition to tab buttons needing re-display) (7.25 KB, patch)
2006-06-28 19:01 PDT, Aaron Schulman
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
Patch (Fixed: Notification issues, spaces) (4.75 KB, patch)
2006-07-06 23:46 PDT, Aaron Schulman
no flags Details | Diff | Splinter Review
Patch (7.08 KB, patch)
2006-07-11 21:07 PDT, Aaron Schulman
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Stuart Morgan 2006-05-01 22:44:08 PDT
It's possible (although not 100% reproducible) to wedge a tab in the wrong hover state by changing windows or applications while the mouse is over it.  It looks like this is going to lead to even more weird-looking behavior with 235864, which makes is why I'm marking it minor instead of trivial.
Comment 1 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-23 15:14:58 PDT
*** Bug 339010 has been marked as a duplicate of this bug. ***
Comment 2 User image Aaron Schulman 2006-06-03 15:24:59 PDT
Steps to reproduce behavior:

1. Open any program (iTunes) and Camino
2. Switch to Camino and make several new tabs
3. Move mouse into the tab bar and hover one item in it
4. Hit Apple+Tab to switch to another program
5. Move the mouse so it is no longer inside the tab
6. Go back to Camino and the tab should be stuck in the hover mode

It seems to be that this problem stems from TabButtonCell items not responding to window did become key notifications. I have a patch for this coming soon.
Comment 3 User image Aaron Schulman 2006-06-03 15:27:07 PDT
Forgot to mention!


You the window you are Apple+Tabbing to must cover the hovered tab button.

This is neccesary because it prevents the TabButtonCell drawing code from being called.
Comment 4 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-03 22:50:23 PDT
(In reply to comment #3)
> You the window you are Apple+Tabbing to must cover the hovered tab button.

Not with Sam's steps in bug 339010; the hovered button can be completely visible in that scenario.

Setting this to 1.1 based on Aaron's "patch for this coming soon" comment.
Comment 5 User image Aaron Schulman 2006-06-04 22:11:07 PDT
Created attachment 224399 [details] [diff] [review]
Patch: Makes tabs respond to key window changes

Proposed patch attached.

Enjoy
Comment 6 User image Stuart Morgan 2006-06-26 22:05:49 PDT
Comment on attachment 224399 [details] [diff] [review]
Patch: Makes tabs respond to key window changes

>+- (void)viewDidMoveToWindow
>+{
>+  // setup the tab bar to recieve notifications of key window changes
>+  NSNotificationCenter *nc = [NSNotificationCenter defaultCenter];
>+  [nc removeObserver:self name:NSWindowDidBecomeKeyNotification object:[self window]];
>+  [nc removeObserver:self name:NSWindowDidResignKeyNotification object:[self window]];
>+  if ([self window]) {
>+    [nc addObserver:self selector:@selector(handleWindowIsKey:)
>+          name:NSWindowDidBecomeKeyNotification object:[self window]];
>+    [nc addObserver:self selector:@selector(handleWindowResignKey:)
>+          name:NSWindowDidResignKeyNotification object:[self window]];
>+  }
>+}

Assuming cross-window tab moving were implemented, would this be sufficient?  It look like it would stay registered in the old window. (I'm not very familiar with the details of moving views between windows though, so I may be mistaken.)

>+- (void)handleWindowIsKey:(NSWindow *)inWindow
>+{
>+  // set tab bar dirty because the location of the mouse may have changed when the view's window was not key
>+  [self setNeedsDisplay:YES];
>+}
>+
>+- (void)handleWindowResignKey:(NSWindow *)inWindow
>+{
>+  // set tab bar dirty because the window does not respond to mouse hovering events when it is not key
>+  [self setNeedsDisplay:YES];
>+}

Given how much work is done when the tab bar is setNeedsDisplay'd, it would be nice to avoid doing it when it's not necessary.  These could be conditionalized on the mouse actually being in the tab bar at the time.


Also, be careful about introducing whitespace characters on blank lines; this patch does it in a couple of places.
Comment 7 User image Aaron Schulman 2006-06-27 17:25:07 PDT
(In reply to comment #6)
> (From update of attachment 224399 [details] [diff] [review] [edit])
> >+- (void)viewDidMoveToWindow
> >+{
> >+  // setup the tab bar to recieve notifications of key window changes
> >+  NSNotificationCenter *nc = [NSNotificationCenter defaultCenter];
> >+  [nc removeObserver:self name:NSWindowDidBecomeKeyNotification object:[self window]];
> >+  [nc removeObserver:self name:NSWindowDidResignKeyNotification object:[self window]];
> >+  if ([self window]) {
> >+    [nc addObserver:self selector:@selector(handleWindowIsKey:)
> >+          name:NSWindowDidBecomeKeyNotification object:[self window]];
> >+    [nc addObserver:self selector:@selector(handleWindowResignKey:)
> >+          name:NSWindowDidResignKeyNotification object:[self window]];
> >+  }
> >+}
> 
> Assuming cross-window tab moving were implemented, would this be sufficient? 
> It look like it would stay registered in the old window. (I'm not very familiar
> with the details of moving views between windows though, so I may be mistaken.)
> 

The notification is being added to the tab bar view, this should not move inbetween windows when a tab is moved. I believe in this case there will be only movement of BrowserTabViewItems.

> >+- (void)handleWindowIsKey:(NSWindow *)inWindow
> >+{
> >+  // set tab bar dirty because the location of the mouse may have changed when the view's window was not key
> >+  [self setNeedsDisplay:YES];
> >+}
> >+
> >+- (void)handleWindowResignKey:(NSWindow *)inWindow
> >+{
> >+  // set tab bar dirty because the window does not respond to mouse hovering events when it is not key
> >+  [self setNeedsDisplay:YES];
> >+}
> 
> Given how much work is done when the tab bar is setNeedsDisplay'd, it would be
> nice to avoid doing it when it's not necessary.  These could be conditionalized
> on the mouse actually being in the tab bar at the time.
> 
> 
> Also, be careful about introducing whitespace characters on blank lines; this
> patch does it in a couple of places.
> 

This is fixed and will be added to the patch following comments from smorgan on the first issue
Comment 8 User image Stuart Morgan 2006-06-27 20:03:05 PDT
> The notification is being added to the tab bar view, this should not move
> inbetween windows when a tab is moved.

Yup, I wasn't thinking. It seems pretty safe to assume that drag and drop of the entire tab bar won't be supported ;)
Comment 9 User image Aaron Schulman 2006-06-28 19:01:32 PDT
Created attachment 227489 [details] [diff] [review]
Patch (Fixed: added condition to tab buttons needing re-display)
Comment 10 User image Stuart Morgan 2006-06-30 08:39:46 PDT
Comment on attachment 227489 [details] [diff] [review]
Patch (Fixed: added condition to tab buttons needing re-display)

r=me
Comment 11 User image Mike Pinkerton (not reading bugmail) 2006-07-06 09:28:14 PDT
+  NSNotificationCenter *nc = [NSNotificationCenter defaultCenter];
+  [nc removeObserver:self name:NSWindowDidBecomeKeyNotification object:[self window]];
+  [nc removeObserver:self name:NSWindowDidResignKeyNotification object:[self window]];

you can just call [ns removeObserver:self] once instead of individually. It will remove it from everything in one go. You only want to do this in the dealloc, i guess the other place you do this you should continue with the individual observers just in case there are other things registered.

+  return NSMouseInRect(mousePointInView,[self frame],NO);

nit, spaces after commas in param lists.

looks good otherwise.
Comment 12 User image Aaron Schulman 2006-07-06 23:46:40 PDT
Created attachment 228397 [details] [diff] [review]
Patch (Fixed: Notification issues, spaces)
Comment 13 User image Aaron Schulman 2006-07-11 21:07:30 PDT
Created attachment 228900 [details] [diff] [review]
Patch
Comment 14 User image Stuart Morgan 2006-07-11 21:21:18 PDT
Comment on attachment 228900 [details] [diff] [review]
Patch

r=me
Comment 15 User image Mike Pinkerton (not reading bugmail) 2006-07-12 05:37:59 PDT
Comment on attachment 228900 [details] [diff] [review]
Patch

sr=pink
Comment 16 User image Nick Kreeger 2006-07-13 08:00:17 PDT
Baking trunk and branch.

Note You need to log in before you can comment on or make changes to this bug.