589592 - [SeaMonkey 2.1, mochitest-browser-chrome] Double id='addBookmarkKb' detected in navigator.xul

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

This probably comes from places bookmarks:

Double id='addBookmarkKb' detected in chrome://navigator/content/navigator.xul:
  Tree 0:
    key addBookmarkKb [39]
    keyset navKeys [24]
    window main-window [25]
    #document undefined [0]
  Tree 1:
    key addBookmarkKb [9]
    keyset navigationKeys [48]
    keyset navKeys [24]
    window main-window [25]
    #document undefined [0]
ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/suite/common/tests/chrome/test_idcheck.xul | check id: navigator.xul#addBookmarkKb


Thanks to Callek for finding that one!

Comment 1

[Robert Kaiser]

http://mxr.mozilla.org/comm-central/search?string=addBookmarkKb tells a pretty good story. I guess we should kill it from platformNavigationBindings.xul or navigatorOverlay.xul - but as it's only on the former in non-Linux, I guess that's what we should change.

Comment 2

[Robert Kaiser]

Attachment: remove platform-specific keys

We have accel,shift in use for tabs already now, so this is a conflict as well. The best thing is to remove the platform-specific bindings, from what I'm seeing.

Comment 3

[[e-mail]]

Comment on attachment 468166 [details] [diff] [review]
remove platform-specific keys

Fortunately the Ctrl+Shift+D shortcut is no longer reserved by GTK. (Sadly you're now trying to use the reserved Ctrl+Alt+D shortcut. Doesn't anyone read the Error Console any more? I really need to review more of those Places patches...)

Comment 4

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/64ad57c8da27

(In reply to comment #3)
> (Sadly
> you're now trying to use the reserved Ctrl+Alt+D shortcut. Doesn't anyone read
> the Error Console any more? I really need to review more of those Places
> patches...)

I read it but didn't find a different combination to use for something I wasn't sure if it would even stick. And actually, it might be a good idea to just remove that option when the fast bookmarking button lands. Though I wonder if FF has a shortcut for that one...

Comment 5

[[e-mail]]

(In reply to comment #4)
> And actually, it might be a good idea to just
> remove that option when the fast bookmarking button lands.
The fast bookmarking button doesn't help keyboard users, who are the people interested in the shortcut...

Comment 6

[Robert Kaiser]

(In reply to comment #5)
> (In reply to comment #4)
> > And actually, it might be a good idea to just
> > remove that option when the fast bookmarking button lands.
> The fast bookmarking button doesn't help keyboard users, who are the people
> interested in the shortcut...

Well, the question is if they need shortcuts for both "add bookmark without being prompted" and "add bookmark with an add bookmark panel showing".

Comment 7

[Justin Wood (:Callek)]

I'd say keyboard shortcut for "Add Bookmark" should trigger fast path, second invoke of keyboard shortcut should prompt (also allowing remove).

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> I'd say keyboard shortcut for "Add Bookmark" should trigger fast path, second
> invoke of keyboard shortcut should prompt (also allowing remove).

This basically means putting the shortcut on the icon and removing the one on the "..." menu item. I also think the other menu item for simple adding should go away. Still, let's put this on a different bug.