243413 - Bookmarks sidebar doesn't open/close on Ctrl-B anymore

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Steffen Wilberg]

mkaply has stolen the keybinding on windows (bug 239113). Didn't notice this
until now because I was on Linux all the time.

I want my Ctrl-B back!

Comment 1

[Steffen Wilberg]

Attachment: stop the thief

Comment 2

[Steffen Wilberg]

Comment on attachment 148312 [details] [diff] [review]
stop the thief

Mike, can you review this? This restores the bookmarks sidebar keybindings to
the previous settings.

That is, Ctrl-B on all platforms, Ctrl-I additionally on Windows.

Comment 3

[Martin]

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

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 148312 [details] [diff] [review]
stop the thief

Comment should be Cmd+I, really, not sure why you're changing the formatting
here, but please make the comment accurate if you're going to play with it. 
I'd rather just move the keybinding out of the #ifdef and be done with it.

Comment 5

[Steffen Wilberg]

Attachment: it's Cmd+I

You're right, it's Cmd+I. I liked it better to keep in one line, and I just had
to remove that trailing whitespace...

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 148329 [details] [diff] [review]
it's Cmd+I

I don't like the comment style change, but I'll decide what to check in when I
land this :)

r=[e-mail]

Comment 7

[Bernard Alleysson]

It sure will confuse Windows users, Ctrl+B is used for ages

Comment 8

[Martin]

> It sure will confuse Windows users, Ctrl+B is used for ages

Are you seriously considering giving up Ctrl-B? «B» stands for bookmark, doesn't it?

Comment 9

[Steffen Wilberg]

