532422 - n900: Cannot select any characters from sym map

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091202 Firefox/3.6b5pre Fennec/1.0b6pre



Steps to Reproduce:
1. Click on the urlbar.
2. Press the blue arrow key + "sym"
3. Select any character from the sym map and press enter

Actual Results:
The selected character isn't added to the urlbar

Expected Results:
The selected character should be added to the urlbar

Comment 1

[Ben Combee (:bcombee)]

In talking with Stuart, current theory is that this is a focus problem -- the symbol dialog sends its keystroke when focus is no longer on the URL bar or page field.

Comment 2

[Ben Combee (:bcombee)]

Here's the xev dump for selecting the '<' key from the symbol map.  In theory, we should reenter the window correctly, but I think we lose focus somehow before the key is handled.

LeaveNotify event, serial 26, synthetic NO, window 0x3800001,
    root 0x44, subw 0x0, time 6343404, (360,7), root:(360,63),
    mode NotifyNormal, detail NotifyNonlinear, same_screen YES,
    focus NO, state 0

EnterNotify event, serial 26, synthetic NO, window 0x3800001,
    root 0x44, subw 0x0, time 6349619, (86,156), root:(86,212),
    mode NotifyNormal, detail NotifyNonlinear, same_screen YES,
    focus NO, state 0

KeymapNotify event, serial 26, synthetic NO, window 0x0,
    keys:  68  0   0   0   0   0   0   0   0   0   0   0   0   0   0   0
           0   0   0   0   0   0   0   0   0   0   0   0   0   0   0   0

Comment 3

[Doug Turner (:dougt)]

Masayuki Nakano, do you have any advice for Ben in helping track down this problem?

Comment 4

[Ben Combee (:bcombee)]

I've tracked a bit further.  I'm seeing that we get IMEComposeStart, IMEComposeEnd from the GTK layer for the symbol popup, but it doesn't turn into a character.

Here's my log output from bringing up the symbol dialog and hitting a key

IMELoseFocus 0x40f2ccd0
--- InputEvent:  blur
--- InputEvent:  blur
--- InputEvent:  blur
IMEComposeStart [0x40f2ccd0]
WARNING: mReferenceWidget is null: file /home/bcombee/src/mozilla-central/widget/src/gtk2/nsWindow.cpp, line 6785
IMEComposeText
WARNING: mReferenceWidget is null: file /home/bcombee/src/mozilla-central/widget/src/gtk2/nsWindow.cpp, line 6785
IMEComposeEnd [0x40f2ccd0]
IMESetFocus 0x40f2ccd0
--- InputEvent:  focus
--- InputEvent:  focus
--- InputEvent:  focus

I'm worried about those warnings... it seems like the IME is trying to set the cursor in the input widget, but that's not available.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #4)
> I'm worried about those warnings... it seems like the IME is trying to set the
> cursor in the input widget, but that's not available.

At that time, IMESetCursorPosition uses 'this' for the reference widget.

Probably, the focus was already lost. If so, we could fix by to change the NS_TargetUnfocusedEventToLastFocusedContent in nsGUIEvent.h.

However, there are bug 519913 and bug 497839, so, our event handling is completely broken when all windows are deactive.

Comment 6

[Ben Combee (:bcombee)]

Patch in bug 519913 doesn't apply cleanly on 1.9.2, and even with edits to allow the diff to work, it relies on behavior that's only in moz-central.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v0.1

I'm not sure this can build and has no bugs.

Comment 8

[Stuart Parmenter]

This fixes the problem

Comment 9

[Stuart Parmenter]

Masayuki:  how safe is this to land on 1.9.2?  Should we ifdef it for mobile only?  Using the hildon ifdefs for now?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Currently, we don't handle the events which NS_TargetUnfocusedEventToLastFocusedContent() returns TRUE by when all windows are deactive. And the events are retargetted to the focused element if the window is deactive.

My patch sends the events to the last focused content on the window.

So, my patch changes the event dispatching model drastically. The patch should be reviewed by smaug. And I recommend that it should be only enabled on the Hildon for minimizing the risk.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v1.0

This patch only for the 1.9.2 branch (only changes the behavior on Hildon), we should fix the bug on m-c later.

