Closed Bug 344249 Opened 14 years ago Closed 14 years ago
Menu key commands processed twice after loading Java
After loading Java on the Mac via JEP, key equivalents for menu commands may be processed twice. This can occur on any branch, but is guaranteed to occur now on the trunk after the patch for bug 50590. Steps to reproduce: 1. Open a browser window 2. Where bug 50590 has not yet landed (1.8.0 and 1.8[.1] branches), open a menu to ensure it's built. For this example, open the File menu. It is not important to select any command from the menu, just close the menu once it's open. 3. Load a Java applet, such as http://www.java.com/en/download/help/testvm.xml 4. Use key equivalents corresponding to the menu in step 2: press command-T to open a new tab. Press command-W to close. Expected behavior: each menu command key equivalent should only cause a single action to be taken. Observed behavior: menu command key equivalents cause the action to be taken twice. The action is performed once as a result of the menu command being selected, and once again on a key handler. Normally, once a menu is constructed, we only take action on menu commands, and we don't receive key events at all. Loading Java is causing us to take key events as well. We may be able to fix this by not installing key event handlers for menu commands, now that we reliably have constructed menus available at all times. Even so, loading Java should not suddenly make these key events available to the browser. On the current trunk, and anywhere else the patch to bug 50590 lands, the steps to reproduce are much more straightforward. That patch changed things to ensure that menus are always constructed, without the need to open the menu first.
I've confirmed this with Firefox 22.214.171.124 plus JEP 0.9.5+f and JEP 0.9.5+e, running on OS X 10.3.9. JEP 0.9.5+d doesn't have the problem, so I guess the bug was introduced with JEP 0.9.5+e. It's really too bad nobody (including me) noticed this earlier. But that may mean that it doesn't often get triggered. I usually use keyboard shortcuts instead of the menu ... which is my excuse for not having seen this :-(
JEP should be fixed anyway, but... This adds a layer of security by allowing widget to disable the key handlers for any commands that go into the menu system. The key commands are extraneous because we now expect only to take these commands via nsMenuBarX::CommandEventHandler, and when everything is working properly, we won't take any of these commands through the XBL handlers.
Assignee: nobody → mark
Status: NEW → ASSIGNED
Attachment #228856 - Flags: review?(joshmoz)
Comment on attachment 228856 [details] [diff] [review] Add XBL disabled-hard attribute, use for disabling commands attached to menus Looks fine to me, but I think we should come up with a better name than "disabledHard". Also a comment about that attribute outside of widget code would be nice, so its easy to find out what its for.
Attachment #228856 - Flags: review?(joshmoz) → review+
Comment on attachment 228856 [details] [diff] [review] Add XBL disabled-hard attribute, use for disabling commands attached to menus For the attribute, how about "no-process?" I can add a brief comment to nsGkAtomList.h describing the use of this attribute to disable XBL commands without touching "disabled" and causing problems for things that monitor that attribute.
Attachment #228856 - Flags: superreview?(bzbarsky)
Attachment #228856 - Flags: superreview?(bzbarsky) → superreview?(bryner)
adding more more people who might care about this. Mark, I'm not sure I understand your explanation completely -- why do the XBL handlers get fired in this case?
On the Mac, with menus functioning properly, we don't handle key equivalents on keypress handlers. We handle them directly through the menu system. When the user presses command-N, we don't want to see a keypress for command-N. The system's menu manager picks up the keypress and generates the same menu command event that would have been generated if the user had used the mouse to select File:New. On other platforms, the XBL handlers for these same keypress events are called as a result of receiving native keypress events, which are turned into Gecko keypress events. On the Mac, we don't normally see these native keypress events, because once we set a key equivalent for a native menu item, we stop receiving native keypress events for that keystroke. That's what usually keeps key equivalents from causing two actions to occur. This problem with the JEP shows that it's possible for us to receive both a native menu command event AND the native keypress event. Because we always expect to have the native menus synced up with the keypress handlers that they replace, the handlers that run for a keypress should never be called. This patch allows us to disable those handlers so that in the event that they are called erroneously, they won't have any harmful effects.
So shouldn't the handlers just be #ifdef whatever the relevant thing is? Alternately, should the window handler thing obey the "disabled" attr?
(In reply to comment #7) > So shouldn't the handlers just be #ifdef whatever the relevant thing is? No, we need the command handlers - we pull the key equivalents out of them and use them to build the native menu items, and continue to monitor them for changes to update the native menu items as necessary. > Alternately, should the window handler thing obey the "disabled" attr? It does. That's why we need a new attribute. We watch the "disabled" attribute on the command elements with an nsIChangeObserver so that we can update the native menu item's state as the command is enabled and disabled when AttributeChanged is called. Because we need to observe this change, we can't just set the command's existing "disabled" attribute, because then we won't get a change notification when something really does need to disable the command.
Hm, so it seems like the menu code could just have an event listener that calls preventDefault() on events that shouldn't fire the key handlers...
bryner's suggestion doesn't work. What I tried is in this patch. HandleEvent is called (for both native menu items and for keystrokes, so we'd need to keep track of things) but preventDefault has no effect.
*** Bug 344734 has been marked as a duplicate of this bug. ***
Nominating as a 1.8.1 blocker. This is a significant regression, and it will be even more obvious once 50590 lands on the 1.8 branch.
Bryner, can we get some sr=magic?
Flags: blocking1.8.1? → blocking1.8.1+
Target Milestone: --- → mozilla1.8.1beta2
This fix is only partial, because I can't find the right nsIContent node in nsXBLPrototypeHandler::ExecuteHandler to look for the noProcess attribute on. The patch as I've got it here fixes double-processing for certain things (like command-T for new tab and command-W for close), but not for others (like command-V for paste. Yuck.) To see what's wrong with this patch, have a letter copied the clipboard, and load a Java applet. (http://www.java.com/en/download/help/testvm.xml). Focus the search box and open the Edit menu. (Focusing the search box ensures that the Paste command is enabled.) Close the Edit menu and press command-V. If someone can tell how to get from that method to the node that I'm setting noProcess on in nsMenuItemX, we should be able to make this work with the following snippet: #ifdef XP_MACOSX nsAutoString noProcess; CONTENTNODE->GetAttr(kNameSpaceID_None, nsXULAtoms:noProcess, noProcess); if (noProcess.EqualsLiteral("true")) return NS_OK; #endif
Last version was missing a file.
Attachment #230190 - Attachment is obsolete: true
I've got this fixed (from the JEP side) in my current version of the JEP. I'm working very hard to get this out (as JEP 0.9.5+g) tomorrow (Saturday 7/22) or the day after. Once it's out I'll ask Mark to bang away at it ... since he's lately been particularly good at finding JEP bugs :-) Then, if it passes muster, I'll ask that it be landed on the trunk and branches.
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/utilityOverlay.xul#181 <!-- These key nodes are here only for show. The real bindings come from XBL, in platformHTMLBindings.xml. See bugs 57078 and 71779. --> If Java can get at the original keystrokes for menuitems, why can't we? That would then solve bug 199019, bug 194147 etc.
Steven: While it's great to have a new JEP with this problem fixed so soon, we are most likly not going to take it for the 126.96.36.199 release. Even landing Marks patch this late in the game is very scary and might end up affecting when we can release. However it's of course great if we can have a new release for 188.8.131.52 Mark: is there any way I can help with completing the patch? Ping me on irc if so.
> If Java can get at the original keystrokes for menuitems, why can't > we? The answer is complicated, and is (as far as I can tell) irrelevant to this bug -- Mark wants to stop using keypress events and only use "native menu command events". But in any case, here's a very basic answer to your question: Once a Java applet has been loaded in a given browser window (for a Carbon-based browser), the JEP needs to intervene for the brower to receive _any_ keyboard or mouse events for that window (aside from mouse-moved events). (This is due to a severe design flaw in Apple's Cocoa-Carbon interface.) As far as I know the JEP now always sends the browser events that it needs to see. But there are still some cases in which it sends a given event more than once. This is one of those cases ... though (as I've already said) it's now been fixed in my current version.
> http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/utilityOverlay.xul#181 > <!-- These key nodes are here only for show. The real bindings come from > XBL, in platformHTMLBindings.xml. See bugs 57078 and 71779. --> That sounds, but something's not quite right - the real bindings aren't coming from that file on the Mac, at least, because it doesn't contain them. > Mark: is there any way I can help with completing the patch? Ping me on irc > if so. Yeah, we need to do one of two things: - In widget, find and set noProcess on the actual content node that's in nsXBLPrototypeHandler::mHandlerElement when you press one of these tempermental keystrokes like command-C. or - In nsXBLPrototypeHandler, find the command content node that we're already setting noProcess on in widget. I will be back later this evening to look into this some more.
Thanks Mark for the super rush job on this. I'll be around through the evening to start testing once you have a fix.
Attachment #230192 - Attachment is obsolete: true
Another workaround, which we can do entirely in widget/src/mac, is to walk the menu hierarchy for each keypress event and reject any keypresses that are tied to menu items. That's safe - it's exactly what the system does - but there's a slight time penalty as we'll need to walk the menu tree a second time for each keypress, and we'd need to implement the entire procedure ourselves. We can't use the system's because we aren't using the right event model for that call on the 1.8.0 branch.
I think there are a few acceptable options here: 1. Revert to the JEP used in 184.108.40.206. This would of course be sad, but we'd be in a no worse state then for 220.127.116.11, and there's always 18.104.22.168 in a few weeks. 2. Get a 095f+ JEP that does a spot-fix for this regression. But we'd have to get it fast, as in this weekend latest and even that might be too late. 3. Test a patch that does the walking of the menu items to see what the perf hit would be like. Again, we'd need such a patch urgently. I'm actually leaning towards 1 at this point. Getting 22.214.171.124 out the door is important for many other reasons then a better JEP, and there are more releases comming up where we can get the latest and greatest JEP. It's up to the other people in the release time of course.
I had forgotten about this function. It's crappy because it doesn't do a good job of distinguishing shifted key equivalents. Since we've got command-D and command-shift-D in our menus, this is powerless against preventing two "add bookmark" sheets from showing up. Other than that, it's a quick way to get the job done.
> 1. Revert to the JEP used in 126.96.36.199. Bad idea, I think. Do (as Mark suggests) take a look at the changes since JEP 0.9.5+d (the version that was bundled with Firefox 188.8.131.52). In the JEP 0.9.5+f distro, the information is in its Changes.txt and the first section of its History.txt. Particularly important are items 2, 4, 5 and 7 in the change log for 0.9.5+e and items 1 and 2 in the change log for 0.9.5+f. > 2. Get a [095+f] JEP that does a spot-fix for this regression. As it happens, this would be a one-line change. I won't do it as an official release. But I'd be happy to put together a "0.9.5+f+" and either email it to someone (Mark, presumably) or arrange a time where I can temporarily post it in a "Testing" section on my SourceForge site.
I'm now convinced that, due to other bugs in the way we handle menus, the only things we can do to resolve this problem in Mozilla (xbl, widget, anywhere) are imperfect band-aids. As such, I can't in good conscience recommend any of them, and the path of least resistance to fix the menus in a way that would enable these patches to do the right thing, although trivial, probably involves more risk than anyone's willing to take this late in the 184.108.40.206 cycle. The primary bug that causes problems is that we leave a toplevel window's native menu bar up even when we activate certain windows (like the help window) that don't have their own menu bars. If you were to use any of the patches here, open the file menu, open the help window, and press command-W, the window would not close. You can experience this bug for yourself by opening the help window and selecting File:Close from the menus instead of using a keystroke. That's the current behavior, but making it the behavior for the keystroke too would be a perceptible regression that I'm not comfortable introducing. (The solution would be to put the hidden window's menu bar up when a window without its own menu bar is active.)
Comment on attachment 230271 [details] [diff] [review] Another 1.8.0 branch fix I'd happily try a one-line fix to the JEP (comment 26). Going back to JEP v. D is more than dropping the "latest and greatest" - it reintroduces a number of crashes, some of which are easily reproducible.
Attachment #230271 - Attachment is obsolete: true
I've just created a JEP 0.9.5+f+ with my one-line fix for this bug, and tested it against the original report (comment #0), using Firefox 220.127.116.11 RC4 on both OS X 10.4.7 (PPC) and 10.3.9 (I'll test 10.4.7 Intel later). JEP 0.9.5+f+ does clear up the problem. > Load Java, open File menu, press command-P to print, then cancel the > operation. The browser crashes. > Load Java, open the Help menu, and press command-shift-? for Help. > Two help windows open. Firefox 18.104.22.168 RC4 doesn't have these problems with JEP 0.9.5+f+ ... but I wasn't able to reproduce them with JEP 0.9.5+f either. I'll create a JEP 0.9.5+f+ distro. It should be ready within an hour. Then Mark and I can arrange how I'll get a copy to Mark. (Once I've created the distro I'll also post a diff here.)
Steven: Can I get a copy of the 0.9.5+f+ build as well? You can email it to me at firstname.lastname@example.org or just zip it up and post it as an attachment to this bug. I'd like to get started testing 1505 with it so that the release team can make an informed decision about how to proceed. Thanks for all your work on this, --zach
> or just zip it up and post it as an attachment to this bug I hadn't thought of that. Seems like the best way ... but I'm afraid it might be too big (about 3.25 megs). I'll try it anyway.
Alright, it _was_ just slightly too big (and I really can't cut any more of it out). Zach, I've emailed you a copy. Feel free to share it with whoever you like. Let me know right away if there are any urgent problems. Mark, should I just email you a copy? (By the way, I can't email a copy to everyone who wants one -- my ISP wouldn't be too happy. Please share your copies as you see fit. It might be best to put a copy on some internal Mozilla.org server, and post a link here.)
(In reply to comment #32) > Mark, should I just email you a copy? Yes, please - I'll test it this evening.
>> Mark, should I just email you a copy? > >Yes, please - I'll test it this evening. Done.
Steven - thanks for your quick work on this. My own $.02 on backout to .9.5d vs. .9.5+f is that we at least understand the issues with .9.5d - wheres .9.5+f will get very little testing prior to wide release. So I'd be concerned we'd introduce an issue that is worse than one of ones you are fixing. You and Mark are the experts in this area of code - just want to make sure you undertand the risks.
The "recently" introduced regressions (basically the ones that Mark's found) were all introduced by JEP 0.9.5+e, which has been out for a while (I released it on 5-30, and it was landed on the trunk the same day, on the 1.8 branch on 5-31, and on the 1.8.0 branch on 6-13). As you call tell from its change log, 0.9.5+e was quite ambitious -- so these regressions aren't too surprising. But it's been out for long enough (and exposed to enough users) that any really serious bugs should have been discovered by now. JEP 0.9.5+f was released more recently (6-29, landed on trunk and all branches by 7-05), and so has had less exposure. But it was more conservative than 0.9.5+e, and so should be less risky. So, to my mind, you have JEP 0.9.5+d's fairly large number of known problems against what I think is a pretty low likelihood of major, undiscovered problems in 0.9.5+f (and 0.9.5+f+). I think 0.9.5+f+ wins hands down :-) Nonetheless, people working on the 22.214.171.124 release should test 0.9.5+f+ as much as they can (and I will continue to test it through the weekend).
I have been having some rather significant issues with 0.9.5+f+. When I load a simple applet (such as the bar chart example Sun provides at http://java.sun.com/applets/jdk/1.4/demo/applets/BarChart/example1.html), it often does not display--the space where the applet should appear is blank. Usually, switching Firefox to the background and back to the foreground (i.e. clicking on the desktop and then back into ff) causes the applet to suddenly appear. Pressing the back and forward buttons has also caused the applet to appear, but pressing reload causes it to disappear again. This does not happen when I go back to 0.9.5+f. I'm not entirely sure how this could happen considering the diff, so it's quite possible something is wrong on my end. I installed the new version by replacing the JavaEmbeddingPlugin.bundle file inside the .app package with the new one provided by 0.9.5+f+. Is there something else I need to do as well?
> I installed the new version by replacing the > JavaEmbeddingPlugin.bundle file inside the .app package with the new > one provided by 0.9.5+f+. Please read the Readme :-) You need to replace _both_ JavaEmbeddingPlugin.bundle and MRJPlugin.plugin. What you currently have (with your mismatched versions of those two files) is Java 1.3.1 ... which does have its own display problems.
Comment on attachment 230274 [details] Diff from JEP 0.9.5+f to JEP 0.9.5+f+ Those of you following along from home who would like to give 0.9.5+f+ a spin can pick up a copy from: http://jackassofalltrades.com/tmp/JavaEmbeddingPlugin0.9.5+f+.zip To install, replace both MRJPlugin.plugin and JavaEmbeddingPlugin.bundle in your Mozilla app bundle at Contents/MacOS/plugins with the copies in the Binaries directory of this archive.
Comment on attachment 230274 [details] Diff from JEP 0.9.5+f to JEP 0.9.5+f+ r=me for this change for 126.96.36.199. With approval, and provided QA is satisfied as well, I'd like to land this on the branch so that the build team can do the Mac respins. I spent some time following the relevant events from the user, through JEP, into the system, and into the browser. This JEP patch will cause all menu command key equivalents to be handled on key event handlers, bypassing the possibility of being processed twice by being handled on a native menu item. This is the same behavior as JEP 0.9.5+d and earlier, and the redispatch model used here is very similar to what was used in 0.9.5+d. I'd like to thank Steven for his work here. Someone, send that man a t-shirt!
(In reply to comment #35) > My own $.02 on backout to .9.5d vs. .9.5+f is that we at least understand the > issues with .9.5d - one of them being no universal binary build I believe. I'm more comfortable with f+ than going back to d assuming the above patch is an accurate representation of the difference. We were otherwise comfortable with 'f' up to this regression.
Comment on attachment 230274 [details] Diff from JEP 0.9.5+f to JEP 0.9.5+f+ can't find any other drivers, going out on a limb and guessing they'll agree, a=dveditz
Attachment #230274 - Flags: approval188.8.131.52? → approval184.108.40.206+
FYI, we'll also respin SeaMonkey 1.0.3 mac builds once this fix is checked in.
JEP 0.9.5+f+ checked in on MOZILLA_1_8_0_BRANCH for 220.127.116.11. Respins are needed for Firefox and Seamonkey on the Mac to pick up the change. No respin is needed for Thunderbird, as it does not bundle JEP. > one of them being no universal binary build I believe. 0.9.5+d was available as a universal binary, but it was the initial release and there were still a couple of x86-ppc parity bugs, as I recall. We even had a 0.9.5+c universal binary rebuild - I don't recall which one was included with 18.104.22.168.
Can confirm fix for this bug in Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060711 SeaMonkey/1.5a with JEP 0.9.5+f+ copied into the application package replacing 0.9.5+f. Thanks for the download link at http://jackassofalltrades.com/ All my test websites rendered okay except for one (http://www.bilder-seite.de.vu/ where the mouseover effects in the blue menu do not work (but this has only ever worked in Safari or when using Apple Java 1.3.1 in SeaMonkey without JEP), and some links in this menu aren't clickable [but *are* in Safari], maybe related to umlauts, or the menu not loading completely), and sometimes applets drawing in the wrong tabs, and ghost images of applets from tabs which were closed long ago. Otherwise large improvement over 0.9.5+e and f both in general application stability and website compatibility.
Verified fixed on 1505 rc5 with JEP 0.9.5+f+. Thanks again to Steven and Mark for the quick weekend work!
Would be good to get a fix in to the 1.8 branch for 1.8.1 soon.
Comment on attachment 230274 [details] Diff from JEP 0.9.5+f to JEP 0.9.5+f+ The fix would be to land the 0.9.5+f+ JEP on the 1.8 branch.
Attachment #230274 - Flags: approval1.8.1?
Steven tells me that 0.9.5+g will be out soon. I'd rather wait for that on the 1.8 [.1] branch as it will fix some other serious regressions only experienced on that branch and on the trunk.
I'm hoping to release 0.9.5+g tomorrow (Wednesday) or Thursday. I'd originally planned to release it Sunday (7-23) or Monday, but ran into some last-minute problems. The additional bmo bugs fixed by 0.9.5+g are 344006, 344153 (I think) and what's described at bmo bug 344701 comment 6 and following. There are (now) also some other, minor fixes (having to do with keyboard input problems in Java applets in multiple tabs with multiple windows).
Comment on attachment 230274 [details] Diff from JEP 0.9.5+f to JEP 0.9.5+f+ a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #230274 - Flags: approval1.8.1? → approval1.8.1+
Unless there's a +g by then, I'll land +f+ on the 1.8 branch tomorrow because this bug is preventing bug 50590 from landing. No pressure, Steven, I don't think we'll have trouble getting +g in any event because those other bugs are also pretty serious.
Checked in on MOZILLA_1_8_BRANCH, expecting another JEP update soon.
I've just released JEP 0.9.5+g: http://javaplugin.sourceforge.net/
JEP 0.9.5+g has been checked in on the trunk, this problem is now fixed on all branches.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Depends on: 346156
Resolution: --- → FIXED
Fixed by bug 346156.
You need to log in before you can comment on or make changes to this bug.