Bug 25899

Summary: [Chromium] PC Chromium: keyboard selection in Hebrew select element doesn't work
Product: WebKit Reporter: Xiaomei Ji <xji@chromium.org>
Component: WebKit Misc.Assignee: Xiaomei Ji <xji@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov@chromium.org, eric@webkit.org, paul@chromium.org, playmobil@google.com, progame+wk@gmail.com, xji@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
patch
mjs: review-
patch (version 2)
eric: review-
patch w/ manual test
none
patch w/ manual test eric: review+

Description From 2009-05-20 16:36:41 PST
Keyboard selection in Hebrew select element does not work.
Chromium bug
http://crbug.com/8052
------- Comment #1 From 2009-05-21 10:47:28 PST -------
Created an attachment (id=30549) [details]
patch
------- Comment #2 From 2009-05-22 00:42:51 PST -------
(From update of attachment 30549 [details])
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 #3 From 2009-05-22 00:55:10 PST -------
(From update of attachment 30549 [details])
r- for lack of test per Eric's comment. Please resubmit with a regression test.
------- Comment #4 From 2009-05-22 14:01:13 PST -------
(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. 
------- Comment #5 From 2009-05-22 18:54:19 PST -------
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>
------- Comment #6 From 2009-05-26 14:26:02 PST -------
(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.
------- Comment #7 From 2009-05-26 17:01:41 PST -------
Created an attachment (id=30687) [details]
patch (version 2)
------- Comment #8 From 2009-06-01 12:03:18 PST -------
(From update of attachment 30687 [details])
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.
------- Comment #9 From 2009-06-01 12:27:48 PST -------
> 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.
------- Comment #10 From 2009-06-01 17:03:15 PST -------
Created an attachment (id=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?
------- Comment #11 From 2009-06-04 16:36:44 PST -------
Created an attachment (id=30970) [details]
patch w/ manual test
------- Comment #12 From 2009-06-08 14:04:55 PST -------
(From update of attachment 30970 [details])
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?
------- Comment #13 From 2009-06-12 15:06:34 PST -------
(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 #14 From 2009-06-18 17:38:11 PST -------
(From update of attachment 30970 [details])
Looks fine.
------- Comment #15 From 2009-06-22 13:22:58 PST -------
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