235864 - close tab button does not give mouse hover feedback

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Tristan O'Tierney]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040227 Camino/0.7+

when mousing over the close tab button X's in the tab bar, no visual cue is
given to denote that the user is over the button. since the button's size is
quite small, this visual cue would go a long way to increasing the visibility of
the user's intended actions and preventing accidental tab closes.

Reproducible: Always
Steps to Reproduce:
1. open a window
2. open an extra tab
3. mouse over tab bar X button
Actual Results:  
nothing happens

Expected Results:  
the circular X should perhaps become a lighter color, or change somehow to give
feedback.

Comment 1

[Mike Pinkerton (not reading bugmail)]

i know this was on jasper's wish list, not sure if it's covered by other bugs.

Comment 2

[Jan Gundtofte-Bruun]

If many tabs are open, it would be a nice reassurance if the "tooltip" were to
include the title (as in 'HTML meta title tag') of the tab that would close by
clicking the button (i.e. "Close the 'Bug 235864' tab").

Comment 3

[Bruce Davidson]

This would defintely be helpful. Note that the new tab implementation has
currently made this worse - when you mouseover the close widget you get the same
effect (the entire tab highlights) as when you move the mouse over any other bit
of the tab. i.e. while the mouse is over a destructive widget we provide
feedback that we'll perform a safe action - switching to the tab.

I'd suggest that this increases the urgency of a fix for this bug (or at least
starting by disabling the current effect).

Comment 4

[Ludovic Hirlimann [:Usul]]

still valid in 2004090408 (v0.8+)

Comment 5

[Steve Ruge]

(In reply to comment #4)
> still valid in 2004090408 (v0.8+)

No feedback whatsoever for any items on each tab in 2004082512 (v0.8.1). 

Feedback as far as changing the shade of the "X" should be a "would be nice to"
upgrade. I suspect most people clicking the Close wigit would just be expecting
the tab to close, not needing visual feedback.

Comment 6

[Steve Ruge]

(In reply to comment #5)
> (In reply to comment #4)
> > still valid in 2004090408 (v0.8+)
> 
> No feedback whatsoever for any items on each tab in 2004082512 (v0.8.1). 
> 
> Feedback as far as changing the shade of the "X" should be a "would be nice to"
> upgrade. I suspect most people clicking the Close wigit would just be expecting
> the tab to close, not needing visual feedback.

Whoops - see you tested with a later build than mine. Sorry!

Comment 7

[Samuel Sidler (old account; do not CC)]

Still valid with 2005072608. I doubt whether this will be done by 1.0 and if
it's even worth it.

Comment 8

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

Safari does it, and it's a nice bit of polish...for sometime.

Comment 9

[Samuel Sidler (old account; do not CC)]

Not for 1.0.

Comment 10

[Chris Lawson (gone)]

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

Comment 11

[Aaron Schulman]

I have an almost working patch for this bug, it involves a subclass of NSButton called RolloverImageButton.

I only have a few more issues to work out with updating the tracking rectangles, and I also need to create the mouseover close button graphic. If anyone has any recomendations for how the mouseover graphic should look, please post a comment here.

Thank you,
Aaron Schulman

Comment 12

[Aaron Schulman]

Proposed Close Button Graphic:

This is similar to safari:
ButtonUp: grey and white X
ButtonMouseOver: dark grey and white X
ButtonDown: dark grey and grey X

Thanks,
Aaron Schulman

Comment 13

[Markus Amalthea Magnuson]

Do you plan to have variations on these to differ the selected tab from the ones not selected? Since tabs in the background have a darker background color, the close buttons will look different on them if not compensated for in some way (by shifting the grays a bit, I guess). Or would that just make things too complicated?

I'm not sure how this is done in other applications. It looks like Safari darkens the close button when you mouse-over the tab. But when mouseing-over the close button on the same tab, the same image is used as in a selected tab.

Comment 14

[Chris Lawson (gone)]

(In reply to comment #13)
> I'm not sure how this is done in other applications. It looks like Safari
> darkens the close button when you mouse-over the tab. But when mouseing-over
> the close button on the same tab, the same image is used as in a selected tab.

That's how it should be, since the close button has the same action whether it's on a background tab or foreground tab. Either way, it closes the tab, so it should get the same highlight in both cases. Basically, if we emulate Safari here, we're doing exactly what we should be.

cl

Comment 15

[Chris Lawson (gone)]

Aaron and I were just talking on IRC, and there's a minor visual bug in the way his code (and Safari's) works:

1) Put the mouse over a close button.
2) Open a new tab.
3) Repeat until you trigger a resize of all tabs (i.e., 7 or 8 tabs in a typical window, maybe more if you have a Cinema Display :-p)
4) Watch button stay highlighted even though the mouse is no longer over it.

My suggestion for a workaround was to post a notification whenever the tabs resize, and then watch for this notification. If we see the notification *and* the mouse is no longer over the tracking rect for the button, send a mouseExited event to fix the problem.

cl

Comment 16

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

Jasper, can you go ahead and make the new hover state close button (as well as revise the other two, if needed); the code here is almost ready.

Comment 17

[Jasper]

Attachment: tab close icon states

Tab close icon states as descibed in comment #12.

Comment 18

[Aaron Schulman]

Attachment: WIP Patch (Does not include image changes when dragging)

A WIP Patch has been added, the images attached must be put in the Resources/images folder of the project. Also, you will no longer need the image "tab_close_pressed.tif". 

Enjoy,

Aaron Schulman

Comment 19

[Håkan Waara]

Comment on attachment 220122 [details] [diff] [review]
WIP Patch (Does not include image changes when dragging)

