Closed Bug 428405 Opened 12 years ago Closed 12 years ago

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

Categories

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

PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: cmhofman, Assigned: smichaud)

References

Details

(Keywords: fixed1.9.0.2, regression, Whiteboard: [MU?])

Attachments

(3 files, 5 obsolete files)

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
Summary: Close Rab shortcut fails → Close Tab shortcut fails
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 426139
I'm seeing this on the last couple of nightly builds on OS X 10.5.2
Status: RESOLVED → UNCONFIRMED
Flags: blocking-firefox3?
Resolution: DUPLICATE → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Please renominate if we can get a regression range or a clear STR...
Flags: blocking-firefox3?
Keywords: qawanted, regression
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.
Version: unspecified → Trunk
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?
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.
Assignee: nobody → joshmoz
Component: Tabbed Browser → Widget: Cocoa
Keywords: qawanted
Product: Firefox → Core
QA Contact: tabbed.browser → cocoa
Flags: blocking1.9?
I hit this quite often; however, I think it's not something that should hold the entire release.  Please re-nom if you disagree.  
Flags: blocking1.9? → blocking1.9-
Flags: wanted1.9.0.x+
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.
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.
Priority: -- → P1
Attached patch Potential fix (obsolete) — Splinter Review
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.
Assignee: joshmoz → smichaud
Status: NEW → ASSIGNED
Attachment #320819 - Flags: review?(joshmoz)
> 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.
> 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.
Attachment #320830 - Flags: review?(joshmoz)
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.
Attachment #320819 - Attachment is obsolete: true
Attachment #320819 - Flags: review?(joshmoz)
Duplicate of this bug: 434732
Attachment #320830 - Flags: superreview?(roc)
Attachment #320830 - Flags: review?(joshmoz)
Attachment #320830 - Flags: review+
Comment on attachment 320830 [details] [diff] [review]
Potential fix (alternate version)

But guys ... we need to be able to test this.
Attachment #320830 - Flags: superreview?(roc) → superreview+
> 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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attached file 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.
Attached file Mochitest for this bug (obsolete) —
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.
Attachment #329128 - Flags: review?(joshmoz)
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.
Attachment #329128 - Flags: review?(joshmoz)
Attached patch Chrome test for this bug (obsolete) — Splinter Review
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.
Attachment #329128 - Attachment is obsolete: true
Attachment #331357 - Flags: review?(joshmoz)
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.
Flags: blocking1.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.
Attachment #320830 - Attachment is obsolete: true
Whiteboard: [MU?]
Attachment #333981 - Flags: approval1.9.0.2?
Blocks: 425716
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
Attachment #333981 - Flags: approval1.9.0.2? → approval1.9.0.2+
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
Keywords: fixed1.9.0.2
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.
Attachment #331357 - Attachment is obsolete: true
Attachment #334754 - Flags: review?(joshmoz)
Attachment #331357 - Flags: review?(joshmoz)
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.
Summary: Close Tab shortcut fails → Some keyboard shortcuts fail after reloading a tab in the background and switching back to it
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!
Flags: blocking1.9.0.2?
Attachment #334754 - Attachment is obsolete: true
Attachment #335416 - Flags: review?(joshmoz)
Attachment #334754 - Flags: review?(joshmoz)
Attachment #335416 - Flags: superreview?(roc)
Attachment #335416 - Flags: review?(joshmoz)
Attachment #335416 - Flags: review+
Attachment #335416 - Flags: superreview?(roc) → superreview+
Landed my rev2 chrome test (attachment 335416 [details] [diff] [review]) on mozilla-central.

Is this also wanted on the 1.9.0 branch?
Depends on: 591747
No longer depends on: 591747
You need to log in before you can comment on or make changes to this bug.