250005 - Shortcut keys conflict: New Message and Mark All Read are both Command+Shift+M

Status: VERIFIED FIXED

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

Description

[Stewart Gordon]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a2) Gecko/20040705
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a2) Gecko/20040705

First I discovered that Cmd+Shift+C had stopped working to mark all messages in
a newsgroup as read.  I then discovered that it had been changed to Cmd+Shift+M
(bug 186789).  However, this shortcut doesn't work either.

The problem is that two commands have become assigned the same shortcut key:
Message -> New Message and Message -> Mark -> All Read.

Reproducible: Always
Steps to Reproduce:
1. Open a newsgroup.
2. Press Cmd+Shift+M.

Actual Results:  
A message composition window appears.

Expected Results:  
Well, I was expecting it to mark all messages in the newsgroup as read.  We need
to change one of the shortcut keys so that it doesn't conflict.

Comment 1

[Stefan Borggraefe]

CCing the owner of bug 186789. See also bug 186789 comment 70.

Comment 2

[Stewart Gordon]

Thinking about it, we should keep Cmd+Shift+M for New Message and change Mark
All Read.  That way, people who use Cmd+Shift+M for New Message won't suddenly
find their messages inadvertently being marked read when this is fixed.

OTOH nobody uses Cmd+Shift+M for Mark All Read, since it doesn't work, so giving
this command a new shortcut won't interfere with anything.

Comment 3

[R.K.Aa.]

*** Bug 252430 has been marked as a duplicate of this bug. ***

Comment 4

[Eliot Lear]

Guys- up until 1.8a1 this worked!  It's a regression.

Comment 5

[Philip K. Warren]

I would prefer to use the same keyboard shortcut on all platforms for Mark All
Read, and unfortunately both the old shortcut (Ctrl+Shift+C) and the Outlook
Express shortcut (Ctrl+Shift+A) will not work in GTK2 builds (Bug 186789), so we
need to choose a new shortcut from the following list:

http://www.mozilla.org/projects/ui/accessibility/mozkeylist.html

Comment 6

[Philip K. Warren]

Attachment: Patch v1

This changes the shortcut for Mark All Read back to Ctrl+Shift+C for Mac/Win
and only changes it for Linux builds. This is one alternative to defining a new
shortcut on all platforms. Neil - what do you think?

Comment 7

[[e-mail]]

Comment on attachment 154044 [details] [diff] [review]
Patch v1

I don't think we're allowed to have unlocalizable key bindings.

Comment 8

[Stefan [:stefanh]]

While fixing this, please also change in
extensions/help/resources/locale/en-US/shortcuts-mailnews.xhtml.

Comment 9

[Philip K. Warren]

Attachment: Patch v2

I have added a new entity to messenger.dtd, as there is not a preexisting
platform specific dtd file in MailNews. I have also updated the shortcuts help
section to include the changes.

Comment 10

[Mike Cowperthwaite]

*** Bug 253432 has been marked as a duplicate of this bug. ***

Comment 11

[Eliot Lear]

What is the status of this patch?  What needs to be done to get it landed?  Can
I help?

Thanks...

Comment 12

[Philip K. Warren]

Comment on attachment 154496 [details] [diff] [review]
Patch v2

David - Can you super review?

Comment 13

[Philip K. Warren]

Comment on attachment 154496 [details] [diff] [review]
Patch v2

Requesting approval for a simple patch which fixes a very visible regression in
Mail/News on Mac.

Comment 14

[Mike Kaply [:mkaply]]

Comment on attachment 154496 [details] [diff] [review]
Patch v2

a=mkaply

Comment 15

[Scott MacGregor]

I see this patch has been nominated for the aviary 1.0 branch but I don't see
that it contains any changes for aviary files. Does this bug exist on the aviary
branch? And if so, you'll need a patch that modifies the files actually used by
the aviary branch instead of the mailnews equivalents. 

Comment 16

[Philip K. Warren]

Fixed on trunk.

Checking in extensions/help/resources/locale/en-US/shortcuts-mailnews.xhtml;
/cvsroot/mozilla/extensions/help/resources/locale/en-US/shortcuts-mailnews.xhtml,v
 <--  shortcuts-mailnews.xhtml
new revision: 1.8; previous revision: 1.7
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.7; previous revision: 1.6
done
Checking in mailnews/base/resources/locale/en-US/messenger.dtd;
/cvsroot/mozilla/mailnews/base/resources/locale/en-US/messenger.dtd,v  <-- 
messenger.dtd
new revision: 1.191; previous revision: 1.190
done

Comment 17

[Philip K. Warren]

(In reply to comment #15)
> I see this patch has been nominated for the aviary 1.0 branch but I don't see
> that it contains any changes for aviary files. Does this bug exist on the aviary
> branch? And if so, you'll need a patch that modifies the files actually used by
> the aviary branch instead of the mailnews equivalents. 

You are correct - the patch for Bug 186789 (which caused this regression) was
never checked in on the Aviary branch.

Comment 18

[Worcester12345]

(In reply to comment #17)
> (In reply to comment #15)
> > I see this patch has been nominated for the aviary 1.0 branch but I don't see
> > that it contains any changes for aviary files. Does this bug exist on the aviary
> > branch? And if so, you'll need a patch that modifies the files actually used by
> > the aviary branch instead of the mailnews equivalents. 
> 
> You are correct - the patch for Bug 186789 (which caused this regression) was
> never checked in on the Aviary branch.


So do BOTH then get checked in? Just curious how that would work, and why it was
missed the first time around.

Comment 19

[Philip K. Warren]

(In reply to comment #18)
> So do BOTH then get checked in? Just curious how that would work, and why it was
> missed the first time around.

The patch to fix Bug 186789 only fixes the problem in the SeaMonkey Mail/News
client. If someone is interested in changing the keybindings to not conflict in
GTK2 Thunderbird builds, a new bug should be opened for that.

Comment 20

[Stewart Gordon]

Thanks for a prompt (by Mozilla standards) fix to this bug!  Now, if only other
bugs could be such quick fixes....

Comment 21

[Kevin Buhr]

Re: comment #19

The corresponding Thunderbird issues were bug 239483 (Mark All Read
Ctrl+Shift+C), bug 242947 (Search Mail Ctrl+Shift+F), and bug 269228 (Select
Thread Ctrl+Shift+A).  The first two were resolved by changing them to Shift+C
and Shift+F respectively on GTK2 builds.  The last was missed, but I've attached
a corresponding patch to that bug to change it to Shift+A.