Who wants to review? Mento? Smorgan?

Comment 20

[Stuart Morgan]

I can review, but before I get into any specific reviews: why is the button's enclosing view (the tab) doing all the work of tracking the button? That seems like the wrong place for all of this.

Comment 21

[Stuart Morgan]

Also, from a general logic perspective, it looks like you are using the button images differently than comments 12 and 14 (and the images names) suggest they are supposed to be used.  Your three states appear to be
- normal
- hovered, in the current tab
- hovered, in a background tab

Was this discussed elsewhere?

Comment 22

[Aaron Schulman]

(In reply to comment #21)
> Also, from a general logic perspective, it looks like you are using the button
> images differently than comments 12 and 14 (and the images names) suggest they
> are supposed to be used.  Your three states appear to be
> - normal
> - hovered, in the current tab
> - hovered, in a background tab
> 
> Was this discussed elsewhere?
> 

You are correct, the problem I ran into here is what I should do with "tab_close.tif" in the images attached. 

Jasper, is this your intended configuration?

Close Button Up: tab_close_up.tif
Close Button Down: tab_close_down.tif
Close Button Hover: tab_close_over.tif

In other words, are we ignoring the issue here of two different images being used for a close button that is being hovered in a selected tab and one that is being hovered in a non selected tab?

Comment 23

[Aaron Schulman]

(In reply to comment #20)
> I can review, but before I get into any specific reviews: why is the button's
> enclosing view (the tab) doing all the work of tracking the button? That seems
> like the wrong place for all of this.
> 

I thought about this for some time. At first I thought it was best to subclass NSButton and add that tracking stuff into that (RolloverImageButton). This seemed to be more than was neccessary for this feature. 

The way I see it, a close button is a critical part of the Tab View, and thus the Tab View should maintain tracking of all of its components. Also, in the future if someone changes the close button to be some other class than NSButton tracking and changing images will still work so long as the new class has the ability to recieve setImage messages and is a subclass of NSView.

Let me know what you guys think,

Aaron S.

Comment 24

[Aaron Schulman]

In a quick check of my code I have found one mistake.

addTrackingRectInView in TabButtonCell.mm should have

  if (mCloseTrackingTag != -1) 
    [aView removeTrackingRect:mCloseTrackingTag];

Insted of 

  if (mCloseTrackingTag != -1) 
    [mCloseTrackingTag removeTrackingRect:mCloseTrackingTag];

Comment 25

[Aaron Schulman]

Comment on attachment 220122 [details] [diff] [review]
WIP Patch (Does not include image changes when dragging)

>? .DS_Store
>? chrome
>? closeButtonHover-4-28-06-v1.patch
>? closeButtonHover-4-28-06.patch
>? gc
>? camino/.DS_Store
>? camino/Camino.xcodeproj
>? camino/build
>? camino/embed-replacements.tmp
>? camino/wallet
>? camino/IBPalette/build
>? camino/resources/.DS_Store
>? camino/resources/images/.DS_Store
>? camino/resources/images/tab_close.tif
>? camino/resources/images/tab_close_down.tif
>? camino/resources/images/tab_close_over.tif
>? camino/resources/images/tab_close_up.tif
>? camino/resources/images/chrome/.DS_Store
>? camino/resources/images/chrome/tab_close_down.tif
>? camino/resources/images/chrome/tab_close_over.tif
>? camino/resources/images/chrome/tab_close_up.tif
>? camino/src/.DS_Store
>? embedding/components/printingui/src/mac/printpde/build
>? extensions/xmlextras/base/src/nsRect.cpp
>? gfx/gfx-config.h
>? gfx/cairo/cairo/src/cairo-features.h
>? js/src/host_jskwgen
>? js/src/jsautokw.h
>? modules/plugin/samples/default/mac/build
>? widget/src/cocoa/libwidget.rsrc
>? xpfe/bootstrap/appleevents/mozillaSuite.rsrc
>? xpfe/global/buildconfig.html
>Index: camino/resources/images/chrome/tab_close.tif
>===================================================================
>RCS file: /cvsroot/mozilla/camino/resources/images/chrome/tab_close.tif,v
>retrieving revision 1.3
>diff -u -8 -r1.3 tab_close.tif
>Binary files /tmp/cvsfuFynn and tab_close.tif differ
>Index: camino/src/browser/BrowserTabViewItem.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabViewItem.h,v
>retrieving revision 1.10
>diff -u -8 -r1.10 BrowserTabViewItem.h
>--- camino/src/browser/BrowserTabViewItem.h	28 Jun 2005 04:24:16 -0000	1.10
>+++ camino/src/browser/BrowserTabViewItem.h	28 Apr 2006 13:21:54 -0000
>@@ -74,10 +74,12 @@
> - (NSButton *)closeButton;
> - (NSMenuItem *)menuItem;
> - (void) willDeselect;
> - (void) willSelect;
> - (void) selectTab:(id)sender;
> 
> + (NSImage*)closeIcon;
> + (NSImage*)closeIconPressed;
>++ (NSImage*)closeIconSelectedHover;
>++ (NSImage*)closeIconNotSelectedHover;
> 
> @end
>Index: camino/src/browser/BrowserTabViewItem.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabViewItem.mm,v
>retrieving revision 1.21
>diff -u -8 -r1.21 BrowserTabViewItem.mm
>--- camino/src/browser/BrowserTabViewItem.mm	5 Dec 2005 06:38:09 -0000	1.21
>+++ camino/src/browser/BrowserTabViewItem.mm	28 Apr 2006 13:21:54 -0000
>@@ -543,20 +543,36 @@
>     sCloseIcon = [[NSImage imageNamed:@"tab_close"] retain];
>   return sCloseIcon;
> }
> 
> + (NSImage*)closeIconPressed
> {
>   static NSImage* sCloseIconPressed = nil;
>   if ( !sCloseIconPressed )
>-    sCloseIconPressed = [[NSImage imageNamed:@"tab_close_pressed"] retain];
>+    sCloseIconPressed = [[NSImage imageNamed:@"tab_close_down"] retain];
>   return sCloseIconPressed;
> }
> 
>++ (NSImage*)closeIconSelectedHover
>+{
>+  static NSImage* sCloseIconUpHover = nil;
>+  if ( !sCloseIconUpHover )
>+	sCloseIconUpHover = [[NSImage imageNamed:@"tab_close_up"] retain];
>+  return sCloseIconUpHover;
>+}
>+
>++ (NSImage*)closeIconNotSelectedHover
>+{
>+  static NSImage* sCloseIconNotSelectedHover = nil;
>+  if ( !sCloseIconNotSelectedHover )
>+	sCloseIconNotSelectedHover = [[NSImage imageNamed:@"tab_close_over"] retain];
>+  return sCloseIconNotSelectedHover;
>+}
>+
> #define NO_TOOLTIPS_ON_TABS 1
> 
> #ifdef NO_TOOLTIPS_ON_TABS
> // bug 168719 covers crashes in AppKit after using a lot of tabs because
> // the tooltip code internal to NSTabView/NSTabViewItem gets confused and
> // tries to set a tooltip for a (probably) deallocated object. Since we can't
> // easily get into the guts, all we can do is disable tooltips to fix this
> // topcrash by stubbing out the NSTabViewItem's method that sets up the
>Index: camino/src/extensions/RolloverTrackingCell.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/RolloverTrackingCell.h,v
>retrieving revision 1.2
>diff -u -8 -r1.2 RolloverTrackingCell.h
>--- camino/src/extensions/RolloverTrackingCell.h	2 Feb 2005 23:16:18 -0000	1.2
>+++ camino/src/extensions/RolloverTrackingCell.h	28 Apr 2006 13:21:54 -0000
>@@ -47,12 +47,14 @@
> }
> 
> -(void)setFrame:(NSRect)newFrame;
> -(NSRect)frame;
> -(NSSize)size;
> -(BOOL)mouseWithin;
> -(void)addTrackingRectInView:(NSView*)aView withFrame:(NSRect)trackingRect cursorLocation:(NSPoint)currentLocation;
> -(void)removeTrackingRectFromView:(NSView*)aView;
>+-(void)mouseEntered:(NSEvent *)theEvent;
>+-(void)mouseExited:(NSEvent *)theEvent;
> -(void)setDragTarget:(BOOL)isDragTarget;
> -(BOOL)dragTarget;
> 
> @end
>Index: camino/src/extensions/TabButtonCell.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/TabButtonCell.h,v
>retrieving revision 1.4
>diff -u -8 -r1.4 TabButtonCell.h
>--- camino/src/extensions/TabButtonCell.h	5 Dec 2005 06:38:11 -0000	1.4
>+++ camino/src/extensions/TabButtonCell.h	28 Apr 2006 13:21:54 -0000
>@@ -16,16 +16,17 @@
>  *
>  * The Initial Developer of the Original Code is
>  * Geoff Beier.
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Geoff Beier <[e-mail]>
>+ *   Aaron Schulman <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -40,18 +41,20 @@
> #import "RolloverTrackingCell.h"
> #import "BrowserTabViewItem.h"
> 
> @interface TabButtonCell : RolloverTrackingCell
> {
>   BrowserTabViewItem*   mTabViewItem;
>   BOOL                  mNeedsDivider;
>   NSButton*             mCloseButton;
>+  BOOL                  mMouseWithinClose;
>+  NSTrackingRectTag     mCloseTrackingTag;
> }
> 
> -(id)initFromTabViewItem:(BrowserTabViewItem*)tabViewItem;
> -(BOOL)isSelected;
> -(BrowserTabViewItem*)tabViewItem;
> -(BOOL)willTrackMouse:(NSEvent*)theEvent inRect:(NSRect)cellFrame ofView:(NSView*)controlView;
> -(void)hideCloseButton;
> -(void)setDrawDivider:(BOOL)willDraw;
>-
>+-(void)updateCloseButtonImage;
> @end
>Index: camino/src/extensions/TabButtonCell.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/extensions/TabButtonCell.mm,v
>retrieving revision 1.7
>diff -u -8 -r1.7 TabButtonCell.mm
>--- camino/src/extensions/TabButtonCell.mm	2 Feb 2006 06:41:56 -0000	1.7
>+++ camino/src/extensions/TabButtonCell.mm	28 Apr 2006 13:21:55 -0000
>@@ -16,16 +16,17 @@
>  *
>  * The Initial Developer of the Original Code is
>  * Geoff Beier.
>  * Portions created by the Initial Developer are Copyright (C) 2004
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Geoff Beier <[e-mail]>
>+ *   Aaron Schulman <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -33,16 +34,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #import "ImageAdditions.h"
> #import "NSBezierPath+Utils.h"
>+#import "RolloverTrackingCell.h"
> 
> #import "TabButtonCell.h"
> #import "TruncatingTextAndImageCell.h"
> 
> static const int kTabLeftMargin = 4; //distance between left edge and close button
> static const int kTabCloseButtonPad = 2; //distance between close button and label
> static const int kTabRightMargin = 4; //distance between label cell and right edge 
> static const int kTabBottomPad = 4; // number of pixels to offset from the bottom
>@@ -68,24 +70,27 @@
> @implementation TabButtonCell
> 
> -(id)initFromTabViewItem:(BrowserTabViewItem *)tabViewItem
> {
>   if ((self = [super init]))
>   {
>     mTabViewItem = tabViewItem;
>     mNeedsDivider = YES;
>+	mCloseTrackingTag = -1;
>+	mUserData = [[NSDictionary alloc] init];
>   }
>   return self;
> }
> 
> -(void)dealloc
> {
>   [mCloseButton removeFromSuperview];
>   [mCloseButton release];
>+  [mUserData release];
>   [super dealloc];
> }
> 
> -(BOOL)isSelected
> {
>   return ([mTabViewItem tabState] == NSSelectedTab);
> }
> 
>@@ -165,35 +170,97 @@
>     [controlView addSubview:mCloseButton];
>   }
>   [mCloseButton setFrame:buttonRect];
>   [labelCell drawInteriorWithFrame:labelRect inView:controlView];
> }
> 
> -(void)addTrackingRectInView:(NSView *)aView withFrame:(NSRect)trackingRect cursorLocation:(NSPoint)currentLocation
> {
>-  [super addTrackingRectInView:aView withFrame:trackingRect cursorLocation:currentLocation];
>+  [super addTrackingRectInView:aView withFrame:trackingRect cursorLocation:currentLocation]; 
>+  if (mCloseTrackingTag != -1) 
>+    [aView removeTrackingRect:mCloseTrackingTag];
>+  if (!mCloseButton) mCloseButton = [[mTabViewItem closeButton] retain];
>+  NSSize closeButtonSize = [mCloseButton frame].size;
>+  NSSize textSize = [mTabViewItem sizeOfLabel:NO];
>+  float maxHeight = textSize.height > closeButtonSize.height ? textSize.height : closeButtonSize.height;
>+  NSRect closeButtonTrackingRect = NSMakeRect(trackingRect.origin.x + kTabLeftMargin,
>+    trackingRect.origin.y + (int)((trackingRect.size.height - kTabBottomPad - maxHeight)/2.0 + kTabBottomPad),
>+    closeButtonSize.width, closeButtonSize.height); 
>+  mMouseWithinClose = NSPointInRect(currentLocation,closeButtonTrackingRect);
>+  mCloseTrackingTag = [aView addTrackingRect:closeButtonTrackingRect owner:self userData:mUserData assumeInside:mMouseWithinClose];
>+  [self updateCloseButtonImage];
>+  [aView setNeedsDisplay:YES];
>+  [aView displayIfNeeded];
> }
> 
>+
> -(void)removeTrackingRectFromView:(NSView *)aView
> {
>   [super removeTrackingRectFromView:aView];
>+  [aView removeTrackingRect:mCloseTrackingTag];
>+  mCloseTrackingTag = -1;
>+}
>+
>+- (void)mouseEntered:(NSEvent *)theEvent
>+{
>+  NSDictionary *userData = (NSDictionary *)[theEvent userData];
>+  NSView *view = [userData objectForKey:@"view"];
>+  if ([theEvent trackingNumber] == mCloseTrackingTag) {
>+      if ([[view window] isKeyWindow] || [view acceptsFirstMouse:theEvent]) {
>+	    mMouseWithinClose = YES;
>+	    [self updateCloseButtonImage];
>+        [view setNeedsDisplay:YES];
>+        [view displayIfNeeded];
>+      }
>+  } 
>+  else
>+    [super mouseEntered:theEvent];
>+}
>+
>+- (void)mouseExited:(NSEvent *)theEvent
>+{
>+  NSDictionary *userData = (NSDictionary *)[theEvent userData];
>+  NSView *view = [userData objectForKey:@"view"];
>+  if ([theEvent trackingNumber] == mCloseTrackingTag) {
>+      if ([[view window] isKeyWindow] || [view acceptsFirstMouse:theEvent]) {
>+	    mMouseWithinClose = NO;
>+	    [self updateCloseButtonImage];
>+        [view setNeedsDisplay:YES];
>+        [view displayIfNeeded];
>+      }
>+  } 
>+  else
>+    [super mouseExited:theEvent];
> }
> 
> -(void)hideCloseButton
> {
>   if ([mCloseButton superview] != nil)
>     [mCloseButton removeFromSuperview];
> }
> 
> -(void)setDrawDivider:(BOOL)willDraw
> {
>   mNeedsDivider = willDraw;
> }
> 
>+-(void)updateCloseButtonImage 
>+{
>+  if (mMouseWithinClose) {
>+    if ([self isSelected])
>+      [mCloseButton setImage:[BrowserTabViewItem closeIconSelectedHover]];
>+    else 
>+      [mCloseButton setImage:[BrowserTabViewItem closeIconNotSelectedHover]];
>+  }
>+  else {
>+    [mCloseButton setImage:[BrowserTabViewItem closeIcon]];
>+  }
>+}
>+
> 
> +(void)loadImages
> {
>   if (!gTabLeft)                gTabLeft                = [[NSImage imageNamed:@"tab_left_side"] retain];
>   if (!gTabRight)               gTabRight               = [[NSImage imageNamed:@"tab_right_side"] retain];
>   if (!gActiveTabBg)            gActiveTabBg            = [[NSImage imageNamed:@"tab_active_bg"] retain];
>   if (!gTabMouseOverBg)         gTabMouseOverBg         = [[NSImage imageNamed:@"tab_hover"] retain];
>   if (!gTabButtonDividerImage)  gTabButtonDividerImage  = [[NSImage imageNamed:@"tab_button_divider"] retain];

Comment 26

[Stuart Morgan]

Comment on attachment 220122 [details] [diff] [review]
WIP Patch (Does not include image changes when dragging)

The implementation details of the appearance of the close button are not integral to the tab view though; they belong in the button.  I'd like to see this as a button subclass. (I'd really like to see something that doesn't duplicate so much code from the tabs, but without language support for multiple inheritence or mixins I'm not seeing any elegant ways of doing that.)

