52416 - Editor does not accept NS_TEXT_EVENT while losing input focus

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Masaki Katakai]

UNIX IMEs sometimes invoke extra windows for operation,
for example, candidate window which grabs input focus.
As I described the problem in 

http://bugzilla.mozilla.org/show_bug.cgi?id=29065

IME wants to draw pre-edit text (composed text) while
the candidate window is invoked.

It seems that Editor does not accept NS_TEXT_EVENT
for updating pre-edit text by the recent changes in
Editor and Layout (/event?). When I filed a bug 29065,
Editor could accept NS_TEXT_EVENT, so I could see
the pre-edit text was updated event when candidate
window is invoked. However, it does not work on the
current build.

How to reproduce:

1) Start Mozilla
2) Visit www.yahoo.com
3) Click on text field for Search
4) Turn conversion on by Shift+SPACE
5) Type a i
6) Invoke candidate window by SPACE, SPACE
   Main window of Mozilla will loose the input focus

7) Select a candidate
   candidate window will be closed and the selected text
   will be drawn on the text field, but it won't.
   The first candidate word is still displayed. 

8) type return to commit the word
   The correct word is committed, which means the word
   drawn in 7) is not correct.

I'd like to ask Editor and Layout folks to evalute this problem.
Problem of kinput2 (filed as 47568) seems to happen due to this.

Comment 1

[Frank Tang]

reassign to kin. add [e-mail] to the cc list
kin- do you know what happen about this ?

Comment 2

[rubydoo123]

setting to m19, Kin please take a look and see what is up, we will evaluate 
after Kin has preformed a preliminary debug

Comment 3

[kinmoz]

Accepting bug.

I'm going to need some help setting IME up on my Linux box.

Comment 4

[Hidetoshi Tajima]

Kin - Is somebody helping to setup IME on your Linux?
Also, since this is i18n related problem, I suggest to assign
[e-mail] or [e-mail] for QA concact.

Comment 5

[rubydoo123]

future

Comment 6

[Linda Baliman]

Adding intl and nsbeta1 keywords.

Adding mcarlson to cc-list.

Comment 7

[kill this account]

I see slightly different behavior.

 export LANG=ja_JP.eucJP
 jserver
 kinput2 &

 ./mozilla

1) click in the URL bar.
2) shift-space to engage IME
3) type "watashi"
4) type Ctrl-w twice to bring up candidate window

If I use the keyboard to navigate the candidate window
(space to advance, return to select) the new character(s)
is displayed.

If I use the mouse to select a candidate the new character(s)
is not displayed until I click in the URL bar.

Comment 8

[kill this account]

Katakai-san, is your window focus model: pointer or click?

Would someone at sun check if the display is wrong when:

1) just using the keyboard to select the kanji
or
2) using the pointer to select the kanji

Comment 9

[Brendan Eich [:brendan]]

[e-mail] is blocked by this bug.  Any ideas on unblocking?

/be

Comment 10

[Masaki Katakai]

*** Bug 74823 has been marked as a duplicate of this bug. ***

Comment 11

[Masaki Katakai]

*** Bug 74825 has been marked as a duplicate of this bug. ***

Comment 12

[Jaime Rodriguez, Jr.]

changing to nsbeta1-. this one does not meet beta stopper guidelines.

Comment 13

[Jaime Rodriguez, Jr.]

Beppe - I don't we should future this one, it has multiple dupes and is a 
blocker. Pls reconsider for M0.9.2.

Adding nsCatFood and RTM keyword.

Comment 14

[Masaki Katakai]

This problem is still serious when users use enlightenment and
CDE window manager because there is no way to configure IME
candidate window not to grab input focus.

The problem is that event with NS_TEXT_EVENT does not
go to the *last focused* content. I'm now debugging
this problem and need you help. It seems that IME works
well on HTML editor but does not work on any input
field of XUL docs and HTML form. I found the following

mCurrentEventContent = mDocument->GetRootContent();

in

http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsPresShell.cpp#5399

does not return correct content while it's losing
input focus. On HTML editor, it returns correct
value which is same wiht HTML editor itself.

However, on HTML form, the GetRootContent() returns
the parent(?) content of HTML form field, not
the exact content of HTML form field.

Is there any way to get the *last focused* content
(e.g. input form) here? Or if it is OK to return
the parent content here but can we pass the event
to the input form finally?

