87792 - Fix accesskeys based on new flexibility

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement)

Description

[Jeremy M. Dolan]

The wrong 's' is underlined, making it hard to notice. This is quite trivial.

Comment 1

[doctor__j]

Changing summary...

Digging out all cases:
(Pls add in some more if I've missed any item.  I'm using Win32 build.  Not sure
if Linux and Mac have the same underlined command keys.)

File menu:
1. New Navigator Window
2. New -> Navigator window
3. Save As

View menu:
1. Show/Hide -> Navigation Toolbar
2. Show/Hide -> Status Bar
3. Translate
4. Languages and Web Content -> Download More
5. Character Coding
6. Character Coding -> More -> East European
7. Character Coding -> More -> SE & SW Asian

Bookmarks menu:
1. Manage Bookmarks...

Help menu:
1. About Mozilla

Comment 2

[timeless]

Ok I'm taking this bug.  I'm fixing the summary because what was asked for 
would undo the feature which we checked in.

cmanske: a lot of these changes are probably editor so i'd like you to review 
them.

Basically mozilla now allows accesskey to prefer case when checking for a 
character in a menu item label.  Which means that while older checkins were 
random (or used lowercase) because case was meaningless, new checkins (and this 
patch) have to at least consider case. In 9/10 instances that means using the 
uppercase letter for the accesskey if it exists in the label.

Comment 3

[timeless]

Attachment: review me

Comment 4

[timeless]

please test + review.

Comment 5

[sairuh (rarely reading bugmail)]

->keybd nav.

Comment 6

[:deanis74]

Most of these changes aren't needed.  If the case-sensitive match isn't found,
it falls back to a case-insensitive match.  So specifying "n" as an accesskey
for "New" will result in "_N_ew".  The only ones that need fixing are those
where the new case-sensitive matching results in the wrong letter being
underlined.  eg:

 <!ENTITY exportToTextCmd.label "Export to Text...">
-<!ENTITY fileexporttotext.accesskey "t">
+<!ENTITY fileexporttotext.accesskey "T">

This keeps it as "..._T_ext" instead of "Expor_t_..."

Comment 7

[timeless]

the reason i changed more was to improve speed by an unnoticable amount 
by avoiding the fallback.  if someone wants to prune this to only essential 
changes they can, but I don't see any harm in doing what i did. still seeking 
reviews.

Comment 8

[Keyser Sose]

r=ksosez

Comment 9

[Darin Fisher]

sr=darin

Comment 10

[:deanis74]

I couldn't look at Composer or the New Message window - I think it screwed 
something up when patching - but there are more keys that need changing.

Navigator:
  File > New > Blank Pag_e_ to Edit  -- should be _E_dit
  File > Save A_s_  -- should be _S_ave
  Edit > View Sa_v_ed Data  -- should be _V_iew

Mail:
  File > New > Blank Pag_e_ to Edit  -- should be _E_dit
  File > Save A_s_  -- should be _S_ave
  File > Save As > Templa_t_e  -- should be _T_emplate
  View > Messages > Threads Wi_t_h Unread  -- should be _T_hreads
  Message > Reply to Sende_r_ Only  -- should be _R_eply
  Message > Forward As > Inl_i_ne  -- should be _I_nline
  Message > Forward As > Att_a_chment  -- should be _A_ttachment
  Message > Edit M_e_ssage as New  -- should be _E_dit
  Message > Mark > All R_e_ad  -- should be _A_ll

Address Book:
  File > New > Blank Pag_e_ to Edit  -- should be _E_dit
  File > Save A_s_  -- should be _S_ave
  View > Sort by > Organizati_o_n  -- should be _O_rganization

Tasks Overlay:
  Tasks > Privacy and Security > Password Manager > Change Personal Se_c_urity 
          Password  -- should be _C_hange
  Tasks > Privacy and Security > Password Manager > Encrypt S_e_nsitive
          Information  -- should be _E_ncrypt
  Tasks > Privacy and Security > Password Manager > Obscure Sensitive
          Inf_o_rmation  -- should be _O_bscure

Can someone else take a double-check at Composer and New Message?

Comment 11

[:deanis74]

A couple more in the context menu:

Edit Link in Compos_e_r - should be _E_dit
V_i_ew Page Info - should be _I_nfo

Comment 12

[sairuh (rarely reading bugmail)]

should the following be added as blockers for this bug?

bug 38124: duplicate access key 'b' in context menu
bug 88950: duplicate access key 'o' in context menu [framed pages]
bug 77626: duplicate access key 'f' in context menu [framed pages]
bug 68928: duplicate access key 'a' in context menu

Comment 13

[:deanis74]

I don't think so.  This bug is only to fix the accesskey changes that were 
introduced when bug 41572 was fixed.

Comment 14

[doctor__j]

3 more from the context menu:

"Open Link in New Wind_o_w" -> "_O_pen Link in New Window"
"Save A_s_..." -> "_S_ave As..."
"Copy Email Addr_e_ss" -> "Copy _E_mail Address"

Comment 15

[Chad Austin]

Keep in mind that the patch didn't apply 100% correctly, so I may say a few 
things that really are fixed.

Things missed:


Navigator:

File | Save As
Edit | View Saved Data
Tasks | Privacy and Security | Encrypt Sensitive Information
Tasks | Privacy and Security | Obscure Sensitive Information
File | New | Blank Page to Edit


Address Book:

File | New | Mailing List
View | Show/Hide | Status Bar
Sort By | Organization


