340099 - Give bookmark "mouse down" appearance when using cmd-1 through 9

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[froodian (Ian Leue)]

In order to make the new Cmd-1 through 9 shortcuts more discoverable (and a bit shinier), we ought to momentarily give the bookmark button being invoked the mouse-down appearance when the shortcut is invoked.  This would be similar to the way menus momentarily darken when invoking a keyboard shortcut for items in them. STR:

1. Put at least one item in the bookmarks bar.
2. Cmd-1

What happens: bookmark mysteriously (if you didn't know about the feature) opens
What should happen: bookmark opens and favicon (and in future text) darkens momentarily

Comment 1

[Chris Lawson (gone)]

Safari doesn't do this, FWIW.

Whether that's a bug or a feature is arguable. :)

cl

Comment 2

[froodian (Ian Leue)]

Confirming as polish per IRC, and marking dependent on bug 300304.

Comment 3

[Stuart Morgan]

Attachment: fix

This does two things:
- briefly highlights the button in the toolbar when triggering it
- disables the command-key shortcuts while the bookmark bar isn't showing

Those two things together should make this behavior cause much less confusing for people who don't know about it.

Comment 4

[Stuart Morgan]

Removing dependency on bug 300304; we'll automatically pick up any changes to how the highlighting is drawn.

Comment 5

[froodian (Ian Leue)]

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


>+  unsigned int bookmarkBarCount = [bookmarkBarChildren count];
>+  unsigned int loadableItemIndex = -1;
>+  unsigned int i;
>+  BookmarkItem* item;
> 

Just a couple nits.  loadableItemIndex should be signed if we're starting at -1.  Also, can you declare i inline, to reduce pre-loop clutter if nothing else?

Other than that, looks good.  r=me with those changes.

Comment 6

[Stuart Morgan]

Attachment: v2

(In reply to comment #5)

> loadableItemIndex should be signed if we're starting at -1.

Oops, thanks.  I went through a couple versions of the loop and wasn't paying attention when I switched to -1.

> Also, can you declare i inline, to reduce pre-loop clutter if nothing
> else?

No; the point of rearranging the loop structure is that I need |i| after the loop is complete.

Comment 7

[Mike Pinkerton (not reading bugmail)]

+  // We don't want to trigger bookmark bar loads if the bookmark bar isn't visible
+  if (![mPersonalToolbar isShown])
+    return NO;

why not?

Comment 8

[Stuart Morgan]

Because otherwise people who browse without the bookmark bar will have no way of figuring out what the heck happened, and because presumably people browsing without the bookmark bar do so because they don't use the bookmark bar, and are thus really unlikely to want to trigger any items that may be on it.

Comment 9

[Mike Pinkerton (not reading bugmail)]

i would imagine a bunch of people coming from safari who are used to this behavior would hide the bookmark bar because they know they can just use the cmd keys and not have to take the vertical space and extra clutter.

Safar does still do the cmd-key equivs when the bookmark bar is hidden. I think we should too.

Comment 10

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

Given how easy it is to trigger accidentally (I hit cmd-1 accidentally all the time when trying to hit cmd-` :/) and the fact you really have to count if you have folders on the bar as well, it really seems like shooting around in the dark if we allow this to be triggered when the bar is hidden.

Comment 11

[Stuart Morgan]

Per irc, we'll go with not triggering them when it's hidden until/unless we get indication that a significant number of people actually want that functionality.

Comment 12

[Eiichi]

(In reply to comment #9)
> i would imagine a bunch of people coming from safari who are used to this
> behavior would hide the bookmark bar because they know they can just use the
> cmd keys and not have to take the vertical space and extra clutter.
> 
> Safar does still do the cmd-key equivs when the bookmark bar is hidden. I think
> we should too.
> 

 I quite agree, because I'm a PowerBook user with small display.

Comment 13

[Stuart Morgan]

Note to self; needs a respin from the bm bar api change that went in with session saving.

Comment 14

[Stuart Morgan]

Attachment: v3

Unrotted, and doing the timer callback correctly.

Comment 15

[Mike Pinkerton (not reading bugmail)]

+- (void)unhighlightButton:(NSTimer*)timer
+{
+  [(BookmarkButton*)[timer userInfo] highlight:NO];
+}

what happens if the button goes away in the brief period of time it takes for the timer to fire? Worth worrying about? what happens if you close the window, etc?

Comment 16

[Stuart Morgan]

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

(In reply to comment #15)
> what happens if the button goes away in the brief period of time it takes for
> the timer to fire?

I was thinking of NSThreads (which retain their targets) when I wrote this. I'll whip up together a new version.

Comment 17

[Stuart Morgan]

Attachment: v4

It turns out NSTimers do in fact retain their targets; the docs are just not clear about it. So this works fine. This is just v3 respun due to bitrot.

Comment 18

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 249108 [details] [diff] [review]
v4

yeah, now that i think about it, i do recall that timers retain their targets. 

sr=pink

Comment 19

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH.