336216 - Tabs don't do mouse tracking correctly across key window changes

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Stuart Morgan]

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

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

*** Bug 339010 has been marked as a duplicate of this bug. ***

Comment 2

[Aaron Schulman]

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

[Aaron Schulman]

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

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

(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

[Aaron Schulman]

Attachment: Patch: Makes tabs respond to key window changes

Proposed patch attached.

Enjoy

Comment 6

[Stuart Morgan]

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

[Aaron Schulman]

(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

[Stuart Morgan]

> 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

[Aaron Schulman]

Attachment: Patch (Fixed: added condition to tab buttons needing re-display)

Comment 10

[Stuart Morgan]

Comment on attachment 227489 [details] [diff] [review]
Patch (Fixed: added condition to tab buttons needing re-display)

r=me

Comment 11

[Mike Pinkerton (not reading bugmail)]

+  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

[Aaron Schulman]

Attachment: Patch (Fixed: Notification issues, spaces)

Comment 13

[Aaron Schulman]

Attachment: Patch

Comment 14

[Stuart Morgan]

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

r=me

Comment 15

[Mike Pinkerton (not reading bugmail)]

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

sr=pink

Comment 16

[Nick Kreeger]

Baking trunk and branch.