Bug 118445 – [Qt] QtWebKit should allow sending domain specific keycode to HTML applications

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Arunprasad Rajkumar]

Currently QtWebKit allows sending only key values present in(WindowsKeyBoardCodes.h), mostly Keyboard keys. 

But QtWebKit's usage is beyond the PC world & used in may areas like TV/STB/..etc. So it should allow sending domain specific keyvalues like(Color Keys[Red, Green, Yellow, Blue], Subtitle, Program Guide,..) to HTML application.

Soon I will post a patch for this bug.

[Arunprasad Rajkumar]

Created attachment 206194 [details]
Patch

[Allan Sandfeld Jensen]

What is the use-case here? 

Since this is the historic windows keycode value set here, it must be mapped to correct values, just parsing through Qt keycodes is kind of random and unlikely to be useful unless you control the website on the other end.

If you have control over the website why not change the website to use the other properties provided by the DOM key events instead of the windows keycode value?

[Arunprasad Rajkumar]

(In reply to comment #2)
> What is the use-case here? 
To send domain specific key values like(ex, TV Remote Keys like Color Keys[Red, Green, Yellow, Blue], Subtitle, Program Guide,..) to HTML application.

> 
> Since this is the historic windows keycode value set here, it must be mapped to correct values, just parsing through Qt keycodes is kind of random and unlikely to be useful unless you control the website on the other end.

Yes, it is all controlled Web Applications. 
> 
> If you have control over the website why not change the website to use the other properties provided by the DOM key events instead of the windows keycode value?

What other properties in DOM key event? How to dispatch those keys from native to Web App? using script injection?

According to this patch, sender(native world) & receiver(HTML Web App) knows the key, but WebKit doesn't aware of this and just passes the given value to Web App.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (In reply to comment #2)
> > What is the use-case here? 
> To send domain specific key values like(ex, TV Remote Keys like Color Keys[Red, Green, Yellow, Blue], Subtitle, Program Guide,..) to HTML application.
> 
> > 
> > Since this is the historic windows keycode value set here, it must be mapped to correct values, just parsing through Qt keycodes is kind of random and unlikely to be useful unless you control the website on the other end.
> 
> Yes, it is all controlled Web Applications. 
> > 
> > If you have control over the website why not change the website to use the other properties provided by the DOM key events instead of the windows keycode value?
> 
> What other properties in DOM key event? How to dispatch those keys from native to Web App? using script injection?
> 
> According to this patch, sender(native world) & receiver(HTML Web App) knows the key, but WebKit doesn't aware of this and just passes the given value to Web App.

I was thinking of the key value, see http://www.w3.org/TR/DOM-Level-3-Events/#key-values-list

Though I wonder if we support it fully, but I would prefer to add support for that, than a Qt specific passthrough.

[Arunprasad Rajkumar]

(In reply to comment #4)
> I was thinking of the key value, see http://www.w3.org/TR/DOM-Level-3-Events/#key-values-list

So you mean application should rely on the "key" attribute(which is a string representation of a key) of KeyBoardEvent object ? 
> 
> Though I wonder if we support it fully, but I would prefer to add support for that, than a Qt specific passthrough.

But how to transfer the string value from Qt to WebKit? By adding new property to QKeyEvent?

For private applications we could do change it to use "key" attribute rather than "keyCode", but some interactive tv apps(HbbTV/CEHTML apps) refers "keyCode" property to handle the event. I don't think it is possible to change the HbbTV/CEHTML spec to use "key" :( Sorry incase I misinterpreted your comments.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (In reply to comment #4)
> > I was thinking of the key value, see http://www.w3.org/TR/DOM-Level-3-Events/#key-values-list
> 
> So you mean application should rely on the "key" attribute(which is a string representation of a key) of KeyBoardEvent object ? 
>
Yes. It currently also falls back to encoding the Qt keycode as U+<keycode>.

> 
> But how to transfer the string value from Qt to WebKit? By adding new property to QKeyEvent?
> 
Well, we almost already have that in the key property.

> For private applications we could do change it to use "key" attribute rather than "keyCode", but some interactive tv apps(HbbTV/CEHTML apps) refers "keyCode" property to handle the event. I don't think it is possible to change the HbbTV/CEHTML spec to use "key" :( Sorry incase I misinterpreted your comments.

Right, but in those cases we should expand our translation table of keycodes so it matches what those applications expect.

[Arunprasad Rajkumar]

(In reply to comment #6)
> > But how to transfer the string value from Qt to WebKit? By adding new property to QKeyEvent?
> > 
> Well, we almost already have that in the key property.
> 
> > For private applications we could do change it to use "key" attribute rather than "keyCode", but some interactive tv apps(HbbTV/CEHTML apps) refers "keyCode" property to handle the event. I don't think it is possible to change the HbbTV/CEHTML spec to use "key" :( Sorry incase I misinterpreted your comments.
> 
> Right, but in those cases we should expand our translation table of keycodes so it matches what those applications expect.

By adding new keys to Qt::Key enum? So it needs change in Qt right?

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (In reply to comment #6)
> > > But how to transfer the string value from Qt to WebKit? By adding new property to QKeyEvent?
> > > 
> > Well, we almost already have that in the key property.
> > 
> > > For private applications we could do change it to use "key" attribute rather than "keyCode", but some interactive tv apps(HbbTV/CEHTML apps) refers "keyCode" property to handle the event. I don't think it is possible to change the HbbTV/CEHTML spec to use "key" :( Sorry incase I misinterpreted your comments.
> > 
> > Right, but in those cases we should expand our translation table of keycodes so it matches what those applications expect.
> 
> By adding new keys to Qt::Key enum? So it needs change in Qt right?

The color codes and channel-up/down are already added to Qt 5.2 dev branch, but we would need to add them anyway to get keycodes through.

[Arunprasad Rajkumar]

(In reply to comment #8)
> The color codes and channel-up/down are already added to Qt 5.2 dev branch, but we would need to add them anyway to get keycodes through.

Good news :) but lot of other keys there which are specific to TV/STB.(like subtitle,Guide,Teletext,Info,Back,Input Source Selection, & lot). I agree that it would satisfy a bit. But what would be its key value? Based on which standard ?(There are lot of standard are there like CEHTML/Open IPTV,HbbTV,Connected TV,..).
Anyhow the one who uses QtWebKit for those domain should touch the inner side of WebCore to assign their domain specific key value.

[Allan Sandfeld Jensen]

(In reply to comment #9)
> (In reply to comment #8)
> > The color codes and channel-up/down are already added to Qt 5.2 dev branch, but we would need to add them anyway to get keycodes through.
> 
> Good news :) but lot of other keys there which are specific to TV/STB.(like subtitle,Guide,Teletext,Info,Back,Input Source Selection, & lot). I agree that it would satisfy a bit. But what would be its key value? Based on which standard ?(There are lot of standard are there like CEHTML/Open IPTV,HbbTV,Connected TV,..).
> Anyhow the one who uses QtWebKit for those domain should touch the inner side of WebCore to assign their domain specific key value.

It will use Qt defined keycode values. Once a key event has been created by Qt the original values are no longer used. This is also why we need the values defined in Qt in the first place, otherwise there is no keycode value for WebKit either.

Qt do have most of the keys. I am looking into getting Guide, Info and Exit added as well.

[Arunprasad Rajkumar]

(In reply to comment #10)
> > Anyhow the one who uses QtWebKit for those domain should touch the inner side of WebCore to assign their domain specific key value.
> 
> It will use Qt defined keycode values. Once a key event has been created by Qt the original values are no longer used. This is also why we need the values defined in Qt in the first place, otherwise there is no keycode value for WebKit either.

> 
> Qt do have most of the keys. I am looking into getting Guide, Info and Exit added as well.

I understood that Qt::Key enum is going to have more STB/TV specific keys, but what is the DOM keyCode value for those keys? Is it based on CE-HTML(CEA 2014) or any other standard?

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (In reply to comment #10)
> > > Anyhow the one who uses QtWebKit for those domain should touch the inner side of WebCore to assign their domain specific key value.
> > 
> > It will use Qt defined keycode values. Once a key event has been created by Qt the original values are no longer used. This is also why we need the values defined in Qt in the first place, otherwise there is no keycode value for WebKit either.
> 
> > 
> > Qt do have most of the keys. I am looking into getting Guide, Info and Exit added as well.
> 
> I understood that Qt::Key enum is going to have more STB/TV specific keys, but what is the DOM keyCode value for those keys? Is it based on CE-HTML(CEA 2014) or any other standard?

The keycode values in the DOM are the windows keycodes, if there is no mapping in windows there is no keycode for that key. This is why you should use the DOM standard 'key' property.

[Arunprasad Rajkumar]

(In reply to comment #12)
> The keycode values in the DOM are the windows keycodes, if there is no mapping in windows there is no keycode for that key. This is why you should use the DOM standard 'key' property.

It is clear now. But still I'm worrying about Interactive TV specs(HbbTV, CEHTML). They suggest to use "keyCode" & "charCode" properties of KeyboardEvent, so some change inside WebCore is needed to support it for those specifications. 

But I still feel passing the keycode directly(the main of the attached patch) is a good option, so the one who uses QtWebKit no need to touch source of WebKit to achieve their intention.

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (In reply to comment #12)
> > The keycode values in the DOM are the windows keycodes, if there is no mapping in windows there is no keycode for that key. This is why you should use the DOM standard 'key' property.
> 
> It is clear now. But still I'm worrying about Interactive TV specs(HbbTV, CEHTML). They suggest to use "keyCode" & "charCode" properties of KeyboardEvent, so some change inside WebCore is needed to support it for those specifications. 
> 
> But I still feel passing the keycode directly(the main of the attached patch) is a good option, so the one who uses QtWebKit no need to touch source of WebKit to achieve their intention.

I can see what you are trying to do, but the patch depends on a non-Qt value being stored in a QKeyEvent. There are many ways this could go wrong.

[Arunprasad Rajkumar]

(In reply to comment #14)
> I can see what you are trying to do, but the patch depends on a non-Qt value being stored in a QKeyEvent. There are many ways this could go wrong.

QKeyEvent takes keycode value as "int", not Qt::Key. Hope user will not be confused if we pass no Qt::Key values. 

So the advanced user could use this way of sending keycodes to Web Apps without hacking more inside WebCore, what do you thing?

[Arunprasad Rajkumar]

(Sorry for the typo)

(In reply to comment #15)
> (In reply to comment #14)
> > I can see what you are trying to do, but the patch depends on a non-Qt value being stored in a QKeyEvent. There are many ways this could go wrong.
> 
> QKeyEvent takes keycode value as "int", not Qt::Key. Hope user will not be confused if we pass no Qt::Key values. 
> 
> So the advanced user could use this way of sending keycodes to Web Apps without hacking more inside WebCore, what do you think?

[Allan Sandfeld Jensen]

The problem with using other values in QKeyEvent is that if they happen to match something already recognized the key will trigger the wrong actions.

I took a look at the HbbTv codes, and there is a big problem here. While many of the new keys use values larger than 255 and therefore do not conflict with traditional keycodes, there are several that does. 

For instance fast-forward and rewind are mapped differently than on GoogleTV. This means you can either have the browser working with hbbtv sites or have it working with GoogleTV sites, but not both.

If you really need this, I would suggest adding another property to QtKeyEvent and pass your domain specific keycodes in that, and if present use that in the PlatformKeyEventQt to set the keycode, but we can not support a poorly thought through standard in trunk that conflicts with existing websites.

[Arunprasad Rajkumar]

(In reply to comment #17)
> The problem with using other values in QKeyEvent is that if they happen to match something already recognized the key will trigger the wrong actions.
> 
> I took a look at the HbbTv codes, and there is a big problem here. While many of the new keys use values larger than 255 and therefore do not conflict with traditional keycodes, there are several that does. 

Thanks for looking HbbTV specific keys :)
> 
> For instance fast-forward and rewind are mapped differently than on GoogleTV. This means you can either have the browser working with hbbtv sites or have it working with GoogleTV sites, but not both.

By default existing behaviour is not changed at all. It is upto the QtWebKit library user to decide whether to support GoogleTV/HbbTV. If the library user wants both then they can detect the application type using <meta> tags or mime types(HbbTV apps has application/ce-html or application/x-ce-html or application/vnd.hbbtv.xhtml+xml,..).

> 
> If you really need this, I would suggest adding another property to QtKeyEvent and pass your domain specific keycodes in that, and if present use that in the PlatformKeyEventQt to set the keycode, but we can not support a poorly thought through standard in trunk that conflicts with existing websites.

Hope it won't conflict, because it is decided by QtWebKit library user.

[Allan Sandfeld Jensen]

If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.

[Arunprasad Rajkumar]

(In reply to comment #19)
> If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.

I'm getting your point :) But I'm sure adding a new property to a Qt's QKeyEvent is a painful job, because WebKit has to wait until the stable Qt release(with the change).

What about adding a global settings(via QWebSettings)? like adding new value to QWebSettings::WebAttribute..?

eg)
QWebSettings::setAttribute(QWebSettings::WebAttribute::TVEnabled, true);

If above attribute is set, then from PlatformKeyboardEventQt.cpp given keycode will be just passed to WebCore.

What do you think?

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (In reply to comment #19)
> > If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.
> 
> I'm getting your point :) But I'm sure adding a new property to a Qt's QKeyEvent is a painful job, because WebKit has to wait until the stable Qt release(with the change).
> 
> What about adding a global settings(via QWebSettings)? like adding new value to QWebSettings::WebAttribute..?
> 
> eg)
> QWebSettings::setAttribute(QWebSettings::WebAttribute::TVEnabled, true);
> 
> If above attribute is set, then from PlatformKeyboardEventQt.cpp given keycode will be just passed to WebCore.
> 
> What do you think?

I was considering that, but I still don't like the passthrough of values that could be misinterpreted. We could implement the correct conversion from Qt keycodes to CE keycodes, but it turns out it is not entirely consistant between the standards. VK_MENU and VK_BACK are mappped differently on CE-HTML than on EBIF and OCAP.

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (In reply to comment #19)
> > If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.
> 
> I'm getting your point :) But I'm sure adding a new property to a Qt's QKeyEvent is a painful job, because WebKit has to wait until the stable Qt release(with the change).
> 
Right, but perhaps you could use the existing nativeVirtualKeyCode field then.

[Arunprasad Rajkumar]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.
> > 
> > I'm getting your point :) But I'm sure adding a new property to a Qt's QKeyEvent is a painful job, because WebKit has to wait until the stable Qt release(with the change).
> > 
> Right, but perhaps you could use the existing nativeVirtualKeyCode field then.

Yeah, I too thought the same, but will it cause any confusion to the one who is seeing the code?

I know that 2 properties like nativeVirtualKeyCode, nativeScanCode are there. But what value it holds in X11/Windows ..?

I'm thinking like incase of Windows for left arrow key, 
nativeVirtualKeyCode = VK_LEFT
keycode = Qt::Key_Left
nativeScanCode = <some keyboard driver scan code>

Isn't it?

[Allan Sandfeld Jensen]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.
> > > 
> > > I'm getting your point :) But I'm sure adding a new property to a Qt's QKeyEvent is a painful job, because WebKit has to wait until the stable Qt release(with the change).
> > > 
> > Right, but perhaps you could use the existing nativeVirtualKeyCode field then.
> 
> Yeah, I too thought the same, but will it cause any confusion to the one who is seeing the code?
> 
> I know that 2 properties like nativeVirtualKeyCode, nativeScanCode are there. But what value it holds in X11/Windows ..?
> 
> I'm thinking like incase of Windows for left arrow key, 
> nativeVirtualKeyCode = VK_LEFT
> keycode = Qt::Key_Left
> nativeScanCode = <some keyboard driver scan code>
> 
> Isn't it?

True, but since you are generating your own QKeyEvents you are essentially a new "native" source of key-events. These values are usually only there for applications that need to detect keys not fully supported or detected by Qt, similar to your case.

[Arunprasad Rajkumar]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (In reply to comment #20)
> > > > (In reply to comment #19)
> > > > > If you can make due with the keycodes outside of what Qt uses >255 <0x01000000. We could pass them through as there is no overlap. Though I think the best solution would be a setting to let us know were are in CE mode.
> > > > 
> > > > I'm getting your point :) But I'm sure adding a new property to a Qt's QKeyEvent is a painful job, because WebKit has to wait until the stable Qt release(with the change).
> > > > 
> > > Right, but perhaps you could use the existing nativeVirtualKeyCode field then.
> > 
> > Yeah, I too thought the same, but will it cause any confusion to the one who is seeing the code?
> > 
> > I know that 2 properties like nativeVirtualKeyCode, nativeScanCode are there. But what value it holds in X11/Windows ..?
> > 
> > I'm thinking like incase of Windows for left arrow key, 
> > nativeVirtualKeyCode = VK_LEFT
> > keycode = Qt::Key_Left
> > nativeScanCode = <some keyboard driver scan code>
> > 
> > Isn't it?
> 
> True, but since you are generating your own QKeyEvents you are essentially a new "native" source of key-events.

OK. Understood :)

>These values are usually only there for applications that need to detect keys not fully supported or detected by Qt, similar to your case.

So it the platformkeyboadreventqt.cpp, instead of returning keycode directly nativeVirtualKeyCode should be passed from default: case?

-default:
-  return keycode; 

+default:
+    return event->nativeVirtualKey();

Am I correct?

[Allan Sandfeld Jensen]

(In reply to comment #25)
> > True, but since you are generating your own QKeyEvents you are essentially a new "native" source of key-events.
> 
> OK. Understood :)
> 
> >These values are usually only there for applications that need to detect keys not fully supported or detected by Qt, similar to your case.
> 
> So it the platformkeyboadreventqt.cpp, instead of returning keycode directly nativeVirtualKeyCode should be passed from default: case?
> 
> -default:
> -  return keycode; 
> 
> +default:
> +    return event->nativeVirtualKey();
> 
> Am I correct?

Exactly, but you will then need a setting like passThroughŃativeVirtualKeys, or something similar. We don't want the normally (except on Windows where it would be okay)

[Arunprasad Rajkumar]

(In reply to comment #26)
> (In reply to comment #25)
> > > True, but since you are generating your own QKeyEvents you are essentially a new "native" source of key-events.
> > 
> > OK. Understood :)
> > 
> > >These values are usually only there for applications that need to detect keys not fully supported or detected by Qt, similar to your case.
> > 
> > So it the platformkeyboadreventqt.cpp, instead of returning keycode directly nativeVirtualKeyCode should be passed from default: case?
> > 
> > -default:
> > -  return keycode; 
> > 
> > +default:
> > +    return event->nativeVirtualKey();
> > 
> > Am I correct?
> 
> Exactly, but you will then need a setting like passThroughŃativeVirtualKeys, or something similar. We don't want the normally (except on Windows where it would be okay)

Thanks for your suggestion. That looks good for me. Will update the patch as per your suggestion.

Is it good to call QWebSettings from platformkeyboardeventqt.cpp?

[Arunprasad Rajkumar]

Created attachment 207542 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 207542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207542&action=review

Looks good but the keyIdentifiers will be wrong.

> Source/WebCore/platform/qt/PlatformKeyboardEventQt.cpp:640
> +        m_keyIdentifier = keyIdentifierForQtKeyCode(m_windowsVirtualKeyCode);

This is wrong. The translation in keyIdentifierForQtKeyCode depends on the Qt keys being set correctly.

[Jocelyn Turcotte]

Comment on attachment 207542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207542&action=review

> Source/WebKit/qt/Api/qwebsettings.cpp:515
> +        when Qt::Key to DOM key converstion fails. It might be useful for sending domain specific key values

"when Qt::Key to DOM key converstion fails" is false isn't it? It seems like this will be triggered only if the key is 0.

> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:1244
> -    return frame->eventHandler()->keyEvent(ev);
> +    return frame->eventHandler()->keyEvent(PlatformKeyboardEvent(ev, settings->testAttribute(QWebSettings::UseNativeVirtualKeyAsDOMKey)));

I think this is quite messy to be honest.
Do we need the setting? What would you think about simply passing the nativeVirtualKey as well to windowsKeyCodeForKeyEvent, which would fall back to the nativeVirtualKey if the keycode is 0?

I don't see any use case where a 0 key to a QKeyEvent is expected, so this could allow to shape legacy web key events.

[Arunprasad Rajkumar]

Comment on attachment 207542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207542&action=review

Currently I'm changing the code to use nativeVirtualKey always if 'UseNativeVirtualKeyAsDOMKey' settings is enabled.

>> Source/WebKit/qt/Api/qwebsettings.cpp:515
>> +        when Qt::Key to DOM key converstion fails. It might be useful for sending domain specific key values
> 
> "when Qt::Key to DOM key converstion fails" is false isn't it? It seems like this will be triggered only if the key is 0.

True, it will be triggered only if windowsKeyCodeForKeyEvent() return 0.

>> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:1244
>> +    return frame->eventHandler()->keyEvent(PlatformKeyboardEvent(ev, settings->testAttribute(QWebSettings::UseNativeVirtualKeyAsDOMKey)));
> 
> I think this is quite messy to be honest.
> Do we need the setting? What would you think about simply passing the nativeVirtualKey as well to windowsKeyCodeForKeyEvent, which would fall back to the nativeVirtualKey if the keycode is 0?
> 
> I don't see any use case where a 0 key to a QKeyEvent is expected, so this could allow to shape legacy web key events.

nativeVirtualKey() property may have some other value in window systems like X11(xcb_keycode_t), DirectFB,.. If I say simply, Allen(& me too) don't want to break the existing behavior unless QtWebKit embedder decided to override by enabling a QWebSettings :)

[Arunprasad Rajkumar]

Comment on attachment 207542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207542&action=review

>>> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:1244
>>> +    return frame->eventHandler()->keyEvent(PlatformKeyboardEvent(ev, settings->testAttribute(QWebSettings::UseNativeVirtualKeyAsDOMKey)));
>> 
>> I think this is quite messy to be honest.
>> Do we need the setting? What would you think about simply passing the nativeVirtualKey as well to windowsKeyCodeForKeyEvent, which would fall back to the nativeVirtualKey if the keycode is 0?
>> 
>> I don't see any use case where a 0 key to a QKeyEvent is expected, so this could allow to shape legacy web key events.
> 
> nativeVirtualKey() property may have some other value in window systems like X11(xcb_keycode_t), DirectFB,.. If I say simply, Allen(& me too) don't want to break the existing behavior unless QtWebKit embedder decided to override by enabling a QWebSettings :)

One more point, if QtWebKit embedder wants to override some of Qt::Key to Windows conversion then it is not possible with the approach you mentioned(my prev. patch also has the issue), for example for Qt::Key_Pause it maps to VK_PAUSE(0x13), but in CE-HTML it should be VK_PAUSE(0x51).

So I'm going to modify the patch like below,
    
    m_nativeVirtualKeyCode = event->nativeVirtualKey();
    if (allowNativeVirtualKeyAsDOMKey && m_nativeVirtualKeyCode)
        m_windowsVirtualKeyCode  = m_nativeVirtualKeyCode;
    else
        m_windowsVirtualKeyCode = windowsKeyCodeForKeyEvent(event->key(), m_isKeypad);

[Arunprasad Rajkumar]

Created attachment 207669 [details]
Patch

[Arunprasad Rajkumar]

Comment on attachment 207669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207669&action=review

> Source/WebCore/platform/qt/PlatformKeyboardEventQt.cpp:665
> +        if (!m_allowNativeVirtualKeyAsDOMKey && m_text.isEmpty() && m_windowsVirtualKeyCode && isVirtualKeyCodeRepresentingCharacter(m_windowsVirtualKeyCode))

In-case of using nativeVirtualKey as DOM key don't resolve Char Event type in disambiguateKeyDown. isVirtualKeyCodeRepresentingCharacter() function checks for visual representation, it may unnecessarily generate keypress event for invisible keys.(sending visible keys is not a problem for us)

[Jocelyn Turcotte]

(In reply to comment #31)
> > I don't see any use case where a 0 key to a QKeyEvent is expected, so this could allow to shape legacy web key events.
> 
> nativeVirtualKey() property may have some other value in window systems like X11(xcb_keycode_t), DirectFB,.. If I say simply, Allen(& me too) don't want to break the existing behavior unless QtWebKit embedder decided to override by enabling a QWebSettings :)

The switch of behavior would be done when QKeyEvent::key() returns 0, which isn't a normal use case at all in Qt, and should never happen. What I mean is that using a QWebSettings + key() = 0 should have the same scope of effect as just key() = 0.

I just don't like this boolean passing of the QWebSetting down to PlatformKeyboardEvent on each event.

(btw, his name is Allan, not Allen)

[Arunprasad Rajkumar]

(In reply to comment #35)
> (In reply to comment #31)
> > > I don't see any use case where a 0 key to a QKeyEvent is expected, so this could allow to shape legacy web key events.
> > 
> > nativeVirtualKey() property may have some other value in window systems like X11(xcb_keycode_t), DirectFB,.. If I say simply, Allen(& me too) don't want to break the existing behavior unless QtWebKit embedder decided to override by enabling a QWebSettings :)
> 
> The switch of behavior would be done when QKeyEvent::key() returns 0, which isn't a normal use case at all in Qt, and should never happen. What I mean is that using a QWebSettings + key() = 0 should have the same scope of effect as just key() = 0.
> 
> I just don't like this boolean passing of the QWebSetting down to PlatformKeyboardEvent on each event.
> 
> (btw, his name is Allan, not Allen)

(In reply to comment #35)
> (In reply to comment #31)
> > > I don't see any use case where a 0 key to a QKeyEvent is expected, so this could allow to shape legacy web key events.
> > 
> > nativeVirtualKey() property may have some other value in window systems like X11(xcb_keycode_t), DirectFB,.. If I say simply, Allen(& me too) don't want to break the existing behavior unless QtWebKit embedder decided to override by enabling a QWebSettings :)
> 
> The switch of behavior would be done when QKeyEvent::key() returns 0, which isn't a normal use case at all in Qt, and should never happen. What I mean is that using a QWebSettings + key() = 0 should have the same scope of effect as just key() = 0.

True. But in the new patch, I modified to use nativeVirtualKey always incase settings is enabled. Because there is a use case to override the keyCode value for Qt's standard key also.

> 
> I just don't like this boolean passing of the QWebSetting down to PlatformKeyboardEvent on each event.
> 
I agree with you. It causes QList lookup during each KeyboardEvent. But settings is became necessary for the new implementation.


> (btw, his name is Allan, not Allen)
sorry :(

[Allan Sandfeld Jensen]

Comment on attachment 207669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207669&action=review

> Source/WebKit/qt/Api/qwebsettings.cpp:516
> +    \value UseNativeVirtualKeyAsDOMKey Specifies whether to use QKeyEvent::nativeVirtualKey as a DOM keyCode 
> +        value. It might be useful for sending domain specific key values to Web Application. It is disabled 
> +        by default.

Specifies whether to use QKeyEvent::nativeVirtualKey as the DOM KeyboardEvent keyCode value instead of Windows virtual key codes. This can be used to send domain specific key values to a Web Application. This is disabled by default.

[Arunprasad Rajkumar]

Created attachment 208130 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 208401 [details]
Patch

[Simon Hausmann]

Comment on attachment 208401 [details]
Patch

LGTM. Jocelyn, Allan, what do you think?

[Allan Sandfeld Jensen]

Comment on attachment 208401 [details]
Patch

LGTMT

[WebKit Commit Bot]

Comment on attachment 208401 [details]
Patch

Clearing flags on attachment: 208401

Committed r153884: <http://trac.webkit.org/changeset/153884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.