337802 - Rollover tracking should not "highlight" the close button if only one tab is visible

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Chris Lawson (gone)]

If "Always Show Tab Bar" is checked in prefs and all tabs but one are closed, mousing over the close box will cause a highlight. This shouldn't happen, since the close box is disabled in that case.

This was caused by the checkin for bug 235864.

Comment 1

[Stuart Morgan]

This will be easy to fix by just bailing out where appropriate if the button isn't enabled.

Comment 2

[Aaron Schulman]

Attachment: RolloverImageButton WIP Patch

Not sure about the neccesarness of the extra #import "RolloverImageButton.h" waiting for trunk to compile to find this out

Comment 3

[Chris Lawson (gone)]

Comment on attachment 221894 [details] [diff] [review]
RolloverImageButton WIP Patch

>+- (void)setEnabled:inStatus

inStatus needs to be cast as a BOOL.

>@@ -171,21 +180,23 @@
> }
> 
> @end
> 
> @implementation RolloverImageButton (Private)
> 
> - (void)updateImage:(BOOL)inIsInside
> {
>+  if ([self isEnabled]) {
>     if (inIsInside)
>       [super setImage:mHoverImage];
>     else
>       [super setImage:mImage];
>+  }
> }  
> 
> - (BOOL)isMouseInside
> {
>   NSPoint mousePointInWindow = [[self window] convertScreenToBase:[NSEvent mouseLocation]];
>   NSPoint mousePointInView = [[self superview] convertPoint:mousePointInWindow fromView:nil];
>   return NSMouseInRect(mousePointInView, [self frame], NO);
> }

This hunk of the patch failed to apply here. You didn't hand-edit it, did you?

r=me with the above fixes addressed. Please upload a fresh patch when you've fixed it so I can double-check that it applies properly. :)

cl

Comment 4

[Stuart Morgan]

It didn't apply because it was -w; the four lines that were shifted to the right changed, but aren't actually shown as such in the patch.

>+      [(RolloverImageButton*)[[[self tabViewItems] objectAtIndex:0] closeButton] setEnabled:initialCloseButtonEnabled];

Why are you adding this cast?  There's nothing specific to your subclass here; setEnabled is part of all NSControls

>+  if ([self isEnabled])
>+    [self updateTrackingRectInSuperview];
>+  else
>+    [self removeTrackingRectFromView:[self superview]];

If a button is disabled while hovered, won't this wedge it in hover mode?

Comment 5

[Aaron Schulman]

(In reply to comment #4)
> It didn't apply because it was -w; the four lines that were shifted to the
> right changed, but aren't actually shown as such in the patch.
> 
> >+      [(RolloverImageButton*)[[[self tabViewItems] objectAtIndex:0] closeButton] setEnabled:initialCloseButtonEnabled];
> 
> Why are you adding this cast?  There's nothing specific to your subclass here;

There was an issue with a warning issued on this line. This cast seems to get rid of it, but I do not understand why it would be issued at all because closeButton returns an object of type RolloverImageButton*. I will get rid of this cast for now and once my recompile of the trunk finnishes I will add it in.


> 
> >+  if ([self isEnabled])
> >+    [self updateTrackingRectInSuperview];
> >+  else
> >+    [self removeTrackingRectFromView:[self superview]];
> 
> If a button is disabled while hovered, won't this wedge it in hover mode?
> 

Yes it would I will add a line to make sure that the button changes to the default image, good catch.

Patch coming soon

Comment 6

[Aaron Schulman]

Attachment: RolloverImageButton Patch

Issues described above are fixed, fixing the warning is more part of Bug #235864, so I will address it in there.

Aaron

Comment 7

[Chris Lawson (gone)]

Comment on attachment 221928 [details] [diff] [review]
RolloverImageButton Patch

r=me; patch applies cleanly and button doesn't get "stuck" in highlight mode if you close a tab while hovering over the close button on the last remaining tab.

Comment 8

[Stuart Morgan]

Comment on attachment 221928 [details] [diff] [review]
RolloverImageButton Patch

> - (void)updateImage:(BOOL)inIsInside
> {
>-  if (inIsInside)
>-    [super setImage:mHoverImage];
>-  else
>-    [super setImage:mImage];
>+  if ([self isEnabled]) {
>+    if (inIsInside)
>+      [super setImage:mHoverImage];
>+    else
>+      [super setImage:mImage];
>+  }
> }

This makes it impossible to change the default image of a disabled button. Only the hover image part should be conditionalized.

Comment 9

[Aaron Schulman]

Attachment: RolloverImageButton Patch

Great find smorgan, as always.

Comment 10

[Chris Lawson (gone)]

Comment on attachment 222102 [details] [diff] [review]
RolloverImageButton Patch

Setting r flag to proper reviewer.

cl

Comment 11

[Stuart Morgan]

Comment on attachment 222102 [details] [diff] [review]
RolloverImageButton Patch

r=me

Comment 12

[Aaron Schulman]

Comment on attachment 222102 [details] [diff] [review]
RolloverImageButton Patch

Targeting Pink for SR

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 222102 [details] [diff] [review]
RolloverImageButton Patch

sr=pink

Comment 14

[Håkan Waara]

Checked in

Comment 15

[Paul Borokhov (lensovet)]

hey, why is the close box disabled anyway, when we have just one tab? why not just have = red gumdrop?

Comment 16

[[e-mail]]

Verified on 1.8 branch and trunk