300304 - Bookmarks Bar buttons should darken text on mousesdown

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, enhancement)

Description

[Andrew Collins]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b3) Gecko/20050708 Camino/0.9a1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b3) Gecko/20050708 Camino/0.9a1+

Mouseovers in the Bookmarks Bar currently provide no visual feedback to the
user.  Mouse clicks only provide minimal visual feedback via the favicon or
folder icon.  Usability would be enhanced if the Bookmarks Bar provided more
feedback to the user by having a highlighted state for mouseovers and mouse clicks,

Reproducible: Always

Steps to Reproduce:
1.  Open Camino
2.  No step 2

Comment 1

[Mike Pinkerton (not reading bugmail)]

i say WONTFIX, what say you simon?

Comment 2

[Simon Fraser [no longer active]]

I don't want mouseover (since toolbar buttons don't have that), but I agree the
click feedback could be more. Maybe darken the text like toolbar buttons do?

Comment 3

[Wevah (Nate Weaver)]

(In reply to comment #2)
> I don't want mouseover (since toolbar buttons don't have that), but I agree the
> click feedback could be more. Maybe darken the text like toolbar buttons do?

The toolbar buttons actually get a small shadow under their text when clicked.
If we were 10.3+, we could use NSShadow, but there are also some CoreGraphics
calls that can accomplish the same effect on 10.2.

Comment 4

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

Confirm this for improving shadows on BM bar text for clicks only?

Comment 5

[Andrew Reynhout]

I agree with the original reporter: hover highlighting would be valuable for bookmark bar items.

Since favicons are only 16x16, there could be dozens of them, and they can move around at the user's whim, bookmark target acquisition is significantly more problematic than for the four standard toolbar buttons.  I think hover highlighting would be a definite usability improvement.

I like the way drop targets are highlighted when you drag a URL from the URL window to a folder in the bookmarks bar, although I'd imagine something more subtle for hover highlighting.

Comment 6

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

(In reply to comment #3)
> The toolbar buttons actually get a small shadow under their text when clicked.
> If we were 10.3+, we could use NSShadow, but there are also some CoreGraphics
> calls that can accomplish the same effect on 10.2.

Hey hey... we're 10.3+ now. Make it happen, yo.

Comment 7

[froodian (Ian Leue)]

->aschulm per IRC, since he likes this stuff.

Comment 8

[Stuart Morgan]

Attachment: fix

Swaps in a modified attributedTitle during highlighted drawing; this lets us get the shadow without having to custom-draw the whole cell.

Comment 9

[Stuart Morgan]

Attachment: with SDK workaround

Should actually build with the current setup.

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 243067 [details] [diff] [review]
fix


>Index: src/extensions/DraggableImageAndTextCell.mm
>===================================================================


>+- (void)dealloc
>+{
>+  [mSavedStandardTitle release];
>+}

Missing [super dealloc];


>Index: src/browser/BrowserWindowController.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserWindowController.mm,v
>retrieving revision 1.281
>diff -u -8 -r1.281 BrowserWindowController.mm
>--- src/browser/BrowserWindowController.mm	11 Oct 2006 16:42:29 -0000	1.281
>+++ src/browser/BrowserWindowController.mm	22 Oct 2006 05:25:31 -0000
>@@ -1133,16 +1133,17 @@
>     NSButton* button = [self createToolbarPopupButton:toolbarItem];
>     [toolbarItem setLabel:NSLocalizedString(@"Back", nil)];
>     [toolbarItem setPaletteLabel:NSLocalizedString(@"Go Back", nil)];
> 
>     NSSize size = NSMakeSize(32., 32.);
>     NSImage* icon = [NSImage imageNamed:@"back"];
>     [icon setScalesWhenResized:YES];
>     [button setImage:icon];
>+    [button setTitle:nil];
>     
>     [toolbarItem setView:button];
>     [toolbarItem setMinSize:size];
>     [toolbarItem setMaxSize:size];
>     
>     [button setTarget:self];
>     [button setAction:@selector(back:)];
>     [toolbarItem setTarget:self];
>@@ -1170,16 +1171,17 @@
>     NSButton* button = [self createToolbarPopupButton:toolbarItem];
>     [toolbarItem setLabel:NSLocalizedString(@"Forward", nil)];
>     [toolbarItem setPaletteLabel:NSLocalizedString(@"Go Forward", nil)];
> 
>     NSSize size = NSMakeSize(32., 32.);
>     NSImage* icon = [NSImage imageNamed:@"forward"];
>     [icon setScalesWhenResized:YES];
>     [button setImage:icon];
>+    [button setTitle:nil];

Why do you need these changes?

Comment 11

[Stuart Morgan]

Attachment: v3

reply to comment #10)
> Missing [super dealloc];

Whoops.

> Why do you need these changes?

AppKit weirdness.  The forward and back toolbar button cells are DraggableImageAndTextCells, and although their imagePositions are set to NSImageOnly, as soon as you call setAttributedTitle: (even with just [self attributedTitle]) it starts drawing the title over the buttons in the toolbar.  The buttons come with a default title of "Button", so I needed to remove it.  That wasn't a good place for the fix though; I've moved it into the init of the helper method that sets them up.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 243284 [details] [diff] [review]
v3

+#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_3
+@class NSShadow;
+#endif

still needed after the SDK switch?

sr=pink

Comment 13

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH. I left the SDK thing since branch still hasn't switched and it'll be easier to remove all that junk later if it's not different on branch and trunk.

Comment 14

[Stuart Morgan]

Clearly I shouldn't try to outsmart the 10.2.8 SDK, as it seems to be smarter than I am.  I could have sworn this was compiled against it, but apparently not. Backed out on branch until the SDK changeover.

I removed the VERSION_MAX_ALLOWED on the trunk since there will be no need for it to exist anywhere.

Comment 15

[Stuart Morgan]

Checked in on MOZILLA_1_8_BRANCH.