User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre Half of the time the shortcut for the Close Tab menu item (Command-W) fails to work. Reproducible: Sometimes Steps to Reproduce: 1. open a few tabs in a browser window 2. press Command-W 3. Actual Results: Nothing Expected Results: The current tab should close

I'm seeing this on the last couple of nightly builds on OS X 10.5.2

Please renominate if we can get a regression range or a clear STR...

I've seen this too, and I suspect bug 427559 since I think I only noticed it after that landed. I haven't been able to get reliable STR, though - I usually notice it when Cmd+Opt+Arrowing to a process_bug.cgi page where I had previously submitted a change to a bug, and trying to close it (I often Submit a bug change then switch away while process_bug loads). Cmd+W just does nothing until I click the page, to give it focus. I think other shortcuts are broken too. Could it be that the whatToFocus default of window.content breaks shortcuts somehow, if the check added in bug 427559 fails?

Hmm, I might have found steps to reproduce: 1) Load a bug, then click the submit button 2) Before process_bug has finished loading, Cmd+T to open a new tab. 3) Wait for process_bug.cgi to finish loading. 4) Load a page in that new tab, and before *it* has finished loading, Cmd+Opt+Arrow back to the loaded process_bug.cgi tab. After 4, Cmd+W doesn't work, and I can't Cmd+Opt+Arrow away from the tab.

There were some changes to how keyboard commands were handled on mac, so that might be related.

That didn't seem to work for me.. 1) Submitted 2) Cmd-T right away 3) Wait and load planet.m.o 4) Cmd-Opt-Left before planet loads 5) Cmd-Opt-Right still works

gavin: If you open up domI, can you inspect the browser -> document -> commandDispatcher focusedElement/Window? Additionally, for the broken tab's browser, check focusedElement?

