Closed Bug 393721 Opened 14 years ago Closed 14 years ago

keypresses should not propagate to content when a menubar is active

Categories

(Core :: XUL, defect, P3)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: nelson, Assigned: enndeakin)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Seen with SM 2.0a1pre 20070824.  I recently updated to that from 20070810.
The regression window is between those two dates.

For YEARS, I have been using the rewrap feature of the mail composer to 
rewrap quoted lines in replies being composed.  I do it by highlighting
a line or paragraph of quoted text, and then typing this sequence of keys:

  Press Alt,  release Alt,
  Press e,    release e,
  Press w,    release w

In the past, that would cause the highlighted text to be rewrapped.
Now, in 20070824, when I press the e, it causes the highlighted text 
to be entirely replaced with the letter e. Then when I press the w, 
it rewraps the entire message, not the formerly highlighted (now missing)
text.  

The only workaround is to type this alternative sequence:
  Press Alt, hold it down.  Press e, release e and Alt,
  Press W, release W.  

Why did this fundamental operational behavior have to change?
Flags: blocking1.9?
Flags: blocking-thunderbird3?
Not specific to mail composition, applies to all text fields.
Component: MailNews: Composition → XP Toolkit/Widgets: Menus
Flags: blocking-thunderbird3?
QA Contact: composition → xptoolkit.menus
The same problem is present in 2009, but from your regression range, it sounds likely that it was a bug that went in to both trunk and 2.0.0.x.

Neil, can you take a look at this and see if this was a recent regression?
Assignee: nobody → enndeakin
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Status: NEW → ASSIGNED
Summary: alt key followed by letter acts as if letter was typed without alt → keypresses should not propagate to content when a menubar is active
Attachment #288533 - Flags: superreview?(neil)
Attachment #288533 - Flags: review?(neil)
Comment on attachment 288533 [details] [diff] [review]
cancel event when a menubar is active

Note: I had to guess how to merge the patch to tip, but this seemed to work:

Index: layout/xul/base/src/nsXULPopupManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xul/base/src/nsXULPopupManager.cpp,v
retrieving revision 1.33
diff -p -u -d -r1.33 nsXULPopupManager.cpp
--- layout/xul/base/src/nsXULPopupManager.cpp	12 Nov 2007 21:52:14 -0000	1.33
+++ layout/xul/base/src/nsXULPopupManager.cpp	14 Nov 2007 15:40:12 -0000
@@ -1648,22 +1648,28 @@ nsXULPopupManager::IsValidMenuItem(nsPre
 nsresult
 nsXULPopupManager::KeyUp(nsIDOMEvent* aKeyEvent)
 {
-  nsMenuChainItem* item = GetTopVisibleMenu();
-  if (item && item->PopupType() == ePopupTypeMenu) {
-    aKeyEvent->StopPropagation();
-    aKeyEvent->PreventDefault();
+  // don't do anything if a menu isn't open or a menubar isn't active
+  if (!mActiveMenuBar) {
+    nsMenuChainItem* item = GetTopVisibleMenu();
+    if (!item || item->PopupType() != ePopupTypeMenu)
+      return NS_OK;
   }
 
+  aKeyEvent->StopPropagation();
+  aKeyEvent->PreventDefault();
+
   return NS_OK; // I am consuming event
 }
 
 nsresult
 nsXULPopupManager::KeyDown(nsIDOMEvent* aKeyEvent)
 {
-  // don't do anything if a menu isn't open
-  nsMenuChainItem* item = GetTopVisibleMenu();
-  if (!item || item->PopupType() != ePopupTypeMenu)
-    return NS_OK;
+  // don't do anything if a menu isn't open or a menubar isn't active
+  if (!mActiveMenuBar) {
+    nsMenuChainItem* item = GetTopVisibleMenu();
+    if (!item || item->PopupType() != ePopupTypeMenu)
+      return NS_OK;
+  }
 
   PRInt32 menuAccessKey = -1;
 
@@ -1788,7 +1794,7 @@ nsXULPopupManager::KeyPress(nsIDOMEvent*
     HandleShortcutNavigation(keyEvent, nsnull);
   }
 
-  if (mCurrentMenu) {
+  if (mCurrentMenu || mActiveMenuBar) {
     // if a menu is open, it consumes the key event
     aKeyEvent->StopPropagation();
     aKeyEvent->PreventDefault();
Attachment #288533 - Flags: superreview?(neil)
Attachment #288533 - Flags: superreview+
Attachment #288533 - Flags: review?(neil)
Attachment #288533 - Flags: review+
Checked in, need to figure out how to get a test for this.
Flags: in-testsuite?
(In reply to comment #5)
> Checked in, need to figure out how to get a test for this.

I backed this out, as it turned all unit test machines orange except for Mac.
Some changes:
 - make sure when alt, tab or F10 is pressed that MenuClosed is called. This worked before when events weren't prevented as a the handler that activates menubars handled it, which coincidently happened to work, but isn't correct.
 - store the 'consume' state before processing the keys because a menu may be closed or a menubar can become deactive during the key handler.
Attachment #288533 - Attachment is obsolete: true
Attachment #289548 - Flags: superreview?(neil)
Attachment #289548 - Flags: review?(neil)
Attachment #289548 - Flags: superreview?(neil)
Attachment #289548 - Flags: superreview+
Attachment #289548 - Flags: review?(neil)
Attachment #289548 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.