393721 - keypresses should not propagate to content when a menubar is active

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 1

[[e-mail]]

Not specific to mail composition, applies to all text fields.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 3

[Neil Deakin]

Attachment: cancel event when a menubar is active

Comment 4

[[e-mail]]

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();

Comment 5

[Neil Deakin]

Checked in, need to figure out how to get a test for this.

Comment 6

[Reed Loden [:reed]]

(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.

Comment 7

[Neil Deakin]

Attachment: this version actually works

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.