Closed Bug 410164 Opened 16 years ago Closed 16 years ago

Put back commandkeys without modifiers in Go/Message submenus

Categories

(SeaMonkey :: MailNews: Message Display, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(1 file, 1 obsolete file)

Optionally, someone should confirm that bug 195979 is fixed, but it seems to work now...
Attached patch Display keys in Mac menus (obsolete) — Splinter Review
Basically a back-out of the patch in bug 194147.
Attachment #294831 - Flags: superreview?(neil)
Attachment #294831 - Flags: review?(mnyromyr)
Status: NEW → ASSIGNED
Target Milestone: --- → seamonkey2.0alpha
JFTR: Phil's Thunderbird version is in bug 199019.
Comment on attachment 294831 [details] [diff] [review]
Display keys in Mac menus

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>         <menuitem id="markReadMenuItem"
>         <menuitem id="markFlaggedMenuItem"

Why didn't you remove these ids, too?

But actually, every menuitem should have an id (eg to ease extension development), so please don't remove them at all.
Attachment #294831 - Flags: review?(mnyromyr) → review-
Comment on attachment 294831 [details] [diff] [review]
Display keys in Mac menus

Also, can you please not put the key between the label and its accesskey, thanks.
Attachment #294831 - Flags: superreview?(neil) → superreview-
(In reply to comment #3)
> (From update of attachment 294831 [details] [diff] [review])
> >Index: mailnews/base/resources/content/mailWindowOverlay.xul
> >===================================================================
> >         <menuitem id="markReadMenuItem"
> >         <menuitem id="markFlaggedMenuItem"
> 
> Why didn't you remove these ids, too?

I guess I never checked why they where there before the patch in bug 194147.

> But actually, every menuitem should have an id (eg to ease extension
> development), so please don't remove them at all.

OK. 

(In reply to comment #4)
> (From update of attachment 294831 [details] [diff] [review])
> Also, can you please not put the key between the label and its accesskey,
> thanks.

Oops, I followed the bad examples in the file.

Attached patch New versionSplinter Review
OK, keys are now after accesskeys. All id's are back and I also added two id's in the markMenu.
Attachment #294831 - Attachment is obsolete: true
Attachment #294956 - Flags: superreview?(neil)
Attachment #294956 - Flags: review?(mnyromyr)
Attachment #294956 - Flags: superreview?(neil) → superreview+
Comment on attachment 294956 [details] [diff] [review]
New version

Just two minor nits for check-in: ;-)

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>       <menu id="goNextMenu" label="&nextMenu.label;" accesskey="&nextMenu.accesskey;">
>       <menu id="goPreviousMenu" label="&prevMenu.label;" accesskey="&prevMenu.accesskey;">

Please use the one-attribute-per-line style for their menuitems (as used for later ones) and add the ids nextFlaggedMenuItem/prevFlaggedMenuItem while you're at it.

r=me with that.
Attachment #294956 - Flags: review?(mnyromyr) → review+
Landed with Karsten's nits addressed:
Checking in mailnews/base/resources/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.344; previous revision: 1.343
done
Checking in mailnews/base/resources/content/unix/platformMailnewsOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/unix/platformMailnewsOverlay.xul,v  <--  platformMailnewsOverlay.xul
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/base/resources/content/win/platformMailnewsOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/win/platformMailnewsOverlay.xul,v  <--  platformMailnewsOverlay.xul
new revision: 1.7; previous revision: 1.6
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.