601440 - Cannot enter Return key in location/search bar when input field was focused

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[eee.dha]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ja-JP-mac; rv:1.9.2.11) Gecko/20101001 Firefox/3.6.11
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ja-JP-mac; rv:1.9.2.11) Gecko/20101001 Firefox/3.6.11

Pressing return key in location/search bar is captured by some type of website's input fields.

Reproducible: Always

Steps to Reproduce:
1. Open http://www.yahoo.com/
2. Press Command+L
3. Type any URL
4. Press Retrun key
Actual Results:  
Yahoo starts searching.

Expected Results:  
Open the URL entered at step 3.

Works fine:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.11pre) Gecko/20100920 Namoroka/3.6.11pre
Not:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.11pre) Gecko/20100921 Namoroka/3.6.11pre

Something wrong in
http://hg.mozilla.org/releases/mozilla-1.9.2/pushloghtml?fromchange=bc021a2ec6a9&tochange=c0c261de23ec

Comment 1

[eee.dha]

This bug doesn't occur on Fx 3.5.14 beta.  It seems that the fix of bug 548480 causes this bug and bug 602161.  And bug 597389 makes it noticeable.

Comment 2

[Makoto Kato [:m_kato]]

Humm, when I backed out bug 597389, it seems to work fine.

TSM uses invalid object???

Comment 3

[Makoto Kato [:m_kato]]

Nakano-san, bug 597389 is yours.  We should consider new fix with bug 597389 (Flash 10.1 regression) + bug 548480 (Top #1 crasher in Thunderbird 3.1 and there is some reports on security device password in Firefox 3.6).

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

If bug 597389 causes this bug, why isn't this reproduced on 3.5.14? The combination of bug 597389 and bug 548480 is the cause of this bug??

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

And I have no idea what is happening by the patches.

There are two differences between trunk and branch.

1. Trunk uses new APIs but branch uses ancient API (KeyScript).
2. Trunk sets the state after the focus is completely moved but branch sets the state during focus moving.

I'll try to create a patch.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Um, the #2 isn't matter. Even if I call KeyScript() with delay, this bug cannot be fixed. I guess that this bug isn't new regression, this was hidden by the if statement which was removed by bug 597389.

If we cannot fix this bug easily, we should backout the patch of bug 597389 which is NOT our fault.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Oops, I tested with wrong STR.

Even if I backedout only the patch of bug 548480, I cannot reproduce this bug. So, this is a new regression.

And I retested my patch, then, it can fix this bug! So, I'll post the patch soon.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.0

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.0.1

This calls KeyScript() (and [NSInputManager currentInputManager]) after focus moving is finished. This works fine for me.

Comment 10

[Mark Banner (:standard8)]

So how much is this going to affect users using the FF 3.6.11 and/or TB 3.1.5 releases?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to comment #10)
> So how much is this going to affect users using the FF 3.6.11 and/or TB 3.1.5
> releases?

I'm not sure. I guess that the focused native widget's key events go to old focused native widget by the API's bug. So, this regression can be dangerous in some cases.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 482137 [details] [diff] [review]
Patch v1.0.1

 nsChildView::~nsChildView()
 {
+  if (--gWidgetCount == 0) {
+    nsTSMManager::Shutdown();
+  }

With this code, nsTSMManager::Shutdown() will be called whenever the
number of open windows falls to zero.  But this can happen without
quitting -- for example when the user closes all open windows.

It's probably better to call nsTSMManager::Shutdown() from
nsAppShell::Exit(), just before or after the call to
NS_RemovePluginKeyEventsHandler().

+nsITimer* nsTSMManager::sTimer = nsnull;

The name should probably be more specific.  How about
"sSyncKeyScriptTimer"?

Comment 13

[Daniel Veditz [:dveditz]]

We need a working, reviewed patch immediately. We need to include this fix in 3.6.11, and our current plan was to ship this Thursday, Oct 14. Obviously that is blown at this point but the clock is ticking.

eee.dha-- thank you for catching this regression before we shipped with it. This would have annoyed a lot of people.

Comment 14

[LegNeato]

Masayuki won't be up for a couple of hours yet...

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v2.0

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7023412fa93c

I pushed to 1.9.2 branch.

Is there a 1.9.2.11 branch??

Comment 17

[LegNeato]

Yep, GECKO19211_20100930_RELBRANCH in mozilla-1.9.2. Please land it there as well.

Comment 18

[LegNeato]

(there are other relbranches in there for TB and SM but I trust they will sort the landings out on their own).

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2cfbdde77508

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Verified fixed on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This caused a 2.3% Ts regression on Camino's 10.4 tinderbox (but not on any of our 10.5 tinderboxen).  Is this patch doing some sort of work at startup only on 10.4?

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to comment #22)
> Is this patch doing some sort of work at startup only
> on 10.4?

No, the patch delays an API call for controlling keyboard layout from "during focus moving" to "after focus moved".

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I filed bug 604455 to cover the Ts regression.