Comment 15

[kinmoz]

This sounds like this is a Focus/Event problem, not an editor problem.

Cc'ing the focus gurus ... [e-mail] and [e-mail]

Anyone want to take this off my plate?

Comment 16

[saari (gone)]

Editor *will* get a blur event if the window is deactivated. Can the editor
accept these events when blurred? Have you verified that it gets them, or does
the event never make it that far?

Comment 17

[Masaki Katakai]

I understand editor can accept NS_TEXT_EVENT events while losing focus
because this problem does not happen on HTMLEditor (Composer). This problem
happens on any text field of GUI and HTML forms.

When input field has an input focus,

        manager->GetFocusedContent(&mCurrentEventContent);

can return the correct and exact content of nsHTMLInputElement. If not,

                mCurrentEventContent = mDocument->GetRootContent();

can not return the exact content of the *last focused* nsHTMLInputElement.

Is it possible to get the *last focused* content here?

Comment 18

[Masaki Katakai]

I've verified this problem does not happen on M16 but M17 has this problem.

Comment 19

[Masaki Katakai]

Should we change Summary to "layout does not throw NS_TEXT_EVENT event
to nsHTMLInputElement while losing focus" ?

Comment 20

[Shotaro Kamio]

I found one questionable point on source codes. 
(But, I don't know this is related to this bug.)
Try to search by NS_TEXT_EVENT.
http://lxr.mozilla.org/seamonkey/search?string=NS_TEXT_EVENT
There are many codes like "event->message == NS_TEXT_EVENT".
However, according to the struct nsEvent,
NS_TEXT_EVENT must be in member "eventStructType", not "message".
http://lxr.mozilla.org/seamonkey/source/widget/public/nsGUIEvent.h#88

Are these codes mistakes?

Comment 21

[Shotaro Kamio]

Did anyone see my comment above ?

Comment 22

[Masaki Katakai]

Thank you, Kamio-san,

widget side always sets both message and
eventStructType to NS_TEXT_EVENT. So I don't
think this causes problem.

/widget/src/gtk/nsWindow.cpp, line 3700 -- textEvent.message =
textEvent.eventStructType = NS_TEXT_EVENT;

Comment 23

[Masaki Katakai]

Attachment: not patch, it's workaround codes

Comment 24

[Masaki Katakai]

I've attached the codes for workaround.

The problem is that if losing input focus, editor
does not accept any NS_TEXT_EVENT so we're seeing
two problems,

 - composed characters are not updated when
   we choose a candidate on candidate window.
   when the candidate window grabs input focus.

 - commit string on candidate window does not
   work because commit event comes before mozilla
   grabs input focus again from candidate window

There is no way to solve problem #1 by IME side,
but for problem #2, we can store committed string
as pending string then commit it by the input
focus is back. I understand this is not really
good way to solive this, but it can be workaround
for reference. #2 is very serious issue.

Comment 25

[Masaki Katakai]

Attachment: patch for nsPresShell.cpp

Comment 26

[Masaki Katakai]

patch for widget side is not working well on composer,

http://bugzilla.mozilla.org/attachment.cgi?id=76393&action=view

because focus is not back to "html editor pane" when
getting focus again... Actually there is not way
for widget side to fix this problem...

I tried to modify PresShell::HandleEvent() of nsPresShell.cpp.

manager->GetFocusedContent(&mCurrentEventContent);

Is the focus in Mozilla, mCurrentEventContent
is returned. This is correct.

If not, trying to get mCurrentEventContent by,

mDocument->GetRootContent(&mCurrentEventContent);

this works on only composer. On HTML form and
any XUL UI, it does not work. When input focus
is not on Mozilla, this code is used but
which will not send NS_TEXT_EVENT to proper
contents.

How about the patch? If the input focus
exactly is not on Mozilla, get the last
focused-content by using GetFocusedElement().

If not, continue to use mDocument->GetRootContent().

Kin, what do you think? logically correct?

Toshi, can you try on your environment?

Comment 27

[Masaki Katakai]

so, this is not editor bug, I think.
Is it better to change component to layout or event handling?

Comment 28

[kinmoz]

Right, the bug is not an editor or layout problem, it's an event 
handling/dispatching problem.

I'm going to defer to the focus/event handling gurus on this one, since I don't 
know the full extent of what could be affeted by your patches.

saari, aaronl, or joki ... any comments?

Comment 29

[Masaki Katakai]

Could anyone review the patch and give comments please?

Comment 30

[kinmoz]

Since we know this isn't an editor problem, I'm going to hand this one over to
saari so it shows up on his radar.

Comment 31

[Margaret Chan]

[e-mail]:  Can you or someone take a look of this patch for us
please?  We want someone in this area to check out the patch because we need
this ASAP for Sun's release.  Thanks, Margaret

Comment 32

[Aaron Leventhal]

Chris Saari is on vacation until Tuesday.

Comment 33

[Masaki Katakai]

Can anyone review the patch?

I've verified the patch is working fine with the
current tree.

Comment 34

[Margaret Chan]

We need this review please.  Can someone give us a pointer to a reviewer for
this bug?  

Comment 35

[Roland Mainz]

CC:'ing some editor folks...

Comment 36

[Margaret Chan]

Hi Roland.  Thanks for cc'ing to others.  Any chance that you can help getting
this in?  We need it badly.  Margaret

Comment 37

[Kathleen :Brade]

I am concerned with the last change; it sounds like that will break something on
Macintosh (maybe keybindings?).

Comment 38

[Masaki Katakai]

Attachment: use defined(XP_UNIX)

Thank you very much for comment, 

I don't have build environment for Mac, so I can not verify this.

OK, this problem happens on only UNIX platform, no problem for
other platoform because candidate window does not grabs input
focus on other platform. We'd better to keep existing codes
for other platform. I've added #if defined(XP_UNIX) for safety.

Also, for safer way and to avoid performance regression for
normal key event, I've put the codes for getting 
mCurrentEventContent into

if (NS_IS_IME_EVENT(aEvent)) {
}

Could you review again please?

Comment 39

[Roland Mainz]

Comment on attachment 97720 [details] [diff] [review]
use defined(XP_UNIX)

I am not sure whether |XP_UNIX| is correct since we have Unix-like OSes without
X11 (MacOSX, Photon port, BeOS etc. etc) ...

... what about using |MOZ_X11| instead ?

Comment 40

[Masaki Katakai]

Thanks, Roland.

Yes, I believe that MOZ_X11 is correct for this case.

Comment 41

[Roland Mainz]

Attachment: Updated patch which is only used on X11-based ports (GTK+, Qt, Xlib)

Comment 42

[Akkana Peck]

Comment on attachment 97757 [details] [diff] [review]
Updated patch which is only used on X11-based ports (GTK+, Qt, Xlib)

I'm not at all familiar with this code, but it looks very safe -- it won't
effect non-X11 platforms and it won't change anything on any event that isn't
NS_IS_IME_EVENT, so I think taking it is safe.	I have three stylistic issues:

>+            nsCOMPtr<nsIFocusController> focusController;
>+            nsCOMPtr<nsIScriptGlobalObject> ourGlobal;
>+            mDocument->GetScriptGlobalObject(getter_AddRefs(ourGlobal));
>+            nsCOMPtr<nsPIDOMWindow> ourWindow = do_QueryInterface(ourGlobal);
>+            if (ourWindow) {
>+              ourWindow->GetRootFocusController(getter_AddRefs(focusController));

You don't really need the focus controller until you're inside the ourWindow
clause, so it would be better to declare it there.

>+                  if(focusedElement) {

You should probably put a space after the if for consistency with the rest of
the function.

The third issue is that I find it hard to read with the X11 clause in one place
and then later the !X11 clause.  Everything in between them is comments, and
they all apply to the non-X11 clause, so can you change that first #endif to an
#else and remove the #ifndef?  I'll attach a revision of the patch in case that
wasn't clear, with my three changes, but I'll mark my r=akkana on this one.

Comment 43

[Akkana Peck]

Attachment: My minor suggested revisions to the patch

Comment 44

[Masaki Katakai]

Attachment: new patch with Akkana 's comment

Thank you very much, Akkana.

I've corrected the position of focusController definition and
space issue. Your changes for #else are included.

Could you r= please?

Comment 45

[Masaki Katakai]

Oh, I'm sorry the patch has r= already.

Now asking sr=... to reviewers.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97919 [details] [diff] [review]
new patch with Akkana 's comment

+		   // if not, search pre-focused element

"search for"

+#endif /* !MOZ_X11 */

That '!' should not be there (since your ifdef is now just MOZ_X11).

+		     focusedElement->QueryInterface(NS_GET_IID(nsIContent),
+						(void
**)&mCurrentEventContent);

CallQueryInterface(focusedElement, &mCurrentEventContent);

Also, there's lots of commented-out code around here that just makes the code
hard to understand, imo.  Could we just remove that code please? (cced saari,
who commented this out ages ago).

Looks good other than those nits.

Comment 47

[Masaki Katakai]

Attachment: update patch

Thank you very much for review and comments, Boris.

I've corrected the patch, except removing codes in comment line.

Saari, could you give suggestion asap?
We're very close to code freeze of OEM branch.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 98409 [details] [diff] [review]
update patch

sr=bzbarsky.  Just land this if saari does not get back to you by the time you
need to land it... If possible, do that on branch only so we can clean the code
up a bit on the trunk.

Comment 49

[Masaki Katakai]

Sure,

Thank you very much.

Trunk tree is frozen for Mozilla 1.2alpha, so I'll not
ask this to Trunk but ask to OEMbrach. Adding branchOEM
keyword to request checkin to OEMbranch.

Comment 50

[Margaret Chan]

Hi Masaki:

Please check this in to the OEM branch.  Jim has already given you the
permission  for checkin to the OEM branch under the assumption that you will
check this into the trunk when the code is cleaned up and the trunk is reopen.

Thanks, Margaret

Comment 51

[Margaret Chan]

Hi Boris:  Is saari around?  We have not heard from him for quite a while. 
Shall we check in to the trunk (once it is open) and file another bug (assigned
to saari) for code clean up.  We just want to make sure that we are in sync with
the trunk to avoid any discrepancies with our future release.  Thanks, Margaret

Comment 52

[Boris Zbarsky [:bzbarsky]]

If saari is not answering, I say we rip out those comments (and make that clear
in the checkin comment).  CVS history will have them if really needed.

Comment 53

[Akkana Peck]

> If saari is not answering, I say we rip out those comments (and make that clear
> in the checkin comment).  CVS history will have them if really needed.

I concur with Boris, in case you want a second opinion/reviewer for that part.

Comment 54

[Masaki Katakai]

Tree is now open for 1.2Beta.

I'll check in the same patch to Trunk and OEMbranch.
For Trunk, after the check-in, I'll remove the
commented out codes with proper CVS comments by
myself for clean up if I do not get answer from
Sarri or ask Sarri to do that.

Comment 55

[Masaki Katakai]

checked in the patch to Trunk.

Will track code cleanup as bug 167866.

Thanks everyone for your help!

Comment 56

[Masaki Katakai]

checked into OEM branch.

Thanks again.

Comment 57

[Yuying Long]

Seems it still not fix for me on 09-12 trunk and 09-13 branch builds / linux
RH7.2-JA.

So, this fix only for OEM? I wonder why?

Comment 58

[Margaret Chan]

It was checked in to the trunk.  Not branch
though.  I am assuming that you are talking about the 1.2 alpha branch?

Comment 59

[Yuying Long]

No, I double checked this on 09-13 both trunk and branch - I don't see there is
any difference:
If I invoke Japanese IME, and start type something to bring up the candidate
window, and press space key to switch the candidate chracters, only first two
characters can be displayed in non-commit character position, all others can be
highlighted but can not showed up in non-commit character.  When I hit return
key to commit, then I will get the highlighted character(s) be commit not the
one in non-commit position.

Is this bug supposedly fix this problem?

Comment 60

[Masaki Katakai]

Hi Yuying, thank you for testing,

The first problem - only first candidate can be displayed on preedit area
   this is correct behavior of kinput2. Please try the same operation on
   normal gtk application such as gedit. You can see the same behavior on it.

The 2nd problem - candidate string can not be displayed at commit
   this is a bug of kinput2 I understand. Please see bug 63456.
   I have marked the bug as WONT FIX.
   kinput2 does not call needed method at commit. Type something
   after your commit, the preedit string can be changed to proper
   characters you selected at candidate window.

Does your testing team have ATOK X input server? I'll try to
verify the fix on Trun with ATOK X and let you know the result.

Comment 61

[Masaki Katakai]

verified the fix on OEM branch and Trunk on both
Linux and Solaris.