Closed Bug 280159 Opened 18 years ago Closed 18 years ago

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

Categories

(Toolkit :: Find Toolbar, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8final

People

(Reporter: aaronlev, Assigned: Gavin)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

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_
Summary: Hightlght button's shortcut not discoverable for key/voice only users → Highlight button's shortcut not discoverable for key/voice only users
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.
Keywords: helpwanted
Priority: -- → P3
Flags: blocking-aviary1.1?
Priority: P3 → P2
Priority: P2 → P3
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Assignee: firefox → gavin.sharp
Keywords: helpwanted
Hardware: PC → All
Target Milestone: --- → Firefox1.1
Version: unspecified → Trunk
Attachment #177997 - Flags: review?(mconnor)
Comment on attachment 177997 [details] [diff] [review]
Add accesskeys for findbar buttons

Oops, wrong patch.
Attachment #177997 - Attachment is obsolete: true
Attachment #177997 - Flags: review?(mconnor)
Attachment #177998 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Whiteboard: [patch-r?]
Mconnor appears to be busy, should we try vlad?
Attachment #177998 - Attachment is obsolete: true
Attachment #177998 - Flags: review?(mconnor)
unbitrotted
Attachment #179635 - Flags: review?(mconnor)
(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.
Blocks: 264349, 269766
Attached patch Remove Ctrl+Enter (obsolete) — Splinter Review
Attachment #179769 - Flags: review?(mconnor)
Together these should take care of bug 264349 and bug 269766 as well.
Attachment #179635 - Flags: review?(mconnor) → review?(neil.parkwaycc.co.uk)
Attachment #179769 - Flags: review?(mconnor) → review?(neil.parkwaycc.co.uk)
Blocks: deera11y
Attachment #179769 - Flags: review?(neil.parkwaycc.co.uk) → review?(mconnor)
Attachment #179635 - Flags: review?(neil.parkwaycc.co.uk) → review?(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.
Attachment #179635 - Flags: review?(mconnor) → review+
Attachment #179769 - Flags: review?(mconnor) → review+
these should probably be combined into a single patch before requesting
approval, since they might clash a little depending on patch.
Attached patch Both togetherSplinter Review
Attachment #179635 - Attachment is obsolete: true
Attachment #179769 - Attachment is obsolete: true
Whiteboard: [patch-r?] → [patch-r+][checkin needed]
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?
Attachment #183099 - Flags: approval-aviary1.1a1?
Comment on attachment 183099 [details] [diff] [review]
Both together

a=mkaply
Attachment #183099 - Flags: approval-aviary1.1a1? → approval-aviary1.1a1+
Whiteboard: [patch-r+][checkin needed] → [patch-r+][checkin needed][a+]
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
> and there's no way to go back and fix it.

Sure there is.  See manual for the "admin" cvs command.
Whiteboard: [patch-r+][checkin needed][a+]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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. ***
No longer blocks: 264349
Summary: Highlight button's shortcut not discoverable for key/voice only users → Highlight button's shortcut not discoverable for key/voice only users (add Alt+A in addtion to Ctrl+Enter)
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.