The highlight button can be activated with Alt+Enter. This is discoverable via a tooltip, which mouse users can see. However it should be discoverable for the keyboard users who need it most, and consistent with Match case, which uses an underlined mnemonic. It looks like a mnemonic was avoided because there is no obvious letter to use, since h is already used by help, t for tools, and i,g,l are not ideal letters to underline. I recommend calling it Highlight all and underlining the _a_

Related: bug 264349 which is about Ctrl+Enter not working with find as you type is initiated with /. Using Alt+a (highlight _a_ll) would solve both bugs.

Comment on attachment 177997 [details] [diff] [review] Add accesskeys for findbar buttons Oops, wrong patch.

Mconnor appears to be busy, should we try vlad?

unbitrotted

(In reply to comment #6) > Created an attachment (id=179635) [edit] > Add accesskeys for findbar buttons > > unbitrotted Gavin just a question. If we're going to change to Alt+a, do we still want "(Ctrl+Enter)" displayed in the tooltip (in findbar.dtd from your latest patch): <!ENTITY highlight.tooltip "Highlight all occurrences of the phrase (Ctrl+Enter)"> It might be a little confusing to the end user to display _a_ for Alt-a on the button and then in the tooltip say Ctrl+Enter (even if the code still allows Ctrl+Enter to work, it's probably better not to show it in the tooltip)

I thought about that, but I see no reason to remove Ctrl+Enter from the tooltip. The fact that there is an additional access key is very unlikely to confuse anyone.

I think we could remove the use of Ctrl+Enter if we have Alt+A. Right now users get confused sometimes by the dual meanings of Ctrl+Enter. Using Alt+A instead this problem and that problem at the same time. Here's the bug for Ctrl+Enter: Bug 269766 "Conflicting shortcuts: Ctrl+Enter has two meanings" That can be solved in the other bug if you want though, since you already have a patch for this one up.

(In reply to comment #9) > the dual meanings of Ctrl+Enter Ah, I hadn't considered that. I'll wrap that into the patch here when I get home, its a trivial change.

Together these should take care of bug 264349 and bug 269766 as well.

Comment on attachment 179635 [details] [diff] [review] Add accesskeys for findbar buttons This doesn't even work with FAYT, which isn't good. The change to accesskeys fixes this.

these should probably be combined into a single patch before requesting approval, since they might clash a little depending on patch.

Comment on attachment 183099 [details] [diff] [review] Both together Incidentally, this seems to work fine with FAYT.

(In reply to comment #16) > (From update of attachment 183099 [details] [diff] [review] [edit]) > Incidentally, this seems to work fine with FAYT. > I think that's what Mike meant -- that the old system doesn't work with FAYT which is another reason to do this.

(In reply to comment #17) > I think that's what Mike meant -- that the old system doesn't work with FAYT > which is another reason to do this. Right, I was just confirming that this does work using FAYT. Is it worth asking for 1.1a approval on this?

Comment on attachment 183099 [details] [diff] [review] Both together a=mkaply

Thanks Gavin. I checked it in for you. Uh, I forgot to give you credit in the checkin comments, and there's no way to go back and fix it. Sorry -- didn't mean to snub you and I really appreciate the fix. Enter passphrase for key '/home/aleventhal/.ssh/id_dsa': Checking in toolkit/components/typeaheadfind/content/findBar.inc; /cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.inc,v <-- findBar.inc new revision: 1.3; previous revision: 1.2 done Checking in toolkit/components/typeaheadfind/content/findBar.js; /cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.js,v <-- findBar.js new revision: 1.8; previous revision: 1.7 done Checking in toolkit/locales/en-US/chrome/global/findbar.dtd; /cvsroot/mozilla/toolkit/locales/en-US/chrome/global/findbar.dtd,v <-- findbar.dtd new revision: 1.2; previous revision: 1.1 done

> and there's no way to go back and fix it. Sure there is. See manual for the "admin" cvs command.

I'm not happy with this change. Alt+A is ridiculously difficult to hit. Plus, we shipped 1.0 with Ctrl+Enter and users may well be accustomed to it already. Adding Alt+A to it is fine, but I'm not convinced we should remove the keybinding.

I don't think I'd agree that Alt-A is ridiculously difficult to hit, I have to move my thumb to alt, and my pinky is on A. Ctrl-Enter requires much more wrist movement in a traditional typing position. You did give a long spiel about your unconventional typing style one night, so maybe it doesn't work for you nearly as well. Ctrl-Enter has really inconsistent results when combined with FAYT, which is why it was removed. If you have a better solution that doesn't involve confusing users, I'm all ears. In any case, file a new bug on restoring Ctrl-Enter, and don't reopen this, since this was an accessibility bug that shows in the tracking hierarchy and is fixed whether or not we restore Ctrl-Enter.

Mike, I believe users are most confused when you silently remove a keybinding out from under them, and that's what was done here. There's no reason that Ctrl+Enter in the find bar should be confusing to people. It should only work when focus is in the textbox. If it doesn't, that's a separate bug that should be fixed, but the solution isn't to remove Ctrl+Enter. Alt+A is a very awkward hand position in the standard typing style. It squishes your hand together. Like I said, I'm happy to have Alt+A as an alternate method, but we already shipped with Ctrl+Enter and I think it should stand until there's a good reason to remove it. I've filed bug 294609 on myself to restore it.

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