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

Add UnhandledKeyPressed.md #3859

Open
wants to merge 12 commits into
base: api-UnhandledKeyPressed
Choose a base branch
from

Conversation

zhuhaichao518
Copy link
Contributor

This is an API spec review for the UnhandledKeyPressed API.

This is an API spec review for the UnhandledKeyPressed API.
@zhuhaichao518 zhuhaichao518 added the API Proposal Review WebView2 API Proposal for review. label Oct 9, 2023
@zhuhaichao518 zhuhaichao518 changed the base branch from main to api-UnhandledKeyPressed October 9, 2023 02:57
@@ -0,0 +1,234 @@

# Background
Consumers of the old [WebBrowser](https://learn.microsoft.com/en-us/dotnet/api/system.windows.controls.webbrowser?view=windowsdesktop-7.0) control that relied on the [OnKeyDown](https://learn.microsoft.com/previous-versions/aa752133(v=vs.85)) API that allowed them to receive and handle key events not handled by the browser, [requested](https://github.com/MicrosoftEdge/WebViewFeedback/issues/468) same ability in WebView2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please limit line lengths to 80 or 100 characters.

// Check if the key is one we want to handle.
if (key == 'Z')
{
MessageBox(
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure the C# and C++ code samples do the same thing. Here the C++ does more checking for other Ctrl+A-Z that C# doesn't and C++ is showing a message box while C# is writing to debug out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Updated the C# code to sync with C++ code.

{
if ((e.KeyEventKind == CoreWebView2KeyEventKind.KeyDown) && (e.Modifiers & CoreWebView2KeyPressedFlagKind.CoreWebView2KeyEventFlagControlDown) != 0)
{
if (e.VirtualKey == 'z')
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation says this is an upper case letter not lower case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. Fixed it.

@@ -0,0 +1,234 @@

# Background
Consumers of the old [WebBrowser](https://learn.microsoft.com/en-us/dotnet/api/system.windows.controls.webbrowser?view=windowsdesktop-7.0) control that relied on the [OnKeyDown](https://learn.microsoft.com/previous-versions/aa752133(v=vs.85)) API that allowed them to receive and handle key events not handled by the browser, [requested](https://github.com/MicrosoftEdge/WebViewFeedback/issues/468) same ability in WebView2.
Copy link
Contributor

Choose a reason for hiding this comment

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

The OnKeyDown links to IWebBrowser2::Navigate. Is it supposed to link to an OnKeyDown page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. Fixed it.

interface ICoreWebView2Controller : IUnknown {
/// Adds an event handler for the `UnhandledKeyPressed` event.
/// `UnhandledKeyPressed` runs when an key is not handled in
/// the DOM.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to explicitly say that this event will be raised after WebView2 has a chance to handle the key combination including the DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified the comment here to make it more clear here.

interface ICoreWebView2Controller : IUnknown {
/// Adds an event handler for the `UnhandledKeyPressed` event.
/// `UnhandledKeyPressed` runs when an key is not handled in
/// the DOM.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this would also not be raised if the Accelerator event marks this handled? Or if its an accelerator that WebView2 handles like Ctrl+P or Ctrl+X? And how does this relate to the DOM key event handler in script? If the DOM event marks the key press handled or not will result in the key press being 'unhandled' and raising this UnhandledKeyPressed event? It would be good to give examples.

{
MessageBox(
nullptr,
L"Key combination Ctrl + Z unhandled by browser is "
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that normally Ctrl+Z is an accelerator that the browser would handle by undoing text typed into a textbox. Will this only be raised if there is nothing to undo? Or does it get raised regardless? It would be good to explain in a comment in the code samples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be raised only when there is nothing to undo. Added a comment here.

/// The `LPARAM` value that accompanied the window message. For more
/// information, navigate to [WM_KEYDOWN](/windows/win32/inputdev/wm-keydown)
/// and [WM_KEYUP](/windows/win32/inputdev/wm-keyup).
/// For visual hosting, only scan code and extended key (16-24bit) are provided.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this the case? AcceleratorKeyPressed does not have this restriction for visual hosting.

Copy link
Contributor Author

@zhuhaichao518 zhuhaichao518 Oct 25, 2023

Choose a reason for hiding this comment

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

For visual hosting, the information of the native part (wparam, lparam, including these bits) was dropped when sent to WebView. This is not an issue for visual hosting because browser does not care about these bits (other bits were converted to a cross-platform part). If we need to get the exact lparam at UnhandledKeyPressed, we need to make some changes on how key events are sent to browser for visual hosting.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this because keyboard is not using the same mojo path as mouse and touch? Mouse and touch do send across the w_param and l_param. We should fix this so this API can return the correct l_param.

Copy link
Member

Choose a reason for hiding this comment

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

Another case for making this change, is if the LParam is missing values, then the COREWEBVIEW2_PHYSICAL_KEY_STATUS should also be missing those values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because we are reusing the mojom type which does not include native part of the input event. It was originally used by browser to send input events to render process, so it drops the native part (I think it is by design and we should not send the native info to renderer). We create the event on the host process so the native info was dropped when sent to browser process. Maybe we can send a copy of the native part to browser process to resolve this.

/// information, navigate to [WM_KEYDOWN](/windows/win32/inputdev/wm-keydown)
/// and [WM_KEYUP](/windows/win32/inputdev/wm-keyup).
/// For visual hosting, only scan code and extended key (16-24bit) are provided.
/// For other bits, check out the origin message from ::GetMessage().
Copy link
Member

Choose a reason for hiding this comment

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

This seems to imply that the app can call GetMessage to retrieve the information. Even if the app saved the MSG from the call to GetMessage, how would they match that to an UnhandledKeyPressed event since the event is async from the MSG being received?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Devs should expect which keys should fire UnhandledKeyPressed by themselves to know which message the UnhandledKeyPressed is related to. It is not very healthy. I did not expect it to be used often, the cases they need to do it is when they need the repeat count and previous key state according to the doc. Maybe we can make some changes to on how key events are sent to browser for visual hosting to avoid this.

Copy link
Member

Choose a reason for hiding this comment

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

The sample code uses Ctrl+Z as an example of a key combination that sometimes is handled and sometimes unhandled by the browser. If the user presses Ctrl+Z twice, the app might only get 1 unhandled Ctrl+Z or it might get 2. It would depend on the content of the text box. When the first unhandled Ctrl+Z comes in, the app wouldn't know which GetMessage to use. As I commented above, we should fix keyboard to use the same path as mouse and touch which does include the w_param and l_param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, what I mean was the dev should keep everything including how many characters users are putting in a textbox -- that includes lot of work on the js side and it is not healthy.
After weighing the use case and the cost of modification, I decided not to change the event and added a comment. Fixing this by making some modification to the event is fine to me.

[propget] HRESULT PhysicalKeyStatus(
[out, retval] COREWEBVIEW2_PHYSICAL_KEY_STATUS* physicalKeyStatus);

/// The `Handled` property will not influence the future WebView
Copy link
Member

Choose a reason for hiding this comment

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

Does this event need a Handled property? The sample code doesn't show using Handled. At this point, WebView2 has already decided not to do anything with the key. How does setting Handled in this event impact WebView2 behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it can be used when devs register more than one UnhandledKeyPressed handler and they do not want to handle they event on the second handler if the previous handler has marked the event as handled.


/// ALT is down, VK_MENU.
/// This bit is 0 when COREWEBVIEW2_KEY_EVENT_FLAG_ALTGR_DOWN bit is 1.
COREWEBVIEW2_KEY_EVENT_FLAG_ALT_DOWN = 1 << 3,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be MENU_DOWN to match VK_MENU?

COREWEBVIEW2_KEY_EVENT_FLAG_ALT_DOWN = 1 << 3,

/// Windows key is down. VK_LWIN | VK_RWIN
COREWEBVIEW2_KEY_EVENT_FLAG_COMMAND_DOWN = 1 << 4,
Copy link
Member

Choose a reason for hiding this comment

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

Command sounds like a Mac term. Should this be WIN_DOWN or WINDOWS_DOWN to match the VK values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is fine to keep these two enum names. We can use the same enum on Mac and CEFSharp is also using this name (https://cefsharp.github.io/api/113.1.x/html/T_CefSharp_CefEventFlags.htm). (They have not removed 'Mac OS-X command key.' comment though this bit supports windows key now.)


/// Flag bits representing the state of keyboard when a "UnhandledKeyPressed" Event happens.
[v1_enum]
typedef enum COREWEBVIEW2_KEY_PRESSED_FLAG_KIND {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the values of these match internal Chromium values. Other enum values around keyboard/mouse are based on publicly documented Windows values which are guaranteed not to change in the future. These internal Chromium values do not provider that guarantee, although I don't know how likely it is that they would change. @david-risney, is that an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be an issue if the chromium value changed and we changed the value in our public API. We cannot change the value in the public API.

For the names, do we already have enums in our WebView2 APIs similar to this? If so, we should be consistent with those names.

If this is a totally new enum and it needs to exist cross-platform then an OS agnostic naming would be OK. If its new and needs to be different on Win and Mac then it would be good to align the names with the existing OS specific names.

Copy link
Contributor Author

@zhuhaichao518 zhuhaichao518 Oct 25, 2023

Choose a reason for hiding this comment

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

The values are new and is sync with upstream and is under cross-platform concept and I expect Mac should have the same enum. Though it is not expected to change frequently, it is possible for them to change the value (it says it is ok to reorganize the value). For this consideration, maybe we remap the constants from them to WebView @bradp0721 @david-risney? We just simply always map COREWEBVIEW2_KEY_EVENT_FLAG_ALT_DOWN to 1 << 3 regardless of how EF_ALT_DOWN changes its value; We map COREWEBVIEW2_KEY_EVENT_FLAG_ALT_DOWN to EF_ALT_DOWN, not its value. It seems CEFSharp is doing like this and append new value at the end when there are new values old version.

Copy link
Member

Choose a reason for hiding this comment

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

The upstream comment about "It is OK to add values to the middle of this list and/or reorder it" means we have to do an explicit mapping to the COREWEBVIEW2 enum. We cannot just static_cast them and Chromium or Edge could change these values and break our WV2 API.

Instead of having this enum flag, we could also consider adding a GetKeyState function to the event args. The browser side can call GetKeyboardState to get the full keyboard state in a 256 byte array. This would allow the app to query the state of more keys than just the ones we decided to put in the enum. And would make the code the app writes for this event look more similar to the code for AcceleratorKeyPressed. The difference being, one uses ::GetKeyState and one uses event_args->GetKeyState.


`UnhandledKeyPressed` event can be triggered by all keys, which is different from [AcceleratorKeyPressed](https://learn.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2acceleratorkeypressedeventargs?view=webview2-dotnet-1.0.705.50) event.

`UnhandledKeyPressed` event is async, which means [GetKeyState](https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getkeystate) does not return the exact key state when the key event is fired. Use `UnhandledKeyPressedEventArgs.Modifiers` instead to verify whether Ctrl or Alt is down in this situation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please limit line lengths to no more than 80 or 100 characters.

/// Adds an event handler for the `UnhandledKeyPressed` event.
/// `UnhandledKeyPressed` will be raised ONLY IF ALL of the following conditions are met:
/// 1. The key event has not been marked as handled in the 'AcceleratorKeyPressed' event.
/// 2. The key event is not consumed in the DOM (e.g., through calling event.preventDefault()).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do 2 and 3 happen in that order? I thought standard browser shortcuts came before the COM event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. You can prevent browser accelerators from happening in step 2.

Copy link
Member

@bradp0721 bradp0721 Oct 25, 2023

Choose a reason for hiding this comment

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

Chromium has a PreHandleKeyboardEvent and a HandleKeyboardEvent (which happens after the renderer). So I believe there are some key combinations that the browser can handle before the DOM (Ctrl+T might behave like that), but then other key combinations are handled by the browser only if the DOM did not handle them (I think Ctrl+F works this way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I hadn't noticed before that the keyboard shortcuts for tab and window are processed before the DOM. They are in ReservedCommandOrKey

specs/UnhandledKeyPressed.md Outdated Show resolved Hide resolved
specs/UnhandledKeyPressed.md Outdated Show resolved Hide resolved

/// Flag bits representing the state of keyboard when a "UnhandledKeyPressed" Event happens.
[v1_enum]
typedef enum COREWEBVIEW2_KEY_PRESSED_FLAG_KIND {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be an issue if the chromium value changed and we changed the value in our public API. We cannot change the value in the public API.

For the names, do we already have enums in our WebView2 APIs similar to this? If so, we should be consistent with those names.

If this is a totally new enum and it needs to exist cross-platform then an OS agnostic naming would be OK. If its new and needs to be different on Win and Mac then it would be good to align the names with the existing OS specific names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants