280159 - Highlight button's shortcut not discoverable for key/voice only users (add Alt+A in addtion to Ctrl+Enter)

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P3)

Description

[Aaron Leventhal]

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_

Comment 1

[Aaron Leventhal]

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 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: Add accesskeys for findbar buttons

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 177997 [details] [diff] [review]
Add accesskeys for findbar buttons

Oops, wrong patch.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: Add accesskeys for findbar buttons

Comment 5

[Aaron Leventhal]

Mconnor appears to be busy, should we try vlad?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: Add accesskeys for findbar buttons

unbitrotted

Comment 7

[Michael Clackler]

(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)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 9

[Aaron Leventhal]

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.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: Remove Ctrl+Enter

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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

Comment 13

[Mike Connor [:mconnor]]

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.

Comment 14

[Mike Connor [:mconnor]]

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

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: Both together

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 183099 [details] [diff] [review]
Both together

Incidentally, this seems to work fine with FAYT.

Comment 17

[Aaron Leventhal]

(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.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(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 19

[Mike Kaply [:mkaply]]

Comment on attachment 183099 [details] [diff] [review]
Both together

a=mkaply

Comment 20

[Aaron Leventhal]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

> and there's no way to go back and fix it.

Sure there is.  See manual for the "admin" cvs command.

Comment 22

[Blake Ross]

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.

Comment 23

[Mike Connor [:mconnor]]

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.

Comment 24

[Blake Ross]

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.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

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