I wont get into a full review since it will be rewritten anyway, but some notes that may help for the next version:

You've got tabs scattered through the patch; check your editor settings.

+	mUserData = [[NSDictionary alloc] init];

Since you aren't actually using user data, you should be able to get rid of this and pass nil to addTrackingRect:etc.

+  BOOL                  mMouseWithinClose;

Since there is a one-to-one correspondence between setting this and calling the image-update method, just make it a paramater to the method.

+  [aView setNeedsDisplay:YES];

Similarly, since you need to do this whenever you change the image, call it in the image-update method.

+  [aView displayIfNeeded];

Can the redraw really not wait until the end of the event loop? (And if it can't, move it as above.)

Comment 27

[Aaron Schulman]

Thank you for your comments, here is the new plan based off of what you have said,

New Class: RolloverImageButton <- Subclass of NSButton

The creation of the tracking rectangle will still be done in the TabButtonCell, but the responder will be the RolloverImageButton.

The RolloverImageButton will take care of the mouseEntered and mouseExited events to update the close button's image. 

Also I will move the code that creates the close button's frame into a function getCloseButtonFrame inside TabButtonCell, that way we do not have to duplicate code that is in the draw function and addTrackingRect function.

Also it does appear that you can wait untill the end of the event loop to draw the hover change.

Thanks again!

Aaron

Comment 28

[Stuart Morgan]

(In reply to comment #27)
> The creation of the tracking rectangle will still be done in the TabButtonCell

Can you elaborate on why?

Comment 29

[Aaron Schulman]

(In reply to comment #28)
> (In reply to comment #27)
> > The creation of the tracking rectangle will still be done in the TabButtonCell
> 
> Can you elaborate on why?
> 

NSSize textSize = [mTabViewItem sizeOfLabel:NO];
float maxHeight = textSize.height > closeButtonSize.height ? textSize.height : closeButtonSize.height;
 NSRect closeButtonTrackingRect = NSMakeRect(trackingRect.origin.x + kTabLeftMargin,
trackingRect.origin.y + (int)((trackingRect.size.height - kTabBottomPad - maxHeight)/2.0 + kTabBottomPad),
 closeButtonSize.width, closeButtonSize.height); 

This code requires a refrence to the close button's BrowserTabViewItem, I do not believe this should be passed into the RolloverImageButton, insted RolloverImageButton will have a addTrackingRectInView message that will take the rectangle made from the code above as a param. And as I specified before, because this code appears twice it should be in its own function in TabButtonCell and thus will not be cluttered.

Comment 30

[Stuart Morgan]

(In reply to comment #29)
> NSSize textSize = [mTabViewItem sizeOfLabel:NO];
> float maxHeight = textSize.height > closeButtonSize.height ? textSize.height :
> closeButtonSize.height;
>  NSRect closeButtonTrackingRect = NSMakeRect(trackingRect.origin.x +
> kTabLeftMargin,
> trackingRect.origin.y + (int)((trackingRect.size.height - kTabBottomPad -
> maxHeight)/2.0 + kTabBottomPad),
>  closeButtonSize.width, closeButtonSize.height);

I'm not seeing what the purpose of this code is.  The close button should be tracking mouseover of its own frame, which doesn't depend at all on the text in the tab.

Comment 31

[Aaron Schulman]

> I'm not seeing what the purpose of this code is.  The close button should be
> tracking mouseover of its own frame, which doesn't depend at all on the text in
> the tab.
> 

The problem is, when the TabButtonCell's tracking rectangle is being added, the close button does not have its frame set yet. It is in TabButtonCell's Drawing routine where the close button sets its frame. The reason for this is because the TabButtonCell's position in its superview may change by moving tabs, or selecting a tab and we only care about updating the close button to reflect this change inside of the drawing routine (at that point we must know where to put it).

So I guess technically I could force a display every time a TabButtonCell's tracking rectangle is added and thus I would ensure the cluse button has the correct frame set before I add the tracking rect in RolloverTrackingCell.
Or, I also could just use the same instructions that create the close button's frame in the display routine (the code in Comment #29).

Also to respond to your previous comment, the reason you need a refrence to the textBox inside the TabButtonCell when constructing the close button's frame is because you want to center the close button and text box to the same point, and that point should be  based on 1/2 the height of the larger object.

Again this code was written by Geoff and I may have some trouble understanding what he was doing here, but this seems to be the jist of what is going on. 

Comment 32

[Stuart Morgan]

Right, that code is to compute a new location for the button.  But once the button is moved, you still just want to track the new frame of the button.

All the button needs to do is reset its tracking rect in setFrame:/setBounds:.  That guarantees that whatever is done to it, it is always tracking its own frame correctly.  The details of how other things are computing its position shouldn't matter to the button class, otherwise it won't be at all reusable.

Comment 33

[Aaron Schulman]

(In reply to comment #12)
> Proposed Close Button Graphic:
> 
> This is similar to safari:
> ButtonUp: grey and white X
> ButtonMouseOver: dark grey and white X
> ButtonDown: dark grey and grey X
> 
> Thanks,
> Aaron Schulman
> 

I want to revise this post. I made a mistake here...

ButtonUp should be...   light grey and grey X (the same as what the Camino close button is now). I do not need any extra work from Jasper though because he included this image in his attachment as an extra image.

Also, I am done the implementation of the new class and will post it tonight.

Aaron

Comment 34

[Aaron Schulman]

Attachment: RolloverImageButton Patch

This is the patch to the source tree for the addition of the class RolloverImageButton

Comment 35

[Aaron Schulman]

Attachment: RolloverImageButton Class

These are the source files for the class RolloverImageButton

Comment 36

[Stuart Morgan]

Comment on attachment 220363 [details]
RolloverImageButton Class

This looks a whole lot cleaner. Some specifics, starting with the meatier stuff:

>    mUpImage = pUpImage;
>    mHoverImage = pHoverImage;

These need to be retained; you can't assume in general that someone else is going to retain them for you.

>    mDownImage = pDownImage;

You don't actually need to track this image yourself; just pass it through to the super setAlternateImage:.

>  mTrackingTag = [[self superview] addTrackingRect:pFrame owner:self userData:nil assumeInside:mouseInside];

There's no guarantee that pFrame == [self frame] at this point, so use the latter.

>-(void)setUpImage:(NSImage*)pImage
>-(void)setDownImage:(NSImage*)pImage
>-(void)setHoverImage:(NSImage*)pImage

The implementations of these methods don't work correctly. setDownImage: will never have any effect, and the others only take effect the next time the frame is changed. I'm also not sure about the names; this gives 5 different image-setter methods for this class. setUpImage: should be changed to a setImage: override, since that's a clear enough name for the default image, and eliminates redundancy.  I'm also leaning toward the elimination of setDownImage since it would just be a passthrough to setAlternateImage (the name certainly isn't ideal, but anyone who knows the NSButton API will understand what it does).

setImage: and setHoverImage: will need to update the parent class's image if necessary, which means you'll either need to keep the mouse in/out state all the time, or do a hit-test here as well (the latter is probably better, since changing the image will presumably be rare).

>    [[self superview] removeTrackingRect:mTrackingTag];

mTrackingTag should be reset to -1 whenever you do this. Perhaps it warrants a small method that does both to make sure it's not forgotten in any later changes.

>-(void)updateImage:(BOOL)pIsInside
>  ...
>  [self setNeedsDisplay:YES];

I wasn't thinking before--the setNeedsDisplay: is unnecessary, since setting an NSButton's image will already make it dirty.

>  bool mMouseInside;

>    mMouseInside = false;

These should go (unless you expand their use, as mentioned above).

>-(void)setFrame:(NSRect)pFrame

Because the tab code currently has some incredibly inefficient behaviors (for example, calling setFrame: on this button every single time it's drawn) I think it's worth optimizing away this work when possible.  Doing an NSEqualRects check on the current frame and the target frame, then doing nothing but calling super if they are equal, will prevent a lot of unnecessary work (resetting the tracking rect anytime someone mouses over a tab, for example). Obviously the tab code should be cleaned up too, but it's still good to get the easy win here.


Style notes (some of these are Camino-specific styles that are not followed as well as they should be, but it's worth keeping new code as clean as possible):

RolloverImageButton.mm should just be RolloverImageButton.m; it's not Obj-C++

>-(id)initWithFrameAndImages:(NSRect)frame:(NSImage*)pUpImage:(NSImage*)pDownImage:(NSImage*)pHoverImage;

Named parameters are our friends.  initWithFrameAndImages:::: isn't very descriptive.

If you want to mark parameters 'in' is the standard Camino prefix (rather than 'p').

Also, for all methods (declaration and implmentation), the preferred style is for a space between the -/+ and the (type).

>-(void)setFrame:(NSRect)frame;
>-(void)mouseEntered:(NSEvent*)theEvent;
>-(void)mouseExited:(NSEvent*)theEvent;
>-(void)updateImage:(BOOL)pIsInside;

No need for these in the header.  setFrame: is already part of the inherited API, and the others aren't meant for use by clients of this class.  updateImage: should be declared in the implementation file in a Private category.

>  if ((self = [super initWithFrame:frame])) 
>  {

All conditionals and loops should have the brace on the same line.

>  BOOL mouseInside = NSMouseInRect(mousePointInView,pFrame,NO);

Spaces after commas.

>+  RolloverImageButton*         mCloseButton;       // STRING ref

As long as you are touching this line anyway, it's a good time to fix the typo (STRING -> STRONG)

>-    sCloseIconPressed = [[NSImage imageNamed:@"tab_close_pressed"] retain];
>+    sCloseIconPressed = [[NSImage imageNamed:@"tab_close_down"] retain];

pressed is a better name; better to just use the new image with the old name.

>+	sCloseIconHover = [[NSImage imageNamed:@"tab_close_over"] retain];

'over' -> 'hover' for consistency with the name of the tab_hover graphic.  Also, spaces instead of a tab.

>+-(void)mouseEntered:(NSEvent *)theEvent;
>+-(void)mouseExited:(NSEvent *)theEvent;

As above, no need to add these to headers.


Overall the design is good, and the patch well worked in my tests, so despite the somewhat long comments it's almost there.

Comment 37

[Aaron Schulman]

Attachment: RolloverImageButton Patch

Comment 38

[Aaron Schulman]

Attachment: RolloverImageButton Class

Comment 39

[Aaron Schulman]

Attachment: Close Icon States

Comment 40

[Aaron Schulman]

(In reply to comment #38)
> Created an attachment (id=220423) [edit]
> RolloverImageButton Class
> 

There is a problem with this patch, If you click and drag on a close button it does not change to the normal image when your mouse leaves the tracking area.

Aaron S.

Comment 41

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: Drag, Responder)

This is the class with fixes for the Dragging issue and only responds to hover events when the window is key and view acceptsFirstResponder (follows from what RolloverTrackingCell does).

Enjoy!

Aaron S.

Comment 42

[Stuart Morgan]

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

This part looks good. New notes on the rest forthcoming.

Comment 43

[Stuart Morgan]

Comment on attachment 220469 [details]
RolloverImageButton Class (Fixed: Drag, Responder)

Definitely getting close here.  Good catch on the drag and key issues.

>  if (!NSEqualRects(inFrame, [self frame])) {
>    [super setFrame:inFrame];

This isn't quite right (nor is the way I was thinking about it before, actually).  What you really want to do is:
  NSRect oldFrame = [self frame];
  [super setFrame:inFrame];
  if (!NSEqualRects(oldFrame, [self frame])) {
    ...
that way super has a chance to do whatever it wants to do, and the check is based on the actual new frame, instead of the suggested new frame.

if ([[self window] isKeyWindow] || [[self superview] acceptsFirstMouse:theEvent]) {

The second part of this should really just be [self acceptsFirstMouse:theEvent]. (This will always return no for this class, but makes it easier for someone to make a subclass that does track for click-through). Which brings me to:

This class should override acceptsFirstMouse: to return NO always. Since it's a destructive button, it shouldn't allow click-through. (It's a general class, but since there has to some default behavior it may as well be for the currently intended use).

Also, as discussed on irc, the button will need to register for notifications of windows becoming key/non-key to update if windows are changed while someone is hovering.  (Be sure to note that you'll get notifications for *all* windows, so you'll need to check against [self window])

>  if (mImage)
>    [mImage release];
>  mImage = [inImage retain];

The if is unnecessary; messaging nil is safe.  However, this setter isn't; you want
  if (mImage != inImage) {
    [mImage release];
    mImage = [inImage retain];
  }
or you can construct cases where the setter will crash.

>// this is needed because part of the superview will appear malformed after button redraws unless it is set for update
>[[self superview] setNeedsDisplay:YES];

From my testing and from talking to Aaron on irc, it looks like this is a side-effect of bug 336216, with the button's region being painted correctly while the tab's stays unpainted.  It certainly shouldn't be necessary, so I think it should be left out, and the glitch fixed in the tab view.  I had a pretty hard time getting it to happen, so it shouldn't be a big deal in the short time it will take Aaron to fix that once this is in ;)

Otherwise, it looks good.  I think one more round should do it.

Comment 44

[Aaron Schulman]

I am almost done the last part of this class, but there is one issue remaining to be resolved. If the camino window is key but it is made key by clicking and holding while the mouse is inside a close button what should we do. 

When the window is made key I check to see if the mouse is inside the close button then set the state of the button to be hovering if it inside. the problem arises you move the mouse outside of the close button's tracking rect while still dragging. I am not sure what View exactly is being dragged on in this instance but the close button will remain in the hover state because it does not respond to dragging events.

The only fix i can think of in this case is to write a full dragging response implementation (my little fix of setting the close button's image to the normal image and let the NSButton code take care of setting the alternate image on dragging won't work). This is ok with me but I wanted to see if there was a better idea out there. 

Thanks, Aaron

Comment 45

[Stuart Morgan]

(In reply to comment #44)
> If the camino window is key but it is made key by clicking and
> holding while the mouse is inside a close button what should we do. 

Pop up a dialog telling the user not to active their background windows by trying to drag destructive controls that don't move? ;)

That seems like an edgy enough edge case that I'd be fine with ignoring that for now and filing it as a minor polish bug for later. You could do the whole drag implementation now if you like, but there are plenty of other places in the UI that could use the polish more.

Comment 46

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: Key Window)

Enjoy

Comment 47

[Stuart Morgan]

Comment on attachment 220745 [details]
RolloverImageButton Class (Fixed: Key Window)

Okay, I lied--just one more small round.

>  if (mImage != inImage)
>    [mImage release];
>  mImage = [inImage retain];
>  // set the image based on where the mouse currently is
>  [self updateImage:[self isMouseInside]];

This will over-retain if the images are the same.  The entire contents of the setter should be conditionalized on inequality (anytime you have a code path where you change a value that was retained without always releasing the old one it should be a red flag).  You can also get rid of the comment here, since the code it refers to is self-documenting (the same goes for the comments in mouseEntered:, mouseExited:, and acceptsFirstMouse:, in fact). 

Since you are respinning anyway, also axe the blank line in initWithFrame:

Otherwise, looks great!

Comment 48

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: setImage & setHoverImage)

Woops, can't believe I missed that

Here we go, this could be it :)

Aaron

Comment 49

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: comments)

Oy, forgot to fix the comments.

one more time!

Aaron

Comment 50

[Stuart Morgan]

Comment on attachment 220781 [details]
RolloverImageButton Class (Fixed: comments)

Looks great; r=me. Thanks for your patience!

Comment 51

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

Comment on attachment 220781 [details]
RolloverImageButton Class (Fixed: comments)

Targeting pink for now since he thinks he'll have time for some sr's this week.

Comment 52

[Mike Pinkerton (not reading bugmail)]

 * Portions created by the Initial Developer are Copyright (C) 2002
 * the Initial Developer. All Rights Reserved.

2006, not 2002.

  NSImage* mImage;
  NSImage* mHoverImage;
  NSTrackingRectTag mTrackingTag;

shouldn't these be @private in the header? Also you should mention that |mImage| and |mHoverImage| are strong (owning) references.

what happens when the grandparent's frame changes or it's removed from the window? We aren't removed from our superview, nor does our view move, so we'll end up with the wrong tracking rect.

Comment 53

[Aaron Schulman]

(In reply to comment #52)
>  * Portions created by the Initial Developer are Copyright (C) 2002
>  * the Initial Developer. All Rights Reserved.
> 
> 2006, not 2002.

Will fix, Duh. I'm dumb

> 
>   NSImage* mImage;
>   NSImage* mHoverImage;
>   NSTrackingRectTag mTrackingTag;
> 
> shouldn't these be @private in the header? Also you should mention that
> |mImage| and |mHoverImage| are strong (owning) references.
> 

I agree, will fix

> what happens when the grandparent's frame changes or it's removed from the
> window? We aren't removed from our superview, nor does our view move, so we'll
> end up with the wrong tracking rect.
> 

Will fix window remove issue, but in order to fix the other issue, I would have to go through every superview of the current view and set them to send frame change notifications. Then I would handle these notifications in the RolloverImageButton class. Is this too much for a grandchild to do, to demand all parent views to notify when their frame changes?

Also, it has come to my attention that there are several functions that can change the frame of a NSView (setFrameOrigin etc.) I will add updating of the tracking rect on the reception of these messages as well.

Comment 54

[Mike Pinkerton (not reading bugmail)]

what i had to do in other apps was to override |-NSView resetCursorRects| because there was no way to guarantee that the view didn't leave the window or move if it's grandparent was acted on.

when resetCursorRects is called, just blow away our tracking rect and reset it. It's extra overhead, but it works.

Comment 55

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: comments, TrackRect change, many setFrames)

This includes the fixes from pinkerton. Also I added the ability to change tracking rectangles when setFrameOrigin and setFrameSize are called.

Comment 56

[Simon Fraser [no longer active]]

(In reply to comment #54)
> what i had to do in other apps was to override |-NSView resetCursorRects|
> because there was no way to guarantee that the view didn't leave the window or
> move if it's grandparent was acted on.

-resetCursorRects is where you are supposed to set up your cursor rects  :)

Comment 57

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: trackRect Change, mouseUp image)

Ok guys here is the deal with this one.

First, at the begining of viewDidMoveToWindow I added a line that updates the tracking rectanges (this handles the case where the button moves windows)

Second, the use of resetCursorRects is benifical to RolloverImageButton because this message is sent to the class when any view above it in the view heiarchy makes a change to its frame. This allows us to deal with the issue pink brought up about updating tracking rectangles based on the grandfather. I know this works because I made a simple test program that had a RolloverImageButton inside a view that was inside another view that was inside the main window view. Then I changed the frame of the subview of the window's view and the close button's tracking rect would only follow this change if resetCursorRects was set to update the tracking rectangles. 

Third: While working on this I realized one final issue (hopefully). This class was made with the assumption that it would be used with a destructive button and thus you would never have to deal with the case when the button would have to reset to the hover image if you click on the button then leave the mouse inside the frame and let go. I took care of this by adding a quick image update based on mouse location following the return of NSButton's mouseDown message. As a quick aside, this needed to be done at this point because NSButton's mouseDown eats the mouseUp message.

Fiew!

I think this may be one of the last updates needed :)

Comment 58

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 221545 [details]
RolloverImageButton Class (Fixed: trackRect Change, mouseUp image)

can we get another review on this before i sr it?

Comment 59

[Stuart Morgan]

Comment on attachment 221545 [details]
RolloverImageButton Class (Fixed: trackRect Change, mouseUp image)

Still looks good to me, and works great.  Just two nits:

>  @private NSImage* mImage;               //STRONG Refrence
>  @private NSImage* mHoverImage;          //STRONG Refrence
>  @private NSTrackingRectTag mTrackingTag;

@private/@public/@protected set the visibility of everything until the next of those directives; the normal way of writing it is:

  @private

    NSImage* mImage;               //STRONG Refrence
    NSImage* mHoverImage;          //STRONG Refrence
    NSTrackingRectTag mTrackingTag;

>  if ([[self window] isKeyWindow] || [self acceptsFirstMouse:theEvent]) {
>    if ([theEvent trackingNumber] == mTrackingTag) 
>      [self updateImage:YES];
>  }

if foo
  if bar
    baz

is somewhat confusing style.  Since there is no action for any other case, just do:

  if (([[self window] isKeyWindow] || [self acceptsFirstMouse:theEvent]) &&
      ([theEvent trackingNumber] == mTrackingTag))


r=me with or without those changes though; they are just minor style and don't actually affect anything.

Comment 60

[Aaron Schulman]

Attachment: RolloverImageButton Class (Fixed: Style)

Per Stuart's post, fixed style issues

Comment 61

[Håkan Waara]

Comment on attachment 221787 [details]
RolloverImageButton Class (Fixed: Style)

r=smorgan
targetting pink for sr.

Comment 62

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 221787 [details]
RolloverImageButton Class (Fixed: Style)

looks good. sr=pink

Comment 63

[Håkan Waara]

Checked in on trunk. 

In the physical circumstances of cvs.mozilla.org, my cvs-commit tool broke down, so I couldn't check in on branch.

Comment 64

[Håkan Waara]

With some help from Smokey, this got in on branch too.

Thanks Aaron for the patch!

Comment 65

[[e-mail]]

Verified on 1.8 branch and trunk