Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed WPF osr ime support #2103

Closed
wants to merge 6 commits into from
Closed

Conversation

starts2000
Copy link

fixed @ #1262

@merceyz
Copy link
Member

merceyz commented Aug 25, 2017

Please write a more detailed description so it's easy to see what is done in this PR.

You seem to have changed things unrelated to IME support, please revert that.
Spaces not tabs (1 tab -> 4 spaces)
Use the same formatting style as the rest of the project

@ashu3011
Copy link

Tried this fix on CefSharp.WPF.Example. There are 2 issue I encountered:
Issue1:
To recreate:

  1. Run CefSharp.WPF.Example.
  2. Keyboard Language should be set to any IME supporting language,
  3. Type something in browser IME works fine,
  4. Change tab
  5. Type again

Result: IME doen't get displayed and you are typing in English

Issue 2

Null reference exception in chromiumwebbrowser.cs if the browser window is not displayed yet. The presentationsource returns null which causes the exception when you try to get handle while setting up _imewin

@ashu3011
Copy link

ashu3011 commented Sep 19, 2017

For Issue 1: I don't get null reference exception any more but one of my browser is not properly initialized and thus I can not type in it.

Issue 2 still persists. In CefSharp.wpf example just open a new tab which will default to Google.com. Type a search query, IME will not appear.

@ashu3011
Copy link

Works perfectly with Cef.WPF.Example . I am still having trouble with my xwt application but I think it has to do something with my implementation of it. The issue is that IME does not show up on initial load of the browser but works perfectly after I get away from it and come back to it. It persists to work rest of the time. I will look deeper and see if I can figure it out. Thanks.

@ashu3011
Copy link

ashu3011 commented Nov 30, 2017

IME for Japanese language is not at the correct position, it appears at the bottom right of the screen. Also for other languages, IME do not correct position with Scale (Display settings on windows). Thus with few settings, depending on the monitors resolution, they appear on top of the input where IME is used and thus make it tough for users.

Copy link
Member

@perlun perlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As others have pointed out, this PR needs a bit of cleanup before it's really reviewable. @starts2000, could you look into these details? Like, reverting the .csproj change, resolving the conflicts, using spaces instead of tabs etc.

Once that is done, I think we should give this a consideration. What does the rest of you think?

"$(DevEnvDir)..\..\VC\bin\amd64\editbin" /TSAWARE "$(TargetPath)"
)</PostBuildEvent>
<PostBuildEvent>
</PostBuildEvent>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should be reverted, since it disables strong named signing.

@@ -1,4 +1,4 @@
// Copyright 2010-2017 The CefSharp Authors. All rights reserved.
// Copyright ?2010-2017 The CefSharp Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other similar changes should be reverted - the copyright sign should remain in the previous encoding. (UTF-8 I presume)

@amaitland
Copy link
Member

This PR conflicts with the changes proposed in #2056

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Copy link
Author

@starts2000 starts2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@amaitland amaitland mentioned this pull request Jan 17, 2018
26 tasks
@amaitland
Copy link
Member

#2056 has been merged. With those changes you should be able to implement your own IWpfKeyboardHandler and create a solution that requires no changes to the core CefSharp code. (You'll likely need to create a class that inherits from ChromiumWebBrowser).

Looking at the code it can be greatly simplified, there's lots of code there that has no relevance to CefSharp

Copy link
Member

@amaitland amaitland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to add IME support then I think someone needs to investiage what options the WPF framework provides, I'm assuming TextBox and other input fields support IME, surely there's a cleaner approach than hacking some low level C++

@@ -336,12 +336,30 @@ void CefBrowserHostWrapper::ImeSetComposition(String^ text, cli::array<Compositi
_browserHost->ImeSetComposition(StringUtils::ToNative(text), underlinesVector, CefRange(), range);
}

void CefBrowserHostWrapper::ImeSetComposition(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can likely be removed and you just get a reference to the underlying CefBrowserHost rather than adding code.

It's my understanding that some of the params are only used on Mac and can be left out of any Windows implementation.

@@ -471,7 +496,7 @@ private void NoInliningConstructor()

PresentationSource.AddSourceChangedHandler(this, PresentationSourceChangedHandler);

RenderOptions.SetBitmapScalingMode(this, BitmapScalingMode.HighQuality);
RenderOptions.SetBitmapScalingMode(this, BitmapScalingMode.NearestNeighbor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this, you can change it in your own code if you wish to adjust the quality.


if (CleanupElement != null)
{
Point point = this.TransformToAncestor(CleanupElement).Transform(new Point(0, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CleanupElement here really isn't a reliably method to access the parent Window, which is what I'm assuming your doing. I'm sure you can find a better way of doing this.


if (source != null)
{
_imeWin = new OsrImeWin(source.Handle, this.browser);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you create OsrImeWin when focus is gained and destroyed it when focus Is lost, is there a reason it's created here?

@@ -1859,37 +1954,15 @@ protected virtual IntPtr SourceHook(IntPtr hWnd, int message, IntPtr wParam, Int
return IntPtr.Zero;
}

switch ((WM)message)
if (!IsDisposed && _imeWin != null && IsKeyboardFocused && browser != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can now implement your own IWpfKeyboardHandler to capture and forward key events.


// Handles IME for the native parent window that hosts an off-screen browser.
// This object is only accessed on the CEF UI thread.
class OsrImeHandler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a specific reason this is a separate class I think this should be merged into OsrImeWin

// governed by a BSD-style license that can be found in the LICENSE file.

#ifndef CEF_TESTS_CEFCLIENT_BROWSER_OSR_IME_HANDLER_WIN_H_
#define CEF_TESTS_CEFCLIENT_BROWSER_OSR_IME_HANDLER_WIN_H_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.

@amaitland
Copy link
Member

Closing as I have no plans to merge this.

See #1262 (comment) for an alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants