Closed Bug 272847 Opened 20 years ago Closed 16 years ago

Text input via IME does not work in windowless Flash movie

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: jmott, Assigned: masayuki)

References

Details

(Keywords: fixed1.9.1, inputmethod, Whiteboard: Note that you need a new flash player for using IME which is not released)

Attachments

(2 files, 13 obsolete files)

HTML and SWF
(deleted), application/octet-stream
Details
Patch v2.3
(deleted), patch
emaijala+moz
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

NPP_HandleEvent does not forward WM_IME_* messages, making it impossible for 
Flash to accept IME input.

Reproducible: Always
Steps to Reproduce:
1.Contact me for a test movie or use attachment if I can make that work.
2.Turn on Japanese IME
3.Try to enter text

Actual Results:  
Nothing

Expected Results:  
Kanji

I am a member of the Flash player team, please contact me with any questions 
concerning this bug.
Attached file HTML and SWF (deleted) — 
Test media
Additionally, we could really stand to get WM_CHAR messages.  Lack of WM_CHAR 
is killing non-IME non English input (French special characters for example).
reporter: please file plugin bugs in core:plugins
Assignee: bugs → nobody
Component: OS Integration → Plug-ins
Product: Firefox → Core
QA Contact: firefox.os-integration → core.plugins
Version: unspecified → Trunk
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Confirming. Tested on:
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a1) Gecko/20050927
Firefox/1.6a1
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter or anyone else still seeing this issue with the latest Firefox 3 Nightly Build and the latest Flash Plugin 

