Example open Composer click Insert> see Named Anchor. The access key is a, Logically the output should be: "Named &Anchor" the output we have is "N&amed Anchor". We should prefer capital letters [this is just a gui look and feel issue]. It looks better if you underline the capital letter, it also says something to the brain about what's important. Kairo and the internationalization people would also like to be able to absolutely specify which character is marked as the access key. Another example was Vie&w Password or something, where View Pass&word makes more sense. I think the following logic might make sense: first->last scan for capital letters matching access key the next piece of logic is questionable: last->first scan for lower case letters if not, the normal first->last scan is a reasonable fallback

i could do the search for capital letters first, but the rest is subjective and can be left alone.

Thank you. Out of curiosity what file/class controls this?

this is probably in nsMenuFrame.cpp. jag would know for sure.

No, this is actually in: http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsTextBoxFrame.cpp#514 You'd have to change that to a case sensitive find on the uppercase of the accesskey, if that fails fall back to a case sensitive find on the lowercase.

Can I dive off topic here for a second and ask why it's in nsTextBoxFrame?

Where did you expect to see it?

anyone else want to take this? it should be easy

Gimme

Pretty straight-forward. r= anyone?

Hrm, i've never met gAlwaysAppendAccessKey but from the looks of it, we probably want this logic instead: if (gAlwaysAppendAccessKey) { // use reverse string find for appended access keys mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.RFind(mAccessKey, PR_FALSE); } else { // search for the access key as upper-case first nsString upperCaseAccessKey = mAccessKey; upperCaseAccessKey.ToUpperCase(); mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.Find(upperCaseAccessKey, PR_FALSE); if (mAccessKeyInfo->mAccesskeyIndex == 0 && mAccessKey != upperCaseAccessKey) { // didn't find uppercase access key - check for original only if it's lowercase mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.Find(mAccessKey, PR_TRUE); } } look ok?

What, by the way, is gAlwaysAppendAccessKey, and why would we still not have the uppercase key take precendence if it's true?

Oh, I see, it was done for bug 54285. Timeless, your logic looks right in light of this. Do you want to make up a patch or should I?

go for it

New patch, incorporating timeless's suggestions. The only change from his is that the Find() when gAlwaysAppendAccessKey is true was originally case- insensitive, so I left it that way.

mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.Find(mAccessKey, PR_TRUE); } else { - mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.RFind(mAccessKey, PR_TRUE); it was originally an RFind (i presume that means reverse). sorry about the PR_bits, i wasn't paying much attention. sigh, sorry for all of the spam. r=timeless if you make it an RFind again.

Did you hand edit that last patch? + if (gAlwaysAppendAccessKey) { + // use reverse string find for appended access keys mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.RFind(mAccessKey, PR_TRUE); } else { - mAccessKeyInfo->mAccesskeyIndex = mCroppedTitle.RFind(mAccessKey, PR_TRUE); In the original code that first RFind is a Find. Anyway, here are a couple of comments: What if you take the case of the accesskey as specified in the DTD/XUL to be the preferred match? That gives authors that bit more control... Then your logic becomes: if (gAlwaysAppend) caseless search from the right else { search from the left if not found caseless search from the left } This will require fixing up some dtd/xul, but is cleaner, imo. Also, I think we should be storing the access key in a PRUnichar instead of in a nsString to save space. Though, currently, I think you can force a match against a certain letter by using a short string as "accesskey". E.g. in the string "Access" you could force a match against the second 'c' by using "ce" for the accesskey value. Only the first letter is used to determine the access key and which key to underline. I think that's an accidental feature though :-)

No, I didn't hand-edit that last patch, I try not to do that. I like your idea about doing the case-sensitive match first. I'm going to start from scratch and do it that way. The PRUnichar idea should prolly go in a separate bug, though. So much spam for such a little enhancement. It's just not my week.

New patch coming with jag's suggestion about a case-sensitive match taking precedence. Should probably re-summarize this bug if we're going to go this way. Tested this by going into editorOverlay.dtd and changing insertanchor.accesskey to "A". To make sure this falls back to a case-insensitive search, change insertanchor.accesskey back to "a" and insertAnchorCmd.label to "NAmed Anchor".

r=timeless

Need an sr= on this 0.9.2 bug. Hyatt? (Whoops, accepting)

sr=blake

a=dbaron for trunk checkin (on behalf of drivers)

I don't have check-in permissions. Can someone check this in for me?

fix checked in. thanks

Verified on 2001060804. Of course there's now a bunch of poorly-matched access keys. eg. Import Ut_i_lity instead of _I_mport Utility, Manage Book_m_arks instead of _M_anage Bookmarks, ...

So Dean, do you want to fix them, then? :-) Gerv

Blake emailed me and he's on 'em.