Alt+`

Switching away from the window with the stuck key (to DOMi) and switching back "fixes" it. Backing out the patch for bug 427559 doesn't fix it.

I hit this quite often; however, I think it's not something that should hold the entire release. Please re-nom if you disagree.

I disagree++ When one of the most commonly used keyboard shortcuts doesn't work consistently -- and for me it's pretty much any time I return to a tab that contains a page that's a result of a form submit, that's a major issue. If this is as widespread as I believe it to be, we really, absolutely, cannot ship with this.

(In reply to comment #5) For what it's worth, I'm also able to reproduce this STR (in today's Minefield nightly on OS X 10.5.2). I can't repro in today's Camino trunk nightly.

(Following up comment #5) I should mention the pages I tested with. The bug I used in step 1 was this bug. The page I loaded in the new tab in step 4 was my favorite example of a grossly overweight Flash monstrosity -- http://www/vg.no.

http://www.vg.no

Testing with today's Minefield nightly (on OS X 10.4.11), I can no longer reproduce Gavin's STR from comment #5. Gavin, how about you?

> Testing with today's Minefield nightly (on OS X 10.4.11), I can no > longer reproduce Gavin's STR from comment #5. But I _am_ still able to reproduce it on OS X 10.5.2. (And I'm not sure at this point I ever tested on 10.4.11 before.)

I still see this all the time. I'm running 10.5.2.

Here's a patch that (in my very limited testing) fixes this bug. But, since it changes key handling, we'll need a lot more testing before we can be confident that it doesn't cause other problems. In particular it needs testing with oddball key combinations on non-US keyboard layouts. That said, I think this patch has the potential to fix more than just this bug. The specific problem it addresses is as follows: Previously there was code in [ChildView performKeyEquivalent:] that handed things off to other code if "self" (a ChildView object) wasn't the first responder. This makes sense if the first responder is another ChildView object. But I'm pretty sure it doesn't make sense if the first responder is some other kind of object (say an NSWindow object, or an NSView object that isn't a ChildView). Among other things, doing this with an NSWindow first responder makes it possible for performKeyEquivalent: to be called twice on the same NSWindow object for the same (NSKeyDown) event -- once from [NSApplication _handleKeyEquivalent:] and once from [ChildView performKeyEquivalent:]. I think it makes better sense to just ignore the first-responder-state if the "other" first responder isn't a ChildView object -- to trust that the OS knows what it's doing when it calls performKeyEquivalent: on us anyway. Josh, you seem to be the one who originally wrote this chunk of code. Do you remember what problem(s) it solved? If so, we should test that my patch hasn't reopened them. A tryserver build will follow in an hour or two.

> Josh, you seem to be the one who originally wrote this chunk of > code. Do you remember what problem(s) it solved? If so, we should > test that my patch hasn't reopened them. I've now taken a closer look, and it seems this code was part of a fix for bug 358379, bug 418226, bug 411304 and bug 417108. As best I can tell my patch doesn't regress any of these.

Here's the tryserver build: https://build.mozilla.org/tryserver-builds/2008-05-13_15:50-smichaud@pobox.com-bugzilla428405/smichaud@pobox.com-bugzilla428405-firefox-try-mac.dmg

> I think it makes better sense to just ignore the > first-responder-state if the "other" first responder isn't a > ChildView object -- to trust that the OS knows what it's doing when > it calls performKeyEquivalent: on us anyway. Here's a slightly different alternative patch: It trusts the OS only if the "other" first responder isn't an NSView object at all. I'll post another tryserver build in a bit.

Here's the tryserver build for my alternate patch: https://build.mozilla.org/tryserver-builds/2008-05-13_17:09-smichaud@pobox.com-bugzilla428405-alt/smichaud@pobox.com-bugzilla428405-alt-firefox-try-mac.dmg

Comment on attachment 320819 [details] [diff] [review] Potential fix I've decided I prefer my "alternate" patch -- mostly because it makes a smaller change. But if someone discovers problems testing the "alternate" patch, please also try the "original" patch.

Comment on attachment 320830 [details] [diff] [review] Potential fix (alternate version) But guys ... we need to be able to test this.

> But guys ... we need to be able to test this. We don't yet have automated tests for Cocoa widgets. But this bug does have a reliable STR (from comment #5), and there's a tryserver build made with this patch (in comment #23). So we do have a (manual) way to find out whether my patch fixes this bug (the same thing provided by automated tests on other modules). Whoever's listening here and can easily reproduce the STR from comment #5, please test the tryserver build from comment #23 and report your results (particularly if you still see the bug).

Did this ever get fixed? The file from comment#23 doesn't exist.

> Did this ever get fixed? Not yet. > The file from comment#23 doesn't exist. That's because tryserver builds are deleted after 30 days. Here's another, made with a revision of my alternate patch (attachment 320830 [review]) that brings it up to date with current code. https://build.mozilla.org/tryserver-builds/2008-06-23_08:47-smichaud@pobox.com-bugzilla428405-alt-rev1/smichaud@pobox.com-bugzilla428405-alt-rev1-firefox-try-mac.dmg

I'm going to try to create an automated test for this patch (though I'm not sure that's possible with our current testing capabilities). In the meantime, can this land on mozilla-central first, to give it time to bake there?

Landed on mozilla-central.

Here's a reduced testcase for this bug. The instructions are in the testcase itself, but I'll repeat them here (and expand them a little) for the sake of clarity: 1) Hit 'Enter' or click on 'Hit Enter'. A new page will open in the second tab, then the first tab will reload in the background (while the second tab is focused). 2) Wait a few seconds in the second tab for the first tab to finish reloading (in the background). 3) Press cmd-opt-left-arrow to return to the first tab. 4) Try cmd-opt-right or cmd-t in the first tab. If bug 428405 still exists they should fail -- as they do in current 1.9.0-branch builds. If bug 428205 has been fixed (as it has in current mozilla-central builds) they should work.

After many false starts, and by the skin of my teeth, I've been able to fashion a Mochitest for this bug that works properly -- it fails when it should, and works when it should. It's modeled on the my reduced testcase from comment #32. It should be marked to only run on OS X. I found (no big surprise) that the standard Mochitest synthesizeKey() method doesn't work for this test. But luckily the nsIDOMWindowUtils::SendNativeKeyEvent() method recently added by roc does work properly. Though I suspect that this won't always be the case, and that Josh is right about what he says in bug 431503 comment #17. My results point out the need for a nsIDOMWindowUtils::SendNativeMouseEvent() method. Sometime over the next few months, I hope to be able to do an OS-X-specific implementation of this. (Someone else would need to do the Windows and GTK implementations.)

Maybe the EventUtils methods should use those as well?

(In reply to comment #34) I suppose there could be a synthesizeNativeKey() method (and eventually a systhesizeNativeMouse() method) in EventUtils. But I suspect the "native" variant of the synthesizeKey() method (at least) will always be more difficult to use, since it's inevitably more OS-specific. So it'll (probably) always make sense to use the non-native methods if you can, and the native ones only if you have to.

Comment on attachment 329128 [details] Mochitest for this bug This test relies on Firefox being set up in a particular way. We need a test that is not dependent on Firefox, a XUL doc that has its own tab widget and keyboard commands. Tests should be posted in patch form.

I'm very lucky to have managed to create a working test that uses its own tabs (instead of using the browser's own tabs, belonging to the built-in <tabbrowser> object). It fails when it should and succeeds when it should. It's very unlikely this strategy will work for all tabbing-related bugs. But we can worry about that later. It was strongly recommended that I avoid using setTimeout() (it doesn't behave well on the testboxes, and can produce unpredictable results). I've managed to do this, at the cost of making my test's logic somewhat convoluted.

Comment on attachment 331357 [details] [diff] [review] Chrome test for this bug + <!-- test code goes here --> Remove this. + function setGlobals() { I don't like that loadFirstTab() is called from within setGlobals(). The name setGlobals() shouldn't do more than it implies. Just call loadFirstTab() right after calling setGlobals(). + function synthesizeNativeEnterKey() { + gUtils.sendNativeKeyEvent(0, 36, 0, "\u000d", "\u000d"); + } Please document at this sendNativeKeyEvent callsite what exactly those numbers mean. We shouldn't be running tests on the browser chrome itself from widget code. Please remove all of the "tabbrowser" test stuff and make sure that we are still effectively testing the bug without it. The other tests should still fail without your patch and succeed with it. It concerns me that there are no calls to the testing function "ok" in this test. You need to test conditions by calling "ok". See all of the other tests in widget. Please ensure that your test leaves the browser in its original state when concluded. Right now, that is not the case. There is also the issue of your creating a new window. If you want to do that, which is probably wise here, please see the following example: test_native_menus.xul native_menus_window.xul The test file, 'test_native_menus.xul', opens a new window which is based on the contents of 'native_menus_window.xul'. At the end of the test the new window is closed and thus the original window is left alone.

I think this needs to block 1.9.0.2, and I think we should take the patch on the 1.9.0 branch ASAP. IMHO, this is the worst bug in Firefox 3.0.1 for English users and we should not ship a major update without fixing it. See comments 11 and 12. It can be really frustrating for users with certain tabbed browser usage patterns, and the bug is not limited to tabbed browsing. The patch here has been on trunk for 18 days with no regressions reported. The test on this bug works, though it is not ready for check-in. Steven is working on it right now and we'll have it ready soon. In the mean time we're missing out on QA on the 1.9.0 branch, and like I said, I think we do need to take this there for 1.9.0.2.

(In reply to comment #38) > It concerns me that there are no calls to the testing function "ok" > in this test. Sure there is -- the following call in finishTest(), (which accomplishes the same purpose as ok()): is(gTabOpened, true); > Please ensure that your test leaves the browser in its original > state when concluded. Right now, that is not the case. Not true. In all my tests it _does_ leave the browser in its original state. Maybe you had problems testing my test? > There is also the issue of your creating a new window. My test _doesn't_ create a new window -- it uses cmd-t to (try to) open a new tab (a tabbrowser tab). Then it tests whether or not that tab was successfully opened (which determines whether or not the bug has been fixed). Since you want me to get rid of the tabbrowser stuff, I'll have to come up with a new test. Conceivable I could test whether cmd-n opens a new window (though I'm not sure, off the top of my head, whether the bug prevents cmd-n from working).

Here's a revision of my "alt" patch (attachment 320830 [details] [diff] [review]) for current 1.9.0-branch code. There've been no other changes.

Comment on attachment 333981 [details] [diff] [review] Alternate fix, rev2 (update for current code) Let's go ahead and take this on 1.9.0.x asap so we can get some baking. Definitely want the test to land as soon as possible though. Approved for 1.9.0.2. a=ss

Landed on the 1.9.0 branch: Checking in widget/src/cocoa/nsChildView.mm; /cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v <-- nsChildView.mm new revision: 1.363; previous revision: 1.362 done

After talking to Josh a couple of days ago, I got a clearer idea of what he wants. Here are two chrome tests for this bug. They use different strategies (one tests in a separate window, the other doesn't). Both also had to work around wierdness elsewhere in the browser (things that should work but don't). One of these problems is clearly a bug (the fact that onload handlers don't work in <browser> elements in a separate window). When I have time I'll open a new bug and report the number here. We clearly only need one of these tests. Josh, pick your favorite and I'll post a revised patch.

I hate to say this, but I still see this issue from time to time (in mozilla-central nightlies). I haven't found exact steps to reproduce, but it's on my radar.

(In reply to comment #45) This precise problem (as defined in comment #5 and comment #32) is now fixed. > I haven't found exact steps to reproduce When you do, please open a new bug.

Comment on attachment 334754 [details] [diff] [review] Two more chrome tests for bug 428405 I'm OK with the in-window test, please remove the test that is in a new window and rename the in-window test file without "_alt". Thanks!

Landed my rev2 chrome test (attachment 335416 [details] [diff] [review]) on mozilla-central. Is this also wanted on the 1.9.0 branch?