Mail:

File | Save As
File | Rename Folder
View | Messages | Threads With Unread
Message | Forward As | Inline
Message | Mark | All Read


My build crashes on creating a new mail window, so I can't test that.



There are still a few menu items which don't have accesskeys at all.

Comment 16

[chris hofmann]

time for 0.9.4 has run out.  try for 0.9.5. thanks -chofmann

Comment 17

[:deanis74]

This just screams Patch Maker.

Comment 18

[Ben Ruppel]

Hi, I've collected all of the problems mentioned thus far and I am about 3/4 
way through fixing them all.  However, I can't figure out what to do 
with "About Mozilla", where the "a" is underlined instead of "A".  I have 
found the file where this seems to be defined (\chrome\locale\en-
US\global\about.dtd) but &brandShortName is used as a variable and I can not 
find where the access key is definied (if it is explicitly defined at all).  I 
don't think it would be correct to hardcode the accesskey to "A", but what 
else can we do?

Comment 19

[:deanis74]

Ben: It looks like about.dtd is for the about modal dialog, which is displayed
if you have the pref set in the Debug prefs.  To fix the access key in the menu,
you do want to hard-code the access key to "A", since you want it to show as
"_A_bout".  Change aboutCmd.accesskey from "a" to "A" in utilityOverlay.xul.

Comment 20

[Ben Ruppel]

Wow, somehow I missed that in LXR.  Thanks for the quick feedback.  Now the 
only thing holding me back is the fact that patchmaker doesn't seem to 
recognize the addressbook dtd file I need to change.

Comment 21

[Ben Ruppel]

Attachment: Patch that works with Nov 10 builds

Navigator:			
File > New > Blank Page to Edit X	
File > Save Page As		X	
File > Save As			Invalid 
Edit > View Saved Data		X	
File > New Navigator Window	X	
File > New -> Navigator window	X	
			
Tasks > Privacy and Security > Encrypt Sensitive Information	X
Tasks > Privacy and Security > Obscure Sensitive Information	X
Tasks > Privacy and Security > Reset Master Password		X

View menu:			
Show/Hide -> Navigation Toolbar 		X	
Show/Hide -> Status Bar 			X	
Translate					X	
Languages and Web Content -> Download More	X	
Character Coding				X
Character Coding > More > East European 	X
Character Coding > More > SE & SW Asian 	X
		
Bookmarks menu: 	
Manage Bookmarks...	X

Help menu:		
About Mozilla		X
		

Context Menu:		
Edit Link in Composer	X
View Page Info		X
Edit Mode Toolbar	Unable to Find	
Open Link in New Window X
Save As...		Changed?
Copy Email Address	X		

Address Book:				
View > Sort by > Organization	X	
File > New > Mailing List	X

This patch gives an offset with navigator.dtd and Nov. 10 builds, but it still
works as advertised.  If there are any issues with items on this list, bring
them to my attention and I will incorporate them.

Comment 22

[Ben Ruppel]

So, does anyone have a problem with this patch?  Anything to add?  Who should I
contact for an r/sr?  

Comment 23

[Aaron Leventhal]

r=aaronl

Looking for sr= (Ben Goodger, Alec Flett?)

Comment 24

[:deanis74]

1. File > Save As   Invalid
 It's now "S_a_ve Page As", and needs to be changed to "A"

2. Context Menu > Save As...   Changed?
 There's "S_a_ve Page As..." and "S_a_ve Link As".  Could change them both
 to "A" which would correct the mis-matched accesskey and maintain the duplicate
 access key.

I'll apply this patch later tonight and see if I can find any else.  I wouldn't
worry too much about context menus, because eventually they'll be revamped if
bug 75338 gets fixed.  But if you've got them in your patch, that's great.

Comment 25

[Ben Ruppel]

Dean: File > Save As is fixed (as noted in the list right above Save As) :)
Save As is also fixed in the context menu, but I didn't catch Save Link As. 
Should I bother?  This is a dupe accesskey anyway and will probably be changed.

Comment 26

[Jesse Ruderman]

Bug 68928 covers the duplicate accesskey 'a' for the various save as items.

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 57413 [details] [diff] [review]
Patch that works with Nov 10 builds

sr=[e-mail]

Comment 28

[Ben Ruppel]

Thank you Ben.  So now someone else (timeless?) needs to check it in, correct?

Comment 29

[:deanis74]

What about the Mail menu items that I mentioned on 2001-06-27?  Plus, there are
a few in Composer and New Message, which I didn't mention back then because they
were crashing.

Comment 30

[Ben Ruppel]

I made a file of every single problem reported here, and stupidly I deleted
entries when I found them to be fixed (I should have kept them and given them a
mark).  In other words, the Mail Menu items have already been fixed.  

About the editor items: Blast you Tessman!  Should I make a separate patch (or
maybe even second bug) to cover this, since the current one already got a r/sr?
 List of items forthcoming.

Comment 31

[:deanis74]

spin off a new bug for Composer and New Message since you've already got r/sr
for this patch.

Comment 32

[Ben Ruppel]

Posted Bug 109824.

Comment 33

[Ben Ruppel]

FYI, I've got a patch for Bug 109824 posted.  Should be ready for r/sr, but I
guess I should wait until some other people (Dean) check it out and find some
other items I've missed.

Comment 34

[timeless]

checked in, thanks

Comment 35

[sairuh (rarely reading bugmail)]

rs vrfy --much has changed since the toplevel and context menus have been reorg'd.