Prevent accelerator keys to work when typing text #2056
Conversation
joaompneves
changed the title
|
Please provide an example that can be used for testing. |
|
Without looking into this in detail, from a maintenance point of view I'm not keen on adding a third piece of keyboard handling logic, having two is one more than I'd like as it is. |
|
I've got no problem with removing the |
|
Removed SourceHook and ended up with 2 key handling methods, one for key down/up to handle non-textual key presses and another for textual key presses. |
CefSharp.Wpf/ChromiumWebBrowser.cs
Outdated
| } | ||
| base.OnTextInput(e); |
There was a problem hiding this comment.
Should base.OnTextInput(e); be base.OnPreviewTextInput(e);?
CefSharp.Wpf/ChromiumWebBrowser.cs
Outdated
| /// <param name="e">The <see cref="TextCompositionEventArgs"/> instance containing the event data.</param> | ||
| protected override void OnPreviewTextInput(TextCompositionEventArgs e) | ||
| { | ||
| if (browser != null) |
There was a problem hiding this comment.
We need to check if (!e.Handled)` here, handle the case where an overriding implementation chooses to handle the input.
|
❌ Build CefSharp 63.0.0-CI2400 failed (commit 2833209eab by @joaompneves) |
|
✅ Build CefSharp 63.0.0-CI2401 completed (commit 8f4174d45f by @joaompneves) |
|
✅ Build CefSharp 63.0.0-CI2402 completed (commit 98fff118e2 by @joaompneves) |
|
This is now again ready to merge. |
|
Any plans to merge this PR? |
|
@amaitland Since you looked at this before, maybe it makes sense that you look at the latest changes as well. (If you don't have time, let me know as usual and we'll try to make it work in other ways.) |
|
Firstly it conflicts with the changes proposed in #2103, whilst I like the idea of removing the source hook and having a higher level solution it's unclear if a Secondly there's the question of testing, I'm assuming you've deployed this to a sizeable user base? Testing keyboard input is difficult, so many combinations to factor in. If this |
|
This change has been deployed and its running for 6 months now. Regarding the IME support, I would like to have support for it, although I think its more important to have the standard keyboard flow handling working correctly and then have IME support. |
How many users? Are they all using the same Whilst I'm the first to admit the current keyboard implementation has a few problems, it's also pretty stable, it''s rare that someone raises an issue, so that adds to my reluctance in merging. @perlun Thoughts? |
|
Around 200 users, as the current software version using this CefSharp version is still in beta stage. In a few weeks/months this user base will grow to thousands as the software reaches other release stages. Currently we have users testing in portuguese and english locales (at least). |
|
Any plans for this? |
|
✅ Build CefSharp 63.0.0-CI2420 completed (commit 31f21efb59 by @joaompneves) |
|
If you can make the behavior configurable then the risk of merging is trivial and I'm happy to include this. You can default to the new behavior, long as the old is there as a fall back. |
|
Added the ability to fallback to the old behavior using a property. Please review. Tested both approaches against: http://en.key-test.ru/ |
|
✅ Build CefSharp 63.0.0-CI2421 completed (commit a11cb4c6c5 by @joaompneves) |
|
Thanks 👍 Assuming this passes basic testing I'll include in the |
- Make public so users can implement their own handler (the OSR IME patch can be rewritten as a IWpfKeyboardHandler) - Formatting for consistency - Make methods virtual Follow up to #2056
* Call correct base method and handle text input in case event wasn't handled * Removed SourceHook method * Added previous keyboard handling logic and ability to use it through a property
- Make public so users can implement their own handler (the OSR IME patch can be rewritten as a IWpfKeyboardHandler) - Formatting for consistency - Make methods virtual Follow up to cefsharp#2056
* Call correct base method and handle text input in case event wasn't handled * Removed SourceHook method * Added previous keyboard handling logic and ability to use it through a property # Conflicts: # CefSharp.Wpf/ChromiumWebBrowser.cs
- Make public so users can implement their own handler (the OSR IME patch can be rewritten as a IWpfKeyboardHandler) - Formatting for consistency - Make methods virtual Follow up to #2056
When you have buttons with accelerator keys defined in a window with CefSharp and you press the key of the accelerator while the web view is focused, the key won't be sent to the web view.
Expected the web view to receive the key event when focused.
This pull request reestablishes that behavior.
Ex: If you have a button _Go (the accelerator key is G) and you type G inside the web view, the web view won't get the event.