This bug is critical for mobile. So, this patch is pretty risky but we need to fix this bug.

My patch retargets all key/IME related events to the (last) focused DOM window/content in the top level window.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> This bug is critical for mobile. So, this patch is pretty risky but we need to
> fix this bug.

I wonder whether we need something similar for FF3.6.
Anyway, reviewing...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 417965 [details] [diff] [review]
Patch v1.0


>+#ifdef NS_FIX_BUG532422
>+  // This returns the focused window and the focused element under our top
>+  // level window.  I.e., when we are deactive, this returns the *last* focused
>+  // element and window.
>+  nsresult GetFocusedContent(nsIContent** aFocusedContent,
>+                             nsPIDOMWindow** aFocusedWindow);
>+#endif
This returns always the last focused element and window under the top window of
mDocument. Should this actually return the focused/active window if there is such?
That way we'd change the behavior only if there wasn't any active/focused window.
It should be less regression risky. This one looks pretty scary.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 417965 [details] [diff] [review]
Patch v1.0

Neil should look at this too.

Comment 15

[Neil Deakin]

Is the bug here that:

1. a key event needs to fire when a window is deactive, or
2. a key event needs to fire at a child window that isn't focused but may be active, or
3. an ime event needs to be handled in either if those cases

Point 1 is handled by this patch but the patch also retargeted a lot of other cases that probably shouldn't be.
Point 2 could be fixed by just disabling the if (!sDontRetargetEvents ...) line.
I don't know about 3.

4. Or is the fix actually just to change the NS_TargetUnfocusedEventToLastFocusedContent and NS_IsEventTargetedAtFocusedWindow macros to handle deactive windows differently or perhaps correctly if there is a bug in them. That is what those macros were added for, no?

As an aside, calling the static nsFocusManager::GetFocusedDescendant would make the code in the patch much simpler.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Masayuki, what is the difference between the patch in this bug and the patch
for Bug 519913?
Bug 519913 looks almost like something which should block 1.9.2.

Comment 17

[Stuart Parmenter]

Smaug: I believe this is just the 1.9.2 version of that patch.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking 1.9.2, though we need to make a decision about whether or not we want it to be IFDEF'd for Fennec only.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Bug 519913 is for trunk and I fixed some bugs of the patch in this bug's patch.

If we need to fix bug 519913, we need to retarget in the deactive window rather than to the active window because Mac needs such behavior on IME committing.

If we only fix this bug (not fix bug 519913), it may be good thing that we retarget only when there are no active window.

