133155 - Site Navigation menu doesn't have access keys

Status: RESOLVED FIXED

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

Description

[Greg Valure]

View | Show/Hide | Site Navigation Bar and associated sub-menu items don't have
access keys defined.

2002-03-11-04 Win98 (0.9.9)

Comment 1

[Greg Valure]

Attachment: Patch

Defines access keys as follows:

View | Show/Hide | S_i_te Navigation Bar
(S is used by Status Bar, and N by Navigation Toolbar)

 | _S_how Always
 | Show _O_nly as Needed
 | _H_ide Always

Comment 2

[Greg Valure]

Also consider Show Only as _N_eeded.  I can make another patch if that's
determined to be better.

Comment 3

[timeless]

i think i'd prefer site na&vigation bar

Comment 4

[Greg Valure]

Why exactly?  I would think that because "site" is the distinguishing word, it
would get the emphasis.  Granted, an underlined "i" doesn't stand out very well
though.

Comment 5

[Scott Tran (Wildcard)]

-> NEW 

Comment 6

[Aaron Leventhal]

Gerv, shame on you for not putting the accesskeys in in the first place - very
naughty, please don't do it again or you'll have the wrath of the accessibility
world upon you.

It's your bug. Maybe Greg is still around to do it for you.

Comment 7

[Aaron Leventhal]

Comment on attachment 75872 [details] [diff] [review]
Patch

Greg, if you're still around, we avoid putting accesskey on lower case i or l,
because they are thin letters so the underline is hard to see.

The full xul accesskey faq is at
http://www.mozilla.org/projects/ui/accessibility/accesskey.html

Comment 8

[Greg Valure]

Attachment: Patch 2

Wow, I forgot about this bug altogether.  This patch sets the accesskeys to:

Site N_a_vigation Bar >
   _S_how Always
   Show _O_nly as Needed
   _H_ide Always

Comment 9

[Gervase Markham [:gerv]]

Huh? I didn't implement this menu item. sballard did. I just checked it in for
him. (Yes, perhaps I should have spotted this in review.)

Gerv

Comment 10

[Aaron Leventhal]

Comment on attachment 95512 [details] [diff] [review]
Patch 2

r=aaronl

Thanks Greg - are you interested in helping out with any of the other bugs
under meta bug 129179? 

BTW, I'm currently getting bug 68841 reviewed, which will help a lot, because
XUL radios and checkboxes don't currently get their accesskeys underlined.

Comment 11

[jag (Peter Annema)]

Comment on attachment 95512 [details] [diff] [review]
Patch 2

sr=jag

Comment 12

[Greg Valure]

Attachment: Patch 2.1

Ack.  PatchMaker didn't do a very good filename translation on that one.  This
one should work.

Comment 13

[Greg Valure]

Comment on attachment 96265 [details] [diff] [review]
Patch 2.1

Carrying over reviews.

Comment 14

[Aaron Leventhal]

Hmm, why still no lines in the patch like:
Index: blah
==============

Maybe there's some way to apply this diff that I don't know?
patch -p0 <patchname won't work

Comment 15

[Greg Valure]

Hmm...  I'll try to look into this.  There weren't any changes to the underlying
files since the patch was made, and the patch to bug 139624 checked in okay,
which was also made in PatchMaker and has the same style.

Comment 16

[Greg Valure]

Attachment: Patch 2.2

Okay, this one was pulled from CVS and done without PatchMaker.

Comment 17

[Greg Valure]

Comment on attachment 96272 [details] [diff] [review]
Patch 2.2

Maybe it was the date format.  The PatchMaker patch seems to use a different
one than diff.	The PatchMaker patch from the other bug doesn't use that date
format either, so maybe a bug got added to the new version.

Comment 18

[Aaron Leventhal]

Checked in. I manually edited the lines starting "Index: " to include full
paths, and then the patch applied.

I think I've heard this is a problem with cvs-mirror's old CVS server software,
but I could be wrong.

Gerv, are patches created by patchmaker all suffering from these problems?

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Aaron, no those diffs are fine.  You just need to cd into the directory where
the patch was created.  Since there are no directories in the index lines, the
patch was created in the same directory as the file.  It will apply cleanly from
within mozilla/xpfe/browser/resources/content/

Comment 20

[Gervase Markham [:gerv]]

As the man says. Patch Maker users should create all patches from the top-level
Mozilla directory, i.e. using pma from that directory, and running pmd in it.

Gerv

Comment 21

[Greg Valure]

Actually, that's what I did for patch 2 and 2.1.  Compare their date lines to
the first patch.  Do you think that would cause the problem?

Comment 22

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old