(In reply to comment #8)
> > It sure will confuse Windows users, Ctrl+B is used for ages
> Are you seriously considering giving up Ctrl-B?
Just the opposite. He says it will confuse Windows users if Ctrl-B isn't
reintroduced. This bug is about reintroducing Ctrl-B for Windows. On the current
trunk and branch, only Ctrl-I works.
The discussion above (about Ctrl-I/Cmd-I) was just about changing a comment.

Mike, this needs to be fixed on the branch as well:
http://bonsai.mozilla.org/cvsquery.cgi?branch=&date=explicit&mindate=2004-04-27+08%3A53&maxdate=2004-04-27+08%3A54
Rip out that comment change, if you don't like it.

Comment 10

[Mike Connor [:mconnor]]

checked in branch and trunk 2004-05-16 08:17

Comment 11

[Steffen Wilberg]

Thanks Mike.
You checked this in the trunk and the MOZILLA_1_7_BRANCH, but the
AVIARY_1_0_20040515_BRANCH has been created before (on 2004-05-15 22:10).
Will Firefox 0.9 and 1.0 be based on that branch?

Comment 12

[Dorando]

There will be now two <key>s with the same id on Windows builds, is that intended?

Comment 13

[Steffen Wilberg]

The keys had the same id "viewBookmarksSidebarKb" before mkaply's change, and
they have now. Both keys work.
But I just noticed that in the View->Sidebar menu, Ctrl-I is displayed instead
of Ctrl-B, because the key with Ctrl-I (manBookmarksWinCmd.commandkey) is
defined before the key with Ctrl-B (manBookmarksCmd.commandkey). We might want
to change that back.
-> Reopening. And removing "fixed0.9", since the checkin didn't hit the
aviary_1_0 branch.

Comment 14

[Mike Connor [:mconnor]]

yeah, I should get to that as soon as my ISP stops sucking

yeah, we should move the ctrl-b binding in front, I'll fix that when I land on 
the branch (and on the trunk)

Comment 15

[Dorando]

(In reply to comment #13)
> The keys had the same id "viewBookmarksSidebarKb" before mkaply's change, and
> they have now.
Still, was this intended or just an oversight (which mkaply might have also
thought) as an id is supposed to be unique within any given document?

> Both keys work.
Yes they do, but anything trying to access it via getElementById will have a
hard time.

> But I just noticed that in the View->Sidebar menu, Ctrl-I is displayed instead
> of Ctrl-B, because the key with Ctrl-I (manBookmarksWinCmd.commandkey) is
> defined before the key with Ctrl-B (manBookmarksCmd.commandkey). We might want
> to change that back.
Could likely be fixed by changing the id of the Win specific key to
"viewBookmarksSidebarWinKb" in accordance to its key entity.

Comment 16

[Steffen Wilberg]

> Still, was this intended or just an oversight (which mkaply might have also
> thought) as an id is supposed to be unique within any given document?
No, this wasn't intended. I just wanted to restore the ctrl-b keybinding, so I
looked what mkaply did and more or less reversed it.

> Yes they do, but anything trying to access it via getElementById will have a
> hard time.
True. But not a problem here, except that we have to specify the keys in the
right order.

> > But I just noticed that in the View->Sidebar menu, Ctrl-I is displayed
> > instead of Ctrl-B, because the key with Ctrl-I
> > (manBookmarksWinCmd.commandkey) is defined before the key with Ctrl-B
> > (manBookmarksCmd.commandkey). We might want to change that back.
> Could likely be fixed by changing the id of the Win specific key to
> "viewBookmarksSidebarWinKb" in accordance to its key entity.
True. But it's easier to move the ctrl-b keybinding up front. ;-)

Comment 17

[Dorando]

(In reply to comment #16)
> > Yes they do, but anything trying to access it via getElementById will have a
> > hard time.
> True. But not a problem here, except that we have to specify the keys in the
> right order.
Actually it is a problem since http://mozilla.dorando.at/keyconfig.xpi won't be
able to change this key...

Comment 18

[Mike Connor [:mconnor]]

checked in on the aviary branch, universal keybinding first.

split off another bug about the duplicate IDs :)

Comment 19

[Steffen Wilberg]

Thanks Mike.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The way this was landed looks like it backed out part of bug 239113.

Comment 21

[Steffen Wilberg]

Indeed. Is anything wrong with that?
We used to have, and now again have Ctrl+B (B like in Bookmarks) on all
platforms, and additionally Ctrl+I (for IE compatibility) on Windows. That was
the case before mkaply's checkin; he removed Ctrl+B on Windows in bug 239113,
which we didn't like, so we restored it with this bug.

Trunk:
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser-sets.inc#250

Avary branch:
http://lxr.mozilla.org/aviarybranch/source/browser/base/content/browser-sets.inc#250

Comment 22

[Steffen Wilberg]

As you can see with the links I provided above, the only difference between
trunk and aviary branch is the comment. The patch attached to this bug isn't
exactly what was landed.

Comment 23

[Mike Kaply [:mkaply]]

Is having two keys with the same ID valid? I assumed that the second <key> with
the same id as the first would override the first...

Comment 24

[Steffen Wilberg]

Well, sort of valid, it works... :)

Actually the first key takes preference. The first one (Ctrl-B) is displayed in
Menu View->Sidebar->Bookmarks on all platforms.

The only problem is, which has been pointed out in comment 15 and comment 17,
that a key with duplicate ids can't be accessed. So we should give the
windows-specific keybinding a unique id, changing

<key id="viewBookmarksSidebarKb" key="&manBookmarksWinCmd.commandkey;" ...
to something like
<key id="viewBookmarksSidebarKbWin" key="&manBookmarksWinCmd.commandkey;"

Feel free to change it if you like. I can't do it, as I have no cvs account yet.
Maybe you could also make the comments identical on branch and trunk, while
you're at it?

Comment 25

[Steffen Wilberg]

Er, rather viewBookmarksSidebarWinKb.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: better merging?

This is how I think you should have merged it.

(If this is supposed to be an XML file, I'll note that it should be illegal to
have two elements with the same id (assuming an appropriate DTD).)

Comment 27

[Steffen Wilberg]

David, we want to have Ctrl-B on all platforms _and_ Ctrl-I on Windows. The
latter is for IE compatibility, the former is the key we've always used and want
to use in the future. That's what this bug was about: mkaply removed Ctrl-B on
windows, and I wanted to restore it.

browser-sets.inc is indeed part of an XML file, it's imported into browser.xul
and hiddenWindow.xul (for Mac). Having identical ids for different keys leads to
the problems mentioned in comment 15 and comment 17. I already told Jesse to
give the windows key a separate id, see bug 250058.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

ok, never mind me

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Now that I understand -- the thing that confused me in the first place was that
this patch was landed differently on the 1.7 branch from how it was landed on
the trunk and the aviary 1.0 branch.  In the future, please try to land exactly
the same patch when landing on multiple branches.  (It makes a whole bunch of
things easier, such as merging and landing future patches.)

Comment 30

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change