Bug 34936

Summary: [Chromium] Typing into Flash with wmode = opaque|transparent and non-latin language active outputs as if US keyboard layout active
Product: WebKit Reporter: Hironori Bono <hbono>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
URL: http://baseonmars.co.uk/bugs/wmode
Attachments:
Description Flags
A WebKit-side fix
fishd: review-
A webKit-side fix 2
none
A webkit-side fix 3 none

Hironori Bono
Reported 2010-02-15 02:28:06 PST
(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.
Attachments
A WebKit-side fix (8.13 KB, patch)
2010-02-23 01:23 PST, Hironori Bono 		fishd: review-
DetailsFormatted DiffDiff
A webKit-side fix 2 (8.26 KB, patch)
2010-03-04 03:00 PST, Hironori Bono 		no flags
DetailsFormatted DiffDiff
A webkit-side fix 3 (8.25 KB, patch)
2010-03-04 23:22 PST, Hironori Bono 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Hironori Bono
Comment 1 2010-02-23 01:23:57 PST
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 2 2010-03-03 23:18:12 PST
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
Comment 3 2010-03-04 03:00:20 PST
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 4 2010-03-04 07:54:52 PST
Comment on attachment 50002 [details] A webKit-side fix 2 ok.
Darin Fisher (:fishd, Google)
Comment 5 2010-03-04 10:06:11 PST
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
Comment 6 2010-03-04 23:22:30 PST
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 7 2010-03-05 14:03:12 PST
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 8 2010-03-05 14:33:32 PST
Comment on attachment 50089 [details] A webkit-side fix 3 OK.
WebKit Commit Bot
Comment 9 2010-03-06 05:21:02 PST
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
Comment 10 2010-03-06 05:21:06 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.