428405 - Some keyboard shortcuts fail after reloading a tab in the background and switching back to it

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Christiaan Hofman]

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

Comment 2

[Asa Dotzler [:asa]]

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

Comment 3

[Mike Connor [:mconnor]]

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

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 6

[Ed Lee :Mardak]

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

Comment 7

[Ed Lee :Mardak]

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

Comment 8

[Ed Lee :Mardak]

gavin: If you open up domI, can you inspect the browser -> document -> commandDispatcher focusedElement/Window?

Additionally, for the broken tab's browser, check focusedElement?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Alt+`

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 11

[Damon Sicore (:damons)]

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

Comment 12

[Asa Dotzler [:asa]]

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. 

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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

Comment 15

[Steven Michaud [:smichaud] (Retired)]

http://www.vg.no

Comment 16

[Steven Michaud [:smichaud] (Retired)]

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?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

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

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Attachment: Potential fix

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.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

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

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Here's the tryserver build:

https://build.mozilla.org/tryserver-builds/2008-05-13_15:[e-mail]-bugzilla428405/[e-mail]

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Attachment: Potential fix (alternate version)

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

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Here's the tryserver build for my alternate patch:

https://build.mozilla.org/tryserver-builds/2008-05-13_17:[e-mail]-bugzilla428405-alt/[e-mail]

Comment 24

[Steven Michaud [:smichaud] (Retired)]

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 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 320830 [details] [diff] [review]
Potential fix (alternate version)

But guys ... we need to be able to test this.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

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

Comment 28

[James Spencer]

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

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> 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:[e-mail]-bugzilla428405-alt-rev1/[e-mail]

Comment 30

[Steven Michaud [:smichaud] (Retired)]

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?

Comment 31

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Attachment: Reduced testcase

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.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Attachment: Mochitest for this bug

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

Comment 34

[Shawn Wilsher :sdwilsh]

Maybe the EventUtils methods should use those as well?

Comment 35

[Steven Michaud [:smichaud] (Retired)]

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

[Josh Aas]

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.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

Attachment: Chrome test for this bug

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 38

[Josh Aas]

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.

Comment 39

[Josh Aas]

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.

Comment 40

[Steven Michaud [:smichaud] (Retired)]

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

Comment 41

[Steven Michaud [:smichaud] (Retired)]

Attachment: Alternate fix, rev2 (update for current code)

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 42

[Samuel Sidler (old account; do not CC)]

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

Comment 43

[Steven Michaud [:smichaud] (Retired)]

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

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Attachment: Two more chrome tests for bug 428405

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.

Comment 45

[Shawn Wilsher :sdwilsh]

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.

Comment 46

[Steven Michaud [:smichaud] (Retired)]

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

[Josh Aas]

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!

Comment 48

[Steven Michaud [:smichaud] (Retired)]

Attachment: Chrome test for this bug, rev2 (just use in-window test)

Comment 49

[Steven Michaud [:smichaud] (Retired)]

Landed my rev2 chrome test (attachment 335416 [details] [diff] [review]) on mozilla-central.

Is this also wanted on the 1.9.0 branch?