Bug 25899 – [Chromium] PC Chromium: keyboard selection in Hebrew select element doesn't work

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Xiaomei Ji]

Keyboard selection in Hebrew select element does not work.
Chromium bug
http://crbug.com/8052

[Xiaomei Ji]

Created attachment 30549 [details]
patch

[Eric Seidel (no email)]

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.

[Maciej Stachowiak]

Comment on attachment 30549 [details]
patch

r- for lack of test per Eric's comment. Please resubmit with a regression test.

[Xiaomei Ji]

(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. 

[Eric Seidel (no email)]

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>

[Xiaomei Ji]

(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.

[Xiaomei Ji]

Created attachment 30687 [details]
patch (version 2)

[Eric Seidel (no email)]

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.

[Paul Godavari]

> 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.

[Xiaomei Ji]

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?

[Xiaomei Ji]

Created attachment 30970 [details]
patch w/ manual test

[Eric Seidel (no email)]

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?

[Xiaomei Ji]

(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().

[Eric Seidel (no email)]

Comment on attachment 30970 [details]
patch w/ manual test

Looks fine.

[Jungshik Shin]

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