Site Navigation menu doesn't have access keys

RESOLVED FIXED

Status

()

Core
Keyboard: Navigation
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: Greg Valure, Assigned: gerv)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

2.36 KB, patch
Greg Valure
: review+
Greg Valure
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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)
(Reporter)

Comment 1

16 years ago
Created attachment 75872 [details] [diff] [review]
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
(Reporter)

Comment 2

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

Comment 3

16 years ago
i think i'd prefer site na&vigation bar
(Reporter)

Comment 4

16 years ago
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

16 years ago
-> NEW 
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch

Updated

15 years ago
Blocks: 129179

Comment 6

15 years ago
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.
Assignee: aaronl → gerv

Comment 7

15 years ago
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
Attachment #75872 - Flags: needs-work+
(Reporter)

Comment 8

15 years ago
Created attachment 95512 [details] [diff] [review]
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
Attachment #75872 - Attachment is obsolete: true
(Assignee)

Comment 9

15 years ago
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

15 years ago
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.
Attachment #95512 - Flags: review+

Comment 11

15 years ago
Comment on attachment 95512 [details] [diff] [review]
Patch 2

sr=jag
Attachment #95512 - Flags: superreview+
(Reporter)

Comment 12

15 years ago
Created attachment 96265 [details] [diff] [review]
Patch 2.1

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

Updated

15 years ago
Attachment #95512 - Attachment is obsolete: true
(Reporter)

Comment 13

15 years ago
Comment on attachment 96265 [details] [diff] [review]
Patch 2.1

Carrying over reviews.
Attachment #96265 - Flags: superreview+
Attachment #96265 - Flags: review+

Comment 14

15 years ago
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
(Reporter)

Comment 15

15 years ago
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.
(Reporter)

Comment 16

15 years ago
Created attachment 96272 [details] [diff] [review]
Patch 2.2

Okay, this one was pulled from CVS and done without PatchMaker.
Attachment #96265 - Attachment is obsolete: true
(Reporter)

Comment 17

15 years ago
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.
Attachment #96272 - Attachment description: Patch 2,2 → Patch 2.2
Attachment #96272 - Flags: superreview+
Attachment #96272 - Flags: review+

Comment 18

15 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
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/
(Assignee)

Comment 20

15 years ago
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
(Reporter)

Comment 21

15 years ago
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?
Keywords: verifyme
QA Contact: sairuh → nobody
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.