Keyboard selection in Hebrew select element does not work. Chromium bug http://crbug.com/8052
Created attachment 30549 [details] patch
Comment on attachment 30549 [details] patch The patch looks sane to me, but I think we can make a test for this, no? We certainly can fake key events from DumpRenderTree.
Comment on attachment 30549 [details] patch r- for lack of test per Eric's comment. Please resubmit with a regression test.
(In reply to comment #2) > (From update of attachment 30549 [details] [review]) > The patch looks sane to me, but I think we can make a test for this, no? We > certainly can fake key events from DumpRenderTree. > I am not sure how to auto-test this, and seems that there is no test for PopupMenuChromium.cpp. Let me look into it further.
The selection is exposed via select.value no? You can input key events directly to the element using eventSender. Something like this: <select id="mySelect>... <script> mySelect.focus() eventSender.key... assert(mySelect.value == "some string I wanted it to search for"); </scirpt>
(In reply to comment #5) > The selection is exposed via select.value no? You can input key events > directly to the element using eventSender. > > Something like this: > > <select id="mySelect>... > > <script> > mySelect.focus() > eventSender.key... > assert(mySelect.value == "some string I wanted it to search for"); > </scirpt> > Tony has a patch in which he explained why PopupMenuChromium.cpp is not testable. His patch is: https://bugs.webkit.org/attachment.cgi?id=30676 The comments are: + Not testable since it involves sending a keyboard event to + the popup, which is not possible (eventSender sends the key + events through webview, we want to go through the webwidget). But I think my patch should be: - m_lastCharTime = now; + #if PLATFORM(WIN_OS) + if (event.type() == PlatformKeyboardEvent::Char) + m_lastCharTime = now; + #else + if (event.type() == PlatformKeyboardEvent::KeyDown) + m_lastCharTime = now; + #endif I will test in Mac and upload the patch again.
Created attachment 30687 [details] patch (version 2)
Comment on attachment 30687 [details] patch (version 2) I think this should be separated out into a static function. Then the code becomes: if (isCharacterForPrefixString(event)) m_lastCharTime = now; (Or some other equally suitable or better name) And the long comment can be inside the static function, as well as the #if usage. This also needs a test case. Even if it's just a manual test in WebCore/manual-tests/ I would expect there is a way to test this using eventSender though, so you could make an automated test.
> This also needs a test case. Even if it's just a manual test in > WebCore/manual-tests/ > > I would expect there is a way to test this using eventSender though, so you > could make an automated test. I do not believe there is a way to fully test this with an automated test. You might be able to fake a click on the select control, but then you need to fake mouse move and click events to select something. I spent a great deal of time trying this last week to fix this bug: https://bugs.webkit.org/show_bug.cgi?id=25904 In the end, I created a manual test that demonstrates the fix.
Created attachment 30845 [details] patch w/ manual test The patched function PopListBox::typeAheadFind() is only called through WebWidgetImpl::HandleInputEvent(), never through WebviewImpl::HandleInputEvent(). If eventSender.keyDown() send events to webview, not webwidget, that means we can not use it to auto-test the patch. Or maybe I misunderstood anything?
Created attachment 30970 [details] patch w/ manual test
Comment on attachment 30970 [details] patch w/ manual test Style: +static bool isCharacterTypeEvent(const PlatformKeyboardEvent& event) { I'm confused why PopupListBox is in this file at all. Generally WebKit has one-class-per-file. Why doesn't WebKitWin need this same code? Or does it already have it?
(In reply to comment #12) > (From update of attachment 30970 [details] [review]) > Style: > +static bool isCharacterTypeEvent(const PlatformKeyboardEvent& event) { > > > I'm confused why PopupListBox is in this file at all. Generally WebKit has > one-class-per-file. I do not know why it is there originally. Maybe someone more familiar with the file history knows it better. > > Why doesn't WebKitWin need this same code? Or does it already have it? > WebKitWin also has a similar function: SelectElement::typeAheadFind(), but it is called only when the event type is Char type, that is why it does not need to check inside the method. The caller is: SelectElement::defaultEventHandler(), in which, it checks whether isPrintableChar(keyboardEvent->charCode()) is true before calling typeAheadFind(). KeyboardEvent::charCode() returns character code for keypress, 0 for keydown and keyup. PopupListBox::typeAheadFind() is called by PopupListBox::handleKeyEvent(), which handles all the platform keyboard events (keyup, keydown, char, rawkeydown) from WebWidgetImpl::HandleInputEvent(), and differentiate Char type event is needed inside typeAheadFind().
Comment on attachment 30970 [details] patch w/ manual test Looks fine.
Landed in http://trac.webkit.org/changeset/44948 Sending WebCore/ChangeLog Adding WebCore/manual-tests/keyboard_select_non_english.html Sending WebCore/platform/chromium/PopupMenuChromium.cpp