The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Future

Status

Camino Graveyard
Toolbars & Menus
--
enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: froodian (Ian Leue), Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.2, polish})

unspecified
Future
PowerPC
Mac OS X
fixed1.8.1.2, polish

Details

Attachments

(1 attachment, 3 obsolete attachments)

v4
5.95 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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

11 years ago
Safari doesn't do this, FWIW.

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

cl
(Reporter)

Comment 2

11 years ago
Confirming as polish per IRC, and marking dependent on bug 300304.
Status: UNCONFIRMED → NEW
Depends on: 300304
Ever confirmed: true
Keywords: polish
Target Milestone: --- → Future
(Assignee)

Comment 3

11 years ago
Created attachment 244170 [details] [diff] [review]
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.
Assignee: nobody → stuart.morgan
Status: NEW → ASSIGNED
Attachment #244170 - Flags: review?
(Assignee)

Comment 4

11 years ago
Removing dependency on bug 300304; we'll automatically pick up any changes to how the highlighting is drawn.
No longer depends on: 300304
(Reporter)

Comment 5

11 years ago
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.
Attachment #244170 - Flags: review? → review+
(Assignee)

Comment 6

11 years ago
Created attachment 244191 [details] [diff] [review]
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.
Attachment #244170 - Attachment is obsolete: true
Attachment #244191 - Flags: superreview?(mikepinkerton)
+  // We don't want to trigger bookmark bar loads if the bookmark bar isn't visible
+  if (![mPersonalToolbar isShown])
+    return NO;

why not?
(Assignee)

Comment 8

11 years ago
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.
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.
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.
(Assignee)

Comment 11

11 years ago
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

11 years ago
(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.
(Assignee)

Comment 13

11 years ago
Note to self; needs a respin from the bm bar api change that went in with session saving.
(Assignee)

Comment 14

10 years ago
Created attachment 248056 [details] [diff] [review]
v3

Unrotted, and doing the timer callback correctly.
Attachment #244191 - Attachment is obsolete: true
Attachment #248056 - Flags: superreview?(mikepinkerton)
Attachment #244191 - Flags: superreview?(mikepinkerton)
+- (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?
(Assignee)

Comment 16

10 years ago
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.
Attachment #248056 - Flags: superreview?(mikepinkerton)
(Assignee)

Comment 17

10 years ago
Created attachment 249108 [details] [diff] [review]
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.
Attachment #248056 - Attachment is obsolete: true
Attachment #249108 - Flags: superreview?(mikepinkerton)
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
Attachment #249108 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 19

10 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.