The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Tabbed Browsing
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: Aaron Schulman)

Tracking

({fixed1.8.1})

Trunk
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
Assignee: nobody → aschulm
*** Bug 339010 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 2

11 years ago
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.
(Assignee)

Comment 3

11 years ago
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.
(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.
Keywords: polish
Target Milestone: --- → Camino1.1
(Assignee)

Comment 5

11 years ago
Created attachment 224399 [details] [diff] [review]
Patch: Makes tabs respond to key window changes

Proposed patch attached.

Enjoy
Attachment #224399 - Flags: review?(stuart.morgan)
(Reporter)

Comment 6

11 years ago
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.
Attachment #224399 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 7

11 years ago
(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
(Reporter)

Comment 8

11 years ago
> 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 ;)
(Assignee)

Comment 9

11 years ago
Created attachment 227489 [details] [diff] [review]
Patch (Fixed: added condition to tab buttons needing re-display)
Attachment #227489 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Attachment #227489 - Flags: superreview?
(Reporter)

Comment 10

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

r=me
Attachment #227489 - Flags: review?(stuart.morgan) → review+
+  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.
(Assignee)

Comment 12

11 years ago
Created attachment 228397 [details] [diff] [review]
Patch (Fixed: Notification issues, spaces)
Attachment #227489 - Attachment is obsolete: true
Attachment #228397 - Flags: superreview?
Attachment #228397 - Flags: review?(stuart.morgan)
Attachment #227489 - Flags: superreview?
(Assignee)

Comment 13

11 years ago
Created attachment 228900 [details] [diff] [review]
Patch
Attachment #224399 - Attachment is obsolete: true
Attachment #228397 - Attachment is obsolete: true
Attachment #228900 - Flags: review?(stuart.morgan)
Attachment #228397 - Flags: superreview?
Attachment #228397 - Flags: review?(stuart.morgan)
(Reporter)

Comment 14

11 years ago
Comment on attachment 228900 [details] [diff] [review]
Patch

r=me
Attachment #228900 - Flags: superreview?(mikepinkerton)
Attachment #228900 - Flags: review?(stuart.morgan)
Attachment #228900 - Flags: review+
Comment on attachment 228900 [details] [diff] [review]
Patch

sr=pink
Attachment #228900 - Flags: superreview?(mikepinkerton) → superreview+

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 16

11 years ago
Baking trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: polish → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.