Version (http://www.adobe.com/shockwave/download/alternates/) ?
If not please close this bug as WORKSFORME. Thank you.
Whiteboard: CLOSEME 07/23
I can confirm this behavior using Firefox 2.0.0.6 on Windows Vista as well as using the latest trunk build. I can't enter Katakana in the text box in the testcase in comment 1.

Jeff, is there something special about that Flash file? I know nothing about Flash in this regard...


Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007080605 Minefield/3.0a8pre
Whiteboard: CLOSEME 07/23
The thing about the test case is that is uses wmode = opaque or wmode = transparent.  When the control is running windowless, Firefox stops sending us WM_IME_* messages that we depend on.

Jeff:

Does flash process the WM_IME_* messages at windowless mode now? I wrote a draft patch for this bug, but that doesn't work fine. The patch is here:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3854&action=diff
Our windowless message proc forwards messages to the regular message proc, which handles WM_IME_*.  Is there some way I can get a build with your changes to test with?  I'm not set up to build Firefox.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3860&action=diff

This patch works, some bugs of the previous patch was fixed by Kimura-san. Thank you, Kimura-san!

Jeff, you're right, the WM_IME_* is processed. However, the IME composition string is broken when first time inputting. Do you have ideas for the bug?

And if you need the patched trunk build, I'll build it on our tryserver. Do you need the build?
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Could you give me some more detail on what you mean when you say "the IME composition string is broken when first time inputting"?
(In reply to comment #14)
> Could you give me some more detail on what you mean when you say "the IME
> composition string is broken when first time inputting"?

When I input any Japanese characters via IME to windowless flash, the composition string is not displayed correctly. It is displayed as Mid dot that is missing glyph, maybe. However, if the last character of the composition string is displayed as ASCII character, the next composition string is displayed correctly. Of course, when the composition string is broken, the committed string is also broken.
I think I'm going to need to get your build and debug the player running in it - could you tell me how to get it please?
I post the patch to tryserver. The test build will come here:
https://build.mozilla.org/tryserver-builds/?C=M;O=D

Check today's my mail address including directory that will come 1 or 2 hours later. Unfortunately, the patch cannot be built accidentally, the tryserver tinderbox (http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry) will be burned, then, I'll retry to build it tomorrow.
(In reply to comment #15)
> (In reply to comment #14)
> > Could you give me some more detail on what you mean when you say "the IME
> > composition string is broken when first time inputting"?
> 
> When I input any Japanese characters via IME to windowless flash, the
> composition string is not displayed correctly.

Oops, this is wrong. The composition string is displayed correctly. Because it is not displayed by Flash, it is displayed by IME. Only the committed string is broken.
I have tested your build with the player, and we both have some work to do.  For me, there is an issue where I'm trying to get the composition string from the OS but I'm failing because the hWnd I'm using is NULL.  It's just a result of code that never got run this particular way before, and I'll get that sorted out.

On your side, you appear to have tried to work around my problem by generating some WM_CHAR events out of the IME input and sending them into the player.  You should remove this code.  As you've seen, it has issues on a Japanese PC.  On an English PC it doesn't work at all, the WM_CHAR events are all for the '?' character.

To sum up - there is no way you can get this working without me doing some work in the player.  If you could modify your fix to eliminate the WM_CHAR events I can work on the player to handle the IME events that you are giving me.
Attached patch Patch v0.5 (obsolete) (deleted) — — Splinter Review 
o.k. this may be you wanted patch. I'll rebuild a test build with this patch.
Unfortunately, the tinderbox is burned now. I'll use tryserver tonight.

I think that the latest patch may have a problem. That is same as bug 357670 comment 133 (and later Shane's comments).

We should send all input events to plug-ins when focused content is plug-in, not via gecko inner events. Because we discard some native events, we change the meaning of native events, we generate some events without native events. However, that is not good for plug-ins. They should get the native events directly.

Therefore, the patch doesn't fire DOM key events when a plug-in has focus. Then, event listeners of the plug-in element (object or embed) cannot handle the events. But I think that only DOM key events should be re-fired after the focused plug-in processes the native key events.
Masayuki, the Windows-specific parts of your patch (attachment 338039 [details] [diff] [review])
will need to be ported to other platforms, right?

Do you intend this patch's descendants to be the solution to the
ActiveState problem on OS X (bug 357670 comment #133 and following)?
If so, let me know when you begin porting it to OS X, and we can (if
you'd like) work on that together.
(In reply to comment #24)
> Masayuki, the Windows-specific parts of your patch (attachment 338039 [details] [diff] [review])
> will need to be ported to other platforms, right?

Yes.

> Do you intend this patch's descendants to be the solution to the
> ActiveState problem on OS X (bug 357670 comment #133 and following)?
> If so, let me know when you begin porting it to OS X, and we can (if
> you'd like) work on that together.

No, the patch just helps the other platform implementation for windowless plugin support. I think we need to fix bug 435041, first. I have no idea for fix the issue on current our design (using Carbon events for plug-ins).

And note that the coming patch will create the ActiveState issue on Windows too. My patch doesn't fire the IME events for plug-ins.
Your new patch has slightly different behavior, but still not what I’m hoping
for.  For example – I typed “とんかつ” into the IME and hit enter.  

First, the player received a WM_IME_COMPOSITION event with GCS_RESULTSTR set,
which is good – once I get the player working right, this is all I will need to
display the result.

The player then received four WM_CHAR events for 0x3068, 0x3093, 0x304b, and
0x3064.  We really don’t want these events, there’s no way we can handle them
correctly.  So I’m hoping you can change your code so that after the IME is
committed, there will be no WM_CHAR events at all.  This will mean that with
the current player you will see no input in the text field, but that is correct
– fixing that has to be my job, inside the player.

In my internal player build, when I enter "とんかつ" in the IME, this is the result in the text field:  "とんかつ0hｱK0Kd0d".

I'm working on getting a player build with the necessary changes that I can
send you to test with.
Attached patch Patch v0.6 (obsolete) (deleted) — — Splinter Review 
This patch sends key events via DOM key events. However, the all native events are also sent to focused plug-in. This patch is draft, but it should work fine, I think. But Flash doesn't set the committed string to input field. I'm not sure whether the issue is mine.
Attachment #338039 - Attachment is obsolete: true
Oh, I missed this comment.

(In reply to comment #26)
> The player then received four WM_CHAR events for 0x3068, 0x3093, 0x304b, and
> 0x3064.  We really don’t want these events, there’s no way we can handle them
> correctly.  So I’m hoping you can change your code so that after the IME is
> committed, there will be no WM_CHAR events at all.  This will mean that with
> the current player you will see no input in the text field, but that is correct
> – fixing that has to be my job, inside the player.

Thank you, maybe the last patch (v0.6) will help you. I cannot see the committed string. Because, maybe, the WM_CHAR will be discarded.
(In reply to comment #26)
> In my internal player build, when I enter "とんかつ" in the IME, this is the result
> in the text field:  "とんかつ0hｱK0Kd0d".

Note that the current Gecko (Fx3 and later) is an Unicode application. Therefore,
the wParam value and the lParam value are Unicode value when the values mean character, so, they are not legacy multi-byte characters. Please be careful for it. The string should be "とんかつとんかつ". I worry that you have similar bug in other points.
Attached patch Patch v0.7 (obsolete) (deleted) — — Splinter Review 
This might go to review process.
Attachment #338140 - Attachment is obsolete: true
Attached patch Patch v0.8 (obsolete) (deleted) — — Splinter Review 
Jeff:

FYI: I switched to non-IME using keyboard layout (e.g., Arabic) on Vista-ja, however, I cannot input the Arabic characters. We don't send WM_INPUTLANGCHANGE, this should be a cause of it. However, this patched build also cannot input the Arabic characters. 

And maybe, there are some messages which are not sent to plug-ins but they are needed. If you know such messages, I'll include the messages to next patch.
Attachment #338280 - Attachment is obsolete: true
Oh, currently, we never send WM_CHAR to windowless plug-ins. It's very strange thing for me. It seems that the intl text input handling is very hard work for plug-in developers. I have no idea, why we do so.
It would be really great if you could forward WM_CHAR events to the windowless plug in.  That would solve a number of bugs reported against the Flash Player.  WM_IME_* and WM_CHAR are the only events we've had problems with.  When such a change goes live, we're going to need some way to detect which browsers have it, such as an NPVERS or something.  Right now the player fakes a WM_CHAR when it gets a WM_KEYDOWN, and we'll obviously have to stop doing that for browsers that give us a real WM_CHAR.
Thank you, we can send WM_CHAR easily. However, then, there is a problem which is that the committed string is sent by WM_CHAR. Therefore, you need to filter the WM_CHAR events, probably.
It shouldn't have to work that way, and doesn't in the normal (non windowless) plugin.  When an IME composition takes place, the player gets WM_IME_* events, an no WM_CHAR events at all.  When normal keyboard input (Latin or Arabic) takes place, we get WM_CHAR.  It is desirable for the windowless behavior to exactly match the normal behavior.
Kimura-san said that if you don't need the WM_CHARs for IME committed string, you should return PR_TRUE from NPP_HandleEvent if the event is called for WM_IME_*COMPOSITION. Then, WM_CHARs are not fired.
Thank you, I have tested that with patch V.05 and it seems to work, the WM_CHAR events are suppressed after I return true from the IME_COMPOSITION handler.  If you can add the WM_CHAR support back in to the next patch, we should be able to get Arabic working.
o.k. please wait the next patch and test build.
Attached patch Patch v0.9 (obsolete) (deleted) — — Splinter Review 

        Attachment #338289 -
        Attachment is obsolete: true

    
    

    
  
This is working perfectly with my internal Flash Player build.  I still have to make some changes to deal properly with the Unicode values on your WM_CHAR events, but the correct data is coming to the player.

    
    

    
  
Great! Can I test the internal build after you finish the all works?

    
    

    
  
Have you recieved your invitation to the Adobe pre release program yet?  Our internal process tells me the invitation was sent.  Once we have you under NDA I can get you a build to test with.

    
    

    
  
Oh, sorry, I missed your email. I'll reply to your email.

    
    

    
  
Fixing the WM_CHAR issue we have discussed here will fix this bug:  https://bugzilla.mozilla.org/show_bug.cgi?id=347185

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v0.9.1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
updating the patch for latest trunk.

        Attachment #338337 -
        Attachment is obsolete: true

    
    

    
  
Jeff:

I downloaded your internal build (10,0,12,36). However, it doesn't work fine for me.

1. ATOK (Japanese 3rd party's IME) cannot change its open state from Keyboad (Hankaku/Zenkaku key of JIS layout) without Alt key when *windowless* flash has focus. But MS-IME of Vista works fine. And also Alt+Hankaku/Zenkaku key works fine, it's strange.

2. The committed string is still broken. I can type Japanese characters which are displayed by IME. However, when I commit the composition string by Enter key, the committed text is broken on flash. I can reproduce this bug both with windowed mode and windowless mode.

3. I cannot input any characters from non-ASCII characters. E.g., Cyrillic characters from Russian keyboard layout. The characters are also inputted as broken characters.

    
    

    
  
(In reply to comment #50)
> 3. I cannot input any characters from non-ASCII characters.

I meant: I cannot input any non-ASCII characters from any keyboard layout.

And I tested on Vista-ja x64.

    
    

    
  
The problem with non ASCII characters is expected - sorry I failed to communicate that.  We should have a player (internally) that can deal with Unicode input within a few months.

On the IME input issue, we're going to have to investigate that further, as it is working for me here with the plugin I sent you and the Minefiled build you posted for me:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080912103343 Minefield/3.1b1pre (.NET CLR 3.5.30729)

I'm testing with the attached files in opaque.zip.  

It is particularly odd that you see issues with windowed mode.  What happens when you drop my build into the release version of Firefox?

    
    

    
  
(In reply to comment #52)
> I'm testing with the attached files in opaque.zip.  

I'm using http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2974
But I can reproduce with opaque.zip too.

> It is particularly odd that you see issues with windowed mode.  What happens
> when you drop my build into the release version of Firefox?

I get same result.

I input "とんかつ", flash player displayed "縺ｨ繧薙°縺､". I saved this text as Shift-JIS and I loaded the text file as *UTF-8*, then, I can see "とんかつ", correctly.

    
    

    
  
The reason you see Shift-JIS data coming through in the attachment.cgi test is because the SWF used is version 5, and there is a previously unknown bug with IME input and SWF version 5 in Flash Player 10 - thank you for finding it, I'll fix it ASAP.

I am unable to reproduce the Shift-JIS data effect using opaque.swf or opaque.html, with either Firefox or Minefield, with either my plugin or the release plugin.

Testing opaque.html, with Minefield and my plugin, I get correct input, with any other combination I can't enter anything in the first place (no IME).

Testing opaque.swf directly I get correct input with all combinations.

    
    

    
  
(In reply to comment #54)
> Testing opaque.html, with Minefield and my plugin, I get correct input, with
> any other combination I can't enter anything in the first place (no IME).

Ah, sorry. This is my fault. The opaque.html's swf works fine for me. I'm sorry for the wrong report.

But the latest your build works fine the committed string issue, great and thank you!

However, I hope that the 1st issue should be fixed before landing the patch to our trunk build. The message log (only keyboard messages and IME messages):

* Windowless mode:
> P WM_KEY_UP nVirtKey: VK_OEM_ENLW cRepeat:1 ScanCode:29 fExtended:0 fAltDown:0 fRepeat:1 fUp:1
> P WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000005
> S WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000014
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000004
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_SETOPENSTATUS dwCommand:00000008 dwData:00000000
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000014
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000004
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_SETOPENSTATUS dwCommand:00000008 dwData:00000000
> R WM_IME_NOTIFY (lResult:00000000)

* Windowed mode:
> P WM_KEY_UP nVirtKey: VK_OEM_ENLW cRepeat:1 ScanCode:29 fExtended:0 fAltDown:0 fRepeat:1 fUp:1
> P WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000005
> S WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000014
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_PRIVATE dwCommand:0000000E dwData:00000004
> R WM_IME_NOTIFY (lResult:00000000)
> S WM_IME_NOTIFY dwCommand:IMN_SETOPENSTATUS dwCommand:00000008 dwData:00000000
> R WM_IME_NOTIFY (lResult:00000000)

Windowed mode works fine, but windowless mode is not so. It's strange, WM_IME_NOTIFY dwCommand:IMN_SETOPENSTATUS is sent twice only on windowless mode. Then, the open state is turned to the previous (before WM_KEY_UP) state...

    
    

    
  
When WM_IME_NOTIFY for IMN_SETOPENSTATUS is is sent, the DefaultWndProc is called on windowless mode, it's correct. mmmm....

Don't you call ImmSetOpenStatus at this time??

    
    

    
  
Jeff:

Would you check it?

    
    

    
  
I'll take a look - first I'll have to find the ATOK IME, I've never used it.

    
    

    
  
The trial version of ATOK is here:
http://www.justsystems.com/jp/frm/atok/try/08w/

But you need to read Japanese text :-(

    
    

    
  
Ok, I have ATOK installed, with some help from a Japanese reader here.  But we can't quite figure out how to investigate the issue.  You said:

ATOK (Japanese 3rd party's IME) cannot change its open state from Keyboad
(Hankaku/Zenkaku key of JIS layout) without Alt key when *windowless* flash has
focus. But MS-IME of Vista works fine. And also Alt+Hankaku/Zenkaku key works
fine, it's strange.

I'm going to need some help figuring out what that means.  Some kind of hot key is failing to produce some desired result from ATOK?  Do I need a Japanese keyboard?  What keys should I be pressing?  What result should I see?  I always operate the IME with the mouse.

Thanks.

    
    

    
  

      
      
      
      

        Attached image
          
        Screenshot of ATOK toolbar (obsolete) (deleted)
        — 
      

    


  
1st, you need to install Japanese keyboard layout from Control Panel. And run the test build of Fx with your internal build of Flash Player. Set focus to an editor of Flash Player. Then, change the current keyboard layout to Japanese (displayed as 'JP') from Language bar of Windows and change the current IME to ATOK.

Then, you can use ATOK on Fx. (See the language bar screenshot of the attachment)

If you are using 101 keyboard, you have '`' key on left of '1' key. It is Hankanku/Zenkaku key of JIS keyboard layout. You should be able to switch the IME open state with this key. However, you may fail to switch the ATOK state when windowless Flash Player has focus. You can confirm the ATOK state on ATOK toolbar. (See the screenshot for the looks of the ATOK toolbar)

If you cannot switch the IME state by '`' on windowed Flash Player too, please tell me it. I'll prepare the English Windows environment on VM and I'll check the steps for changing the IME state with ATOK on English environment.

    
    

    
  
Jeff: What's the status?

    
    

    
  
I can't find any program on my computer for which the '`' key changes the Atok state.  Notepad, IE, Word for Windows - I have to use Alt + '`' for all of them.  The '`' key just injects a '`' character into the document / IME candidate window.  I'll attach a screen shot.

    
    

    
  

      
      
      
      

        Attached image
          
        Can't get '`' key to change Atok state (obsolete) (deleted)
        — 
      

    


  

    
    

    
  
I should add that I also tried rebooting my system into Japanese mode via the "Language for non-Unicode programs" on the Regional and Languages Options settings panel, but it made no difference.

    
    

    
  
Oh, it's strange...

Irie-san, do you have ideas for the problem Jeff cannot change the IME open state only with Hankaku-Zenkaku key?

    
    

    
  
O.K. I installed WinXP SP3 to VM. And I understand what is needed. You need to update the configuration of ATOK.

1. You click the menu icon on ATOK toolbar. (Then, menu is popped up.)
2. Click "プロパティ(環境設定)(R)..." that is 6th item from bottom of the menu.
3. Chose "キー・ローマ字・色" tab that is 2nd tab from right of the tabs.
4. Click "キーカスタマイズ(K)..." button that is top-most button of the right side.
5. Click "機能を検索(S)..." button that is positioned top-right in the dialog.
6. Select "機能操作" in left listbox (3rd item from bottom).
7. Select "日本語入力ON/OFF" in right listbox and click "OK" button.
8. Click "変更(C)..." button that is positioned bottom-left of the dialog.
9. Select "F2" at the dropdown list and click "OK" button.
10. You may look a warning messege, then, click "Yes".
11. Close all dialogs by "OK" button.

Then, you can turn on/off ATOK by F2 key.

However, on windowless flash player, I cannot change the state by F2 key. Please check it.

If the above steps is too hard for you because that has some Japanese words, I'll post screenshots.

And note that you need to set Japanese for non-Unicode application language setting before you customize the ATOK configurations.

    
    

    
  
Nakano-san, sorry I have no idea. Is the problem clear?

Did you meant following things in comment #67, didn't you?
 * Key or character code generated by '`' (Backquote) key is different from Hankaku/Zenkaku key's one.
 * So, you wrote the instruction configuring ATOK to use other key (F2, here) for turn it on/off.

    
    

    
  
(In reply to comment #68)
> Did you meant following things in comment #67, didn't you?
>  * Key or character code generated by '`' (Backquote) key is different from
> Hankaku/Zenkaku key's one.
>  * So, you wrote the instruction configuring ATOK to use other key (F2, here)
> for turn it on/off.

ATOK doesn't treat to same key Hankaku/Zenkaku key and '`' key. So, I could  not set '`' key on US keyboard layout to the shortcut key of "日本語入力ON/OFF". Therefore I tried to use F2 key for the testing on WinXP-En environment instead.

    
    

    
  
Jeff: Do you need more help for the ATOK issue?

    
    

    
  
Jeff: what your status?

I want to request to review if my patch doesn't have problem for the ATOK issue.

    
    

    
  
I have found and fixed the problem with ATOK, I'll contact you off list with a player build you can test.  Your patch is fine.

Sorry for the delay, lots going on here as usual...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1.0 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Thank you, Jeff! Great, the your internal build works perfectly for me. This patch should be landed to Fx3.2. (The features are frozen in Fx3.1, unfortunately.)

Jst:

This patch throw following Win32 messages to windowless plugins directly.

WM_INPUTLANGCHANGEREQUEST
WM_INPUTLANGCHANGE
WM_IME_COMPOSITION
WM_IME_STARTCOMPOSITION
WM_IME_ENDCOMPOSITION
WM_DEADCHAR
WM_SYSDEADCHAR
WM_CONTEXTMENU
WM_CUT
WM_COPY
WM_PASTE
WM_CLEAR
WM_UNDO
WM_IME_CHAR
WM_IME_COMPOSITIONFULL
WM_IME_CONTROL
WM_IME_KEYDOWN
WM_IME_KEYUP
WM_IME_NOTIFY
WM_IME_REQUEST
WM_IME_SELECT
WM_IME_SETCONTEXT

And following messages are sent to windowless plugins via DOM events.

WM_CHAR
WM_SYSCHAR
WM_KEYUP (same as current behavior)
WM_SYSKEYUP (same as current behavior)
WM_KEYDOWN (same as current behavior)
WM_SYSKEYDOWN (same as current behavior)

By these events, windowless plugins can process the editing text events correctly (inputs characters from key events, IME and clipboard events).

This patch needs to change the message list of MDC (https://developer.mozilla.org/en/NPEvent), so, this patch has impact for plug-in developers.

        Attachment #342558 -
        Attachment is obsolete: true

        Attachment #343172 -
        Attachment is obsolete: true

        Attachment #344310 -
        Attachment is obsolete: true

        Attachment #348150 -
        Flags: review?(jst)

    
    

    
  
What's the impact on existing plugins if we land this patch? I don't see how it will cause problems.

    
    

    
  
(In reply to comment #74)
> What's the impact on existing plugins if we land this patch? I don't see how it
> will cause problems.

I'm not sure. This patch broadcasts very many messages additionally. E.g., WM_CHAR is not sent to windowless plug-in. So, windowless plugins used keydown events instead (this is a cause of bug 347185 and bug 444406). However, the plugins may have the processor for keypress events for windowed plugins. If the code is running by this changes, some plugins might work wrong....

    
    

    
  
I see. Could we ensure it works with Flash, send these messages to Flash only for 3.1, and then try sending to all plugins for 3.2?

    
    

    
  
o.k. it's possible. I'll write such patch ASAP.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2.0 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
O.K. This changes the behavior only when the plugin is flash player.

        Attachment #348150 -
        Attachment is obsolete: true

        Attachment #350122 -
        Flags: superreview?(roc)

        Attachment #350122 -
        Flags: review?(roc)

        Attachment #350122 -
        Flags: review?(jst)

        Attachment #350122 -
        Flags: review?(emaijala)

        Attachment #348150 -
        Flags: review?(jst)

    
    

    
  
Ah, note that I didn't test the patch with other plugins. I'm searching the windowless testcase for another plugin. If somebody know such page, please tell me.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2.0.1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
fix some nits, sorry for the spam.

        Attachment #350122 -
        Attachment is obsolete: true

        Attachment #350130 -
        Flags: superreview?(roc)

        Attachment #350130 -
        Flags: review?(roc)

        Attachment #350130 -
        Flags: review?(jst)

        Attachment #350130 -
        Flags: review?(emaijala)

        Attachment #350122 -
        Flags: superreview?(roc)

        Attachment #350122 -
        Flags: review?(roc)

        Attachment #350122 -
        Flags: review?(jst)

        Attachment #350122 -
        Flags: review?(emaijala)

    
    

    
  
I tested with QuickTime + pict image file. Then, the additional code works fine. We don't send new events and WM_CHAR to QT.

        Attachment #350130 -
        Flags: review?(joshmoz)

    
    

    
  
Comment on attachment 350130 [details] [diff] [review]
Patch v2.0.1

Josh, the new proud owner of the plugin module, should review the plugin changes as well.

    
    

    
  
+// Plug-in event. This is used when a plug-in has a focus, then,
+// we should send the native events directly (We should not change the event
+// counts, contents and etc.
+#define NS_PLUGIN_EVENT_START   3600
+#define NS_PLUGIN_EVENT         (NS_PLUGIN_EVENT_START)

You need to say that this event fires when a native event is received that needs to be passed to the plugin directly.

+   *         IME_STATUS_PLUGIN should be returned only when plug-in has focus,
+   *         this value has a special meaning. It is used as alternative of
+   *         IME_STATUS_ENABLED.

You should say what difference this actually makes (like you do in nsIWidget).

Why do we need this new IME status? Just to track if a plugin has focus? I wonder if it would be simpler to track that with a separate flag.

+#ifdef XP_WIN
+  PRInt8                      mUseNewPluginEvents;
+#endif

Don't call it mUseNewPluginEvents --- the meaning of 'new' will change over time. In fact, don't even have this field. It's fine to just check the name every time. But don't we already have some code that checks for Flash?

+  PRBool UseNewPluginEvents()

Don't call this "New" either. Perhaps SendNativeKeyEvents. And can we make it not #ifdef XP_WIN, just make it return PR_FALSE on other platforms?

+  if (!aNativeEvent.mEventDispatched)
+    aCallDefWndProc = !DispatchPluginEvent(aNativeEvent);

When can mEventDispatched be true here?

I wonder whether it's worth having nsWin32Event. We could just use MSG and pass around one or two extra boolean parameters.

I need to take another look at the nsWindow.cpp changes.

    
    

    
  
(In reply to comment #84)
> Why do we need this new IME status? Just to track if a plugin has focus? I
> wonder if it would be simpler to track that with a separate flag.

Yes. We need to know the current focused content is windowless plugin, or not so. I have already added password state. Therefore, plugin content having special IME state is reasonable way. And by supporting TSF, we may need to deactivate TSF code at this state. So, the new state is really different from ENABLED state.

> +  if (!aNativeEvent.mEventDispatched)
> +    aCallDefWndProc = !DispatchPluginEvent(aNativeEvent);
> 
> When can mEventDispatched be true here?

When KeyDown/KeyUp/KeyPress events are fired via current Gecko event in *most* cases, then, mEventDispatched is true. Otherwise, it's false (e.g., IME events, my patch doesn't send them when plugin has focus).
# The reason why I use Gecko key events only for native key events is some web apps may listen the DOM events like Komodo (See bug 357670 comment 133 and later).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2.1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #350130 -
        Attachment is obsolete: true

        Attachment #350744 -
        Flags: superreview?(roc)

        Attachment #350744 -
        Flags: review?(roc)

        Attachment #350744 -
        Flags: review?(emaijala)

        Attachment #350130 -
        Flags: superreview?(roc)

        Attachment #350130 -
        Flags: review?(roc)

        Attachment #350130 -
        Flags: review?(jst)

        Attachment #350130 -
        Flags: review?(joshmoz)

        Attachment #350130 -
        Flags: review?(emaijala)

        Attachment #350744 -
        Flags: review?(joshmoz)

        Attachment #350744 -
        Flags: superreview?(roc)

        Attachment #350744 -
        Flags: superreview+

        Attachment #350744 -
        Flags: review?(roc)

    
    

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

+       * 'Plug-in' state is a special case for the plug-in contents.

How about
  * This state is used when a plugin is focused.

 #ifdef XP_WIN
+  if (mInstanceOwner->SendNativeEvents() && NS_IS_PLUGIN_EVENT(anEvent)) {
+    *anEventStatus = mInstanceOwner->ProcessEvent(*anEvent);
+    return rv;
+  }
+

This doesn't need to be inside XP_WIN.

+    return PluginHasFocus() ? noDefault : PR_FALSE;

  return PluginHasFocus() && noDefault;

Otherwise looks OK. We definitely need Ere to look at it though.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2.2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #350744 -
        Attachment is obsolete: true

        Attachment #350918 -
        Flags: review?(joshmoz)

        Attachment #350918 -
        Flags: review?(emaijala)

        Attachment #350744 -
        Flags: review?(joshmoz)

        Attachment #350744 -
        Flags: review?(emaijala)

    
  

        Attachment #350918 -
        Flags: review?(joshmoz) → review+

    
  

        Attachment #350918 -
        Flags: review?(emaijala) → review-

    
    

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

Unfortunately the patch won't apply to current trunk. A couple of small issues:

+  if (!PluginHasFocus())
+    return;

This check in RemoveMessageAndDispatchPluginEvent is not needed as DispatchPluginEvent will check it anyway.

+  LRESULT                 OnCharRaw(UINT charCode, UINT aScanCode,
+                                    PRUint32 aFlags = nsnull,

Default for aFlags should be 0.

Some changes to comments:

+// Plug-in event. This is used when a plug-in has a focus and when the native

"has a focus" -> "has focus"

+      // First, we should end the current composing, if there is committed
+      // string.

-> // We should end composition if there is a committed string. 

+      // But there is still existing the composition string, we are still in a
+      // composing trunsaction.

-> // Continue composition if there is still a string being composed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2.3 (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #350918 -
        Attachment is obsolete: true

        Attachment #351680 -
        Flags: review?(emaijala)

    
    

    
  
Ere: Can you review this week end?

    
  

        Attachment #351680 -
        Flags: review?(emaijala) → review+

    
    

    
  
landed on trunk
http://hg.mozilla.org/mozilla-central/rev/2c2d64b8566c
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [c-n: baking for 1.9.1] → [c-n: baking for 1.9.1] Note that you need a new flash player for using IME which is not released

    
    

    
  
Comment on attachment 351680 [details] [diff] [review]
Patch v2.3

This patch may also fix bug 347815.

We didn't send enough keyboard/IME events to windowsless plug-ins. So, we need to send more events. However, current Gecko events are not enough for sending new events. For this issue, this patch creates a new Gecko event which just pass the native events to plug-ins. Therefore, only when focused content is a windowless plug-in, this patch's new codes are used. And the new events are only sent when the focused plug-in is flash player. Because the new events are not documented in Fx3b2.

        Attachment #351680 -
        Flags: approval1.9.1?

    
    

    
  
> This patch may also fix bug 347815.

bug 347185, sorry.

    
    

    
  
Steven:

(In reply to comment #24)
> Masayuki, the Windows-specific parts of your patch (attachment 338039 [details] [diff] [review])
> will need to be ported to other platforms, right?

I said 'yes' for this question. However, it seems that Mac doesn't have same issue on windowless plug-ins now... So, we don't need to port this to Mac now. However, we will need the new plugin event on Mac when NPAPI is changed to cocoa event model version (bug 435041).

    
    

    
  
Does this patch also fix: bug 444406 ?  

Thank you so much for fixing this long-standing bug!

    
    

    
  
(In reply to comment #96)
> Does this patch also fix: bug 444406 ?  

See bug 444406, I commented to it.

    
    

    
  
Comment on attachment 351680 [details] [diff] [review]
Patch v2.3

a191=beltzner

        Attachment #351680 -
        Flags: approval1.9.1? → approval1.9.1+

    
    

    
  
pushed to 191.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a939730d65c4
Keywords: fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1] Note that you need a new flash player for using IME which is not released → Note that you need a new flash player for using IME which is not released
Depends on: 497900

    
  
Depends on: 525762

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: