41572 - Prefer matched case for Access keys

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[timeless]

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

Comment 1

[Mike Pinkerton (not reading bugmail)]

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

Comment 2

[timeless]

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

Comment 3

[Mike Pinkerton (not reading bugmail)]

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

Comment 4

[Peter "jag" Annema]

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.

Comment 5

[Blake Ross]

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

Comment 6

[Peter "jag" Annema]

Where did you expect to see it?

Comment 7

[Mike Pinkerton (not reading bugmail)]

anyone else want to take this? it should be easy

Comment 8

[:deanis74]

Gimme

Comment 9

[:deanis74]

Attachment: Like this?

Comment 10

[:deanis74]

Pretty straight-forward.  r= anyone?

Comment 11

[timeless]

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?

Comment 12

[:deanis74]

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

Comment 13

[:deanis74]

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?

Comment 14

[timeless]

go for it

Comment 15

[:deanis74]

Attachment: new patch

Comment 16

[:deanis74]

Attachment: Better patch. Ignore that last one - there's some many things wrong with it.

Comment 17

[:deanis74]

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.

Comment 18

[timeless]

                 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.

Comment 19

[:deanis74]

Attachment: Aw man, I'm dumb.

Comment 20

[Peter "jag" Annema]

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

Comment 21

[:deanis74]

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.

Comment 22

[:deanis74]

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

Comment 23

[:deanis74]

Attachment: please let this end...

Comment 24

[timeless]

r=timeless

Comment 25

[:deanis74]

Need an sr= on this 0.9.2 bug.  Hyatt?

(Whoops, accepting)

Comment 26

[Blake Ross]

sr=blake

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 28

[:deanis74]

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

Comment 29

[timeless]

fix checked in. thanks

Comment 30

[:deanis74]

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

Comment 31

[Gervase Markham [:gerv]]

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

Gerv

Comment 32

[:deanis74]

Blake emailed me and he's on 'em.