Bug 34936 – [Chromium] Typing into Flash with wmode = opaque|transparent and non-latin language active outputs as if US keyboard layout active

• Component: Plug-ins
• Status: RESOLVED FIXED

[Hironori Bono]

(Copied from <http://crbug.com/13233>.)

Chrome Version       : 2.0.172.28 (Official Build )
URLs (if applicable) :
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
     Safari 4: Fail
  Firefox 3.x: Fail
         IE 7: Fail
         IE 8: Fail

What steps will reproduce the problem?
1. Take a flash movie with a text box
2. Render onto page with wmode parameter set to 'opaque' or 'transparent'
3. Set keyboard language to Greek
4. Type in box

What is the expected result?

Greek characters corresponding to keyboard layout

What happens instead?

Latin characters corresponding to US keyboard layout.

Please provide any additional information below. Attach a screenshot if
possible.

I've made a demonstration page at http://baseonmars.co.uk/bugs/wmode along with short 
movie clips showing the results.

It's possible to write text in another application (or even the address bar) then copy paste it into 
the textbox.

The other browsers have this bug, but IE7, IE8 and Firefox Minefield demonstrate the bug by 
showing 2 characters for each keypress, generally one is a punctuation mark and the other 
shows as a rectangle.

The firefox team have a long history with this bug, blame has been passed back and forth 
between the flash and mozilla team for at least 4 years 
(https://bugzilla.mozilla.org/show_bug.cgi?id=272847, 
https://bugzilla.mozilla.org/show_bug.cgi?id=347185). Hopefully some of their experience can 
help the chromium team.

[Hironori Bono]

Created attachment 49269 [details]
A WebKit-side fix

Greetings,

The chromium-side fix for this issue is <http://codereview.chromium.org/267072/show>. Even though the new test included in this change fails without this chromium-side change, is it possible to review this WebKit-side change?

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

Comment on attachment 49269 [details]
A WebKit-side fix

> Index: WebKit/chromium/src/WebViewImpl.cpp
...
> @@ -507,8 +507,13 @@ bool WebViewImpl::keyEvent(const WebKeyb
>      PlatformKeyboardEventBuilder evt(event);
>  
>      if (handler->keyEvent(evt)) {
> -        if (WebInputEvent::RawKeyDown == event.type)
> -            m_suppressNextKeypressEvent = true;
> +        if (WebInputEvent::RawKeyDown == event.type) {
> +            // Suppress the next keypress event unless the focused node is a plug-in node.
> +            // (Flash needs these keypress events to handle non-US keyboards.)
> +            Node* node = frame->document()->focusedNode();
> +            if (!node || (!node->hasTagName(HTMLNames::embedTag) && !node->hasTagName(HTMLNames::objectTag)))

An <object> tag can be more than just a plug-in.  It would
probably be better check the renderer to see if it is a plug-in
via node->renderer()->isEmbeddedObject().

Otherwise, I think this change is good.  Nice work on the test!

[Hironori Bono]

Created attachment 50002 [details]
A webKit-side fix 2

Thank you for your review and comments.

(In reply to comment #2)

> An <object> tag can be more than just a plug-in.  It would
> probably be better check the renderer to see if it is a plug-in
> via node->renderer()->isEmbeddedObject().

Thank you for noticing this function. I wasn't aware that WebKit has such a convenient function.
Since I have updated this change to use this function, is it possible to review the updated one?

Regards,

Hironori Bono

[Dimitri Glazkov (Google)]

Comment on attachment 50002 [details]
A webKit-side fix 2

ok.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50002 [details]
A webKit-side fix 2

> Index: WebKit/chromium/src/WebViewImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/WebViewImpl.cpp	(revision 55511)
> +++ WebKit/chromium/src/WebViewImpl.cpp	(working copy)
> @@ -507,8 +507,13 @@ bool WebViewImpl::keyEvent(const WebKeyb
>      PlatformKeyboardEventBuilder evt(event);
>  
>      if (handler->keyEvent(evt)) {
> -        if (WebInputEvent::RawKeyDown == event.type)
> -            m_suppressNextKeypressEvent = true;
> +        if (WebInputEvent::RawKeyDown == event.type) {
> +            // Suppress the next keypress event unless the focused node is a plug-in node.
> +            // (Flash needs these keypress events to handle non-US keyboards.)
> +            Node* node = frame->document()->focusedNode();
> +            if (!node || !node->renderer()->isEmbeddedObject())

You might need to null check node->renderer() before de-referencing it.
Most code does so.

[Hironori Bono]

Created attachment 50089 [details]
A webkit-side fix 3

(In reply to comment #5)
> You might need to null check node->renderer() before de-referencing it.
> Most code does so.

Thank you for your comment and sorry for my bonehead mistake. I have added the null check here.
Is it possible to review the updated one?

Regards,

Hironori Bono

[WebKit Commit Bot]

Comment on attachment 50002 [details]
A webKit-side fix 2

Cleared Dimitri Glazkov's review+ from obsolete attachment 50002 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 50089 [details]
A webkit-side fix 3

OK.

[WebKit Commit Bot]

Comment on attachment 50089 [details]
A webkit-side fix 3

Clearing flags on attachment: 50089

Committed r55620: <http://trac.webkit.org/changeset/55620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.