209886 - keyboard shortcut to open selected bookmark

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[S Woodside]

In bookmarks panel, return key opens the selected bookmark. Keyboard
accessibility, mainly.

Comment 1

[S Woodside]

Attachment: patch to BookmarksOutline view and Extended outlineView

There was already a hook in ExtendedOutlineView for mDoubleAction, so I
implemented that. First it checks that there's only 1 item selected. This
applies to history as well as long as it appears in the BookmarksOutlineView as
well.

Comment 2

[Simon Fraser [no longer active]]

->pink

Comment 3

[Mike Pinkerton (not reading bugmail)]

+-(void)setDoubleAction: (SEL)aDoubleAction;
+{
+  mDoubleAction = aDoubleAction;
+}
+
+-(SEL)doubleAction;
+{
+  return mDoubleAction;
+}
+

isn't there already a doubleAction/setDoubleAction on NSTableView (grandparent
of our outline view) that does all this for us? I think all you need is the
keyDown method. Can you try it and verify?

Comment 4

[S Woodside]

Attachment: patch BookmarksOutlineView.mm

You're right. I deleted that part and it still works.

Comment 5

[Mike Pinkerton (not reading bugmail)]

after looking at safari, we mimic their behavior which keys an inline edit. it
may even be built into the NSTableView that way. thoughts?

Comment 6

[S Woodside]

safari is wrong ? ;-)

Comment 7

[Mike Pinkerton (not reading bugmail)]

i'm really torn. return to open seems to be the most logical choice here, but
the default behavior in cocoa seems to be to edit. The finder also behaves this
way. The HIG's are silent.

Comment 8

[S Woodside]

There should be a way to open them with the keyboard whether it's return or
other keyboard shortcut.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 126921 [details] [diff] [review]
patch BookmarksOutlineView.mm

We have a new Camino request flag which can be set multiple times for a patch.
Moving old review requests to the new flag. Sorry for the spam.

Comment 10

[cannedbrain+mozilla]

In Mac OS finder (X and classic):

return key to edit name
command-down arrow to open selected items

It'll be great if Camino use the same shortcuts. For the sake of consistency (and it's a drag to 
remember all shortcuts in all different apps.) ;)

Comment 11

[Ludovic Hirlimann [:Usul]]

Simon, pink is this patch still supose to be valide since dave's new bm landed ?

Comment 12

[S Woodside]

Attachment: enter key

Uses the enter key instead of the return key.

Comment 13

[Ludovic Hirlimann [:Usul]]

Comment on attachment 135594 [details] [diff] [review]
enter key

>+      // open the bookmark
>+      if( [self numberOfSelectedRows] == 1 && [self doubleAction] )

Giving r+.
Waht about the multi selection case, the present patch does not work with that,
but double clicking on multiple selected bookmark does not work either.

Comment 14

[S Woodside]

I think that's a a good idea, I filed Bug 225915.

Comment 15

[David Haas (gone, not reading mail)]

A couple things:

1st: The other special key processing off a key down event is handled in
ExtenededOutlineView.mm.  I'd suggest moving this logic into that file, rather
than in BookmarkOutlineView.mm.  

2nd: as per comment 10: Go with the Finder's behavior on this - "return" edits
the text, "command + down arrow" opens the bookmark.

Comment 16

[David Haas (gone, not reading mail)]

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

Comment 17

[S Woodside]

comment 15 . 1
I think that it should be in BookmarkOutlineView ... why do we even have both
ExtendedOutlineView and BookmarkOutlineView anyway? Since ExtendedOutlineView is
only used by bookmarks manager ... and contains bookmarks-specific code (but
lives  in src/extensions for some reason)

comment 15 . 2
OK, I'll implement Cmd-DownArrow as well. I'll keep EnterKey because I want a
one key press option.

Comment 18

[S Woodside]

Attachment: enter key, Cmd-DownArrow, and multiple bookmarks

I added the ability to open multiple bookmarks and included it here, because
there's no way to open multiple bookmarks without the keyboard shortcut anyway.
I could split them if that's important.

Comment 19

[David Haas (gone, not reading mail)]

Separate out the "multiple bookmark" part of the patch.  It's going to be more
complicated - should things open in a new window?  in new tabs?  Those prefs
need to be checked.  Etc.  And when you do that patch, you don't need to add in
a new method to "BrowserWindowController" - just used the already existing
openTabGroup, or if it really bugs you, globally change all mentions of
openTabGroup to openURLArray.

What's left is the changes to ExtendedOutlineView.  There - itemsToOpen is
leaking. release or autorelease it.  Otherwise works as promised.

As to why ExtenedOutlineView exists - I believe the historical reason was the
split between bookmarks & history in the sidebar.  The history code still
touches it a little.  At one point I think I remember looking at it and deciding
it still made some sense, but that might not be true (or true anymore).

Comment 20

[David Haas (gone, not reading mail)]

sorry - one more thing 

+      if( [self numberOfSelectedRows] == 1 && [self doubleAction] ) {

I don't think you need the check for [self doubleAction].  

Comment 21

[S Woodside]

Attachment: enter & cmd-downarrow

Dumped the multi-bookmarks code. That'll be done in bug 225915. This is much
simpler now, I eliminated the check for just 1 element selected as well, since
it will only open one bookmark anyway ... it will be whichever is the target,
which is the last one you selected.

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 136483 [details] [diff] [review]
enter & cmd-downarrow

r=pink

i noticed that up/down arrow by themselves don't work. didn't they before?
maybe something to fix in the "open multiple" bug?

Comment 23

[Mike Pinkerton (not reading bugmail)]

landed

Comment 24

[sairuh (rarely reading bugmail)]

tested with 2005011808-trunk (10.3.7); looks great!

Return: puts selected bm in inline edit mode
Enter: opens selected bm in current page (or tab, if window has multiple tabs)
Cmd+Down arrow: opens selected bm in new tab (or window, depending on user's
pref setting)