(In reply to comment #15)
> 1. a key event needs to fire when a window is deactive, or
> 2. a key event needs to fire at a child window that isn't focused but may be
> active, or
> 3. an ime event needs to be handled in either if those cases
> 
> Point 1 is handled by this patch but the patch also retargeted a lot of other
> cases that probably shouldn't be.
> Point 2 could be fixed by just disabling the if (!sDontRetargetEvents ...)
> line.
> I don't know about 3.

1 and 3 are needed.

> 4. Or is the fix actually just to change the
> NS_TargetUnfocusedEventToLastFocusedContent and
> NS_IsEventTargetedAtFocusedWindow macros to handle deactive windows differently
> or perhaps correctly if there is a bug in them. That is what those macros were
> added for, no?

No, because the key/IME events are discarded in the 
|if (!sDontRetargetEvents && NS_IsEventTargetedAtFocusedWindow(aEvent))| block.
If there are no focused window, it does nothing.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v2.0

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v2.1

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I tried https://build.mozilla.org/tryserver-builds/[e-mail]-try-4686755845eb/maemo/ on my N900 and sym map did seems to work.
I think that build is with patch 2.0.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v2.1 for trunk

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 418139 [details] [diff] [review]
Patch v2.1

O.K. So, this fixes:

1. retargetting to the last focused content when the all windows are deactive.
2. The IME composition is committed when a Fx window lost focus (on Windows (and perhaps on Linux too)).

However, this cannot fix the 2nd bug on Mac because the commit events are fired after completely deactivated the window.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418152 [details] [diff] [review]
Patch v2.1 for trunk

>+nsPIDOMWindow*
>+PresShell::GetFocusedDOMWindowInOurWindow()
>+{
>+  nsCOMPtr<nsPIDOMWindow> window =
>+    do_QueryInterface(mDocument->GetWindow());
>+  NS_ENSURE_TRUE(window, nsnull);
>+
>+  nsCOMPtr<nsPIDOMWindow> rootWindow = window->GetPrivateRoot();
>+  NS_ENSURE_TRUE(rootWindow, nsnull);
>+  nsPIDOMWindow* focusedWindow;
>+  nsCOMPtr<nsIContent> content =
>+    nsFocusManager::GetFocusedDescendant(rootWindow, PR_TRUE, &focusedWindow);
>+  return focusedWindow;
>+}
This leaks.
Make this method to return already_AddRefed<nsPIDOMWindow>
and change nsCOMPtr<nsIContent> content to
nsIContent* content

with that, r=me
I can't really think of anything safer for 1.9.2.

Neil should review this too.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 418139 [details] [diff] [review]
Patch v2.1

Same thing with this patch.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v2.2

Thank you, Olli.

Enn: See bug 519913 comment 14.

We should change only when all windows are deactive for the safe.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch v2.2 for trunk

Comment 29

[Neil Deakin]

> No, because the key/IME events are discarded in the 
> |if (!sDontRetargetEvents && NS_IsEventTargetedAtFocusedWindow(aEvent))| block.
> If there are no focused window, it does nothing.

So what are these macros for then?

Comment 30

[Neil Deakin]

Comment on attachment 418173 [details] [diff] [review]
Patch v2.2

Seems OK for now, but it would be good to test this a bit more. Especially with regards to things like pressing keys/using ime before and after minimizing windows and opening dialogs and so forth.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #29)
> > No, because the key/IME events are discarded in the 
> > |if (!sDontRetargetEvents && NS_IsEventTargetedAtFocusedWindow(aEvent))| block.
> > If there are no focused window, it does nothing.
> 
> So what are these macros for then?

Before the focus refactoring, the events are not discarded.

(In reply to comment #30)
> (From update of attachment 418173 [details] [diff] [review])
> Seems OK for now, but it would be good to test this a bit more. Especially with
> regards to things like pressing keys/using ime before and after minimizing
> windows and opening dialogs and so forth.

I'll do it tomorrow before I land them.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

It's 3:00 AM JST, so, I cannot watch tinderbox even if I land them now. If the mobile team cannot wait my check-in, please land them yourself.

Comment 33

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/8ad4bf85501b
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/216cb2db5f62

Thanks Masayuki for the awesome help and Neil for the quick reviews.  Fennec loves you.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 418173 [details] [diff] [review] [details])
> > Seems OK for now, but it would be good to test this a bit more. Especially with
> > regards to things like pressing keys/using ime before and after minimizing
> > windows and opening dialogs and so forth.
> 
> I'll do it tomorrow before I land them.

I tested easily, but I didn't find any problems, thank you.

And thank you, dougt.

Comment 35

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091221 Firefox/3.6b5pre Fennec/1.0b6pre

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 418175 [details] [diff] [review]
Patch v2.2 for trunk

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>+  nsIContent* content =
>+    nsFocusManager::GetFocusedDescendant(rootWindow, PR_TRUE, &focusedWindow);
>+  return focusedWindow;
>+}

This change introduced the following build warning on mozilla-central:
>../../../mozilla/layout/base/nsPresShell.cpp: In member function ‘already_AddRefed<nsPIDOMWindow> PresShell::GetFocusedDOMWindowInOurWindow()’:
>../../../mozilla/layout/base/nsPresShell.cpp:5962: warning: unused variable ‘content’

If |content| is intentionally unused, perhaps the first quoted line above ("nsIContent* content =") should just be removed?  There's no point in storing the return value in a local variable, if you're not going to use it.

Or, is |content| actually supposed to be used? (null-checked as a measure of success, or anything like that?)

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, just we can remove it. I'll post the followup patch.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch for comment 36

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

http://hg.mozilla.org/mozilla-central/rev/b919e8a48890

landed the additional patch only on trunk.