Last Comment Bug 300304 - Bookmarks Bar buttons should darken text on mousesdown
: Bookmarks Bar buttons should darken text on mousesdown
Status: RESOLVED FIXED
[Good First Bug]
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: unspecified
: PowerPC Mac OS X
-- enhancement (vote)
: Camino1.5
Assigned To: Stuart Morgan
:
:
Mentors:
Depends on: 359218
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-10 16:40 PDT by Andrew Collins
Modified: 2006-11-14 15:28 PST (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (5.16 KB, patch)
2006-10-21 22:28 PDT, Stuart Morgan
sfraser_bugs: review-
Details | Diff | Splinter Review
with SDK workaround (5.60 KB, patch)
2006-10-22 12:04 PDT, Stuart Morgan
no flags Details | Diff | Splinter Review
v3 (5.60 KB, patch)
2006-10-23 19:59 PDT, Stuart Morgan
sfraser_bugs: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Andrew Collins 2005-07-10 16:40:59 PDT
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 User image Mike Pinkerton (not reading bugmail) 2005-07-12 06:37:57 PDT
i say WONTFIX, what say you simon?
Comment 2 User image Simon Fraser 2005-07-12 09:20:50 PDT
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 User image Wevah 2005-07-12 14:11:31 PDT
(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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-07-26 23:35:45 PDT
Confirm this for improving shadows on BM bar text for clicks only?
Comment 5 User image Andrew Reynhout 2006-04-18 17:05:50 PDT
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 User image Samuel Sidler (old account; do not CC) 2006-05-10 09:16:58 PDT
(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 User image froodian (Ian Leue) 2006-05-14 20:29:04 PDT
->aschulm per IRC, since he likes this stuff.
Comment 8 User image Stuart Morgan 2006-10-21 22:28:12 PDT
Created attachment 243067 [details] [diff] [review]
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 User image Stuart Morgan 2006-10-22 12:04:42 PDT
Created attachment 243123 [details] [diff] [review]
with SDK workaround

Should actually build with the current setup.
Comment 10 User image Simon Fraser 2006-10-23 09:21:34 PDT
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 User image Stuart Morgan 2006-10-23 19:59:30 PDT
Created attachment 243284 [details] [diff] [review]
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 User image Mike Pinkerton (not reading bugmail) 2006-11-11 10:41:30 PST
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 User image Stuart Morgan 2006-11-11 12:50:27 PST
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 User image Stuart Morgan 2006-11-11 14:54:38 PST
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 User image Stuart Morgan 2006-11-14 15:18:49 PST
Checked in on MOZILLA_1_8_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.