Bug 47769 - [chromium] Keyboard+mouse selection in select popups only updated after unfocused
Summary: [chromium] Keyboard+mouse selection in select popups only updated after unfoc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jay Civelli
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-16 07:48 PDT by Yair Yogev
Modified: 2011-05-04 10:48 PDT (History)
8 users (show)

See Also:


Attachments
testcase (701 bytes, text/html)
2010-10-16 07:48 PDT, Yair Yogev
no flags Details
Forward char event to select popups when one is showing. (1.30 KB, patch)
2011-02-06 00:54 PST, Jay Civelli
no flags Details | Formatted Diff | Diff
Addressing fishd comment (3.51 KB, patch)
2011-02-07 23:46 PST, Jay Civelli
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2011-04-27 14:43 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yair Yogev 2010-10-16 07:48:57 PDT
Created attachment 70954 [details]
testcase

1. Download the testcase and follow the instructions:
2. Open the select element using the mouse by clicking on its downarrow
3. Type a letter [a,d]
4. Click on the highlighted item

The box will close but wil not show the new selected item until the box is unfocused.
If you select some other item instead of the one the keybaord search found, the selection will work as expected.
Pressing the Enter key also works as expected.
This regressed somewhere between 56426 and 56476.

My guess is http://trac.webkit.org/changeset/56449/ (Bug 36062)

inChromium issue: http://code.google.com/p/chromium/issues/detail?id=57124
Comment 1 Yair Yogev 2011-01-29 04:05:13 PST
it is possible that the regression window i posted is only true for Chromium, and Safari was affected even before that change.
Comment 2 Jay Civelli 2011-02-06 00:45:46 PST
I now understand the cause. With that CL we turned on the new behavior for the select popup: the actual popup window does not get the focus and we forward keyboard events to it. We were sending the keydown/up events but not the char ones. They would go to the focus element in the page, in that case the HTMLSelectElement, causing the weird behavior.

Actually the behavior when selecting and item was broken, as the select is supposed to show the matching option as the selected one when you type a key. (like IE does).
Comment 3 Jay Civelli 2011-02-06 00:54:16 PST
Created attachment 81393 [details]
Forward char event to select popups when one is showing.
Comment 4 Darin Fisher (:fishd, Google) 2011-02-07 13:00:35 PST
Comment on attachment 81393 [details]
Forward char event to select popups when one is showing.

View in context: https://bugs.webkit.org/attachment.cgi?id=81393&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:722
> +    // If there is a select popup, it should be the one processing that event,

can you explain why this is the best place to put this delegation?  why shouldn't
this go further up?  should we really call handleAccessKey if there is a popup
present?  etc.
Comment 5 Jay Civelli 2011-02-07 23:46:08 PST
Created attachment 81604 [details]
Addressing fishd comment

You are right Darin, we should not call any handler on the page.
Actually when a select popup is showing, it should process any event key.
I changed the keyEvent() to do just this (it used to pass the event to the page if the select popup would not handle it).
Comment 6 Eric Seidel (no email) 2011-04-06 10:26:51 PDT
Comment on attachment 81604 [details]
Addressing fishd comment

View in context: https://bugs.webkit.org/attachment.cgi?id=81604&action=review

> Source/WebKit/chromium/ChangeLog:5
> +       Send the char events to the select popup if one is showing to fix

Tabs?  Or just strange spacing?
Comment 7 Ojan Vafai 2011-04-26 15:53:14 PDT
Comment on attachment 81604 [details]
Addressing fishd comment

View in context: https://bugs.webkit.org/attachment.cgi?id=81604&action=review

Just one concern. Otherwise the patch looks fine.

> Source/WebKit/chromium/src/WebViewImpl.cpp:570
> +    // If there is a select popup, it should be the one processing the event,
> +    // not the page.
> +    if (m_selectPopup)
> +        return m_selectPopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));

seems like this should be after we set m_suppressNextKeypressEvent to false, no?

> Source/WebKit/chromium/src/WebViewImpl.cpp:687
> +    // If there is a select popup, it should be the one processing the event,
> +    // not the page.
> +    if (m_selectPopup)
> +        return m_selectPopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));

seems like this should be after we set m_suppressNextKeypressEvent to false, no?
Comment 8 Jay Civelli 2011-04-27 14:43:10 PDT
Created attachment 91352 [details]
Patch
Comment 9 Jay Civelli 2011-04-27 14:43:26 PDT
Comment on attachment 81604 [details]
Addressing fishd comment

View in context: https://bugs.webkit.org/attachment.cgi?id=81604&action=review

>> Source/WebKit/chromium/ChangeLog:5
>> +       Send the char events to the select popup if one is showing to fix
> 
> Tabs?  Or just strange spacing?

Fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:570
>> +        return m_selectPopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));
> 
> seems like this should be after we set m_suppressNextKeypressEvent to false, no?

I guess you are right, if the keyEvent was to trigger a select popup, we might leave m_suppressNextKeypressEvent true which would interfere with the next key event sent to the renderer (once the popup is closed).
Comment 10 Ojan Vafai 2011-05-02 10:13:15 PDT
Comment on attachment 91352 [details]
Patch

Is there any way to test this code? I know we can't do it from layout tests, but can we do so from a chromium side test (e.g. a browser_test)?
Comment 11 Jay Civelli 2011-05-04 09:39:08 PDT
I filed a bug on Chromium to write an interactive UI test for it.
http://code.google.com/p/chromium/issues/detail?id=81550
I'll try to do that when I have some cycles.
Comment 12 WebKit Commit Bot 2011-05-04 10:46:51 PDT
The commit-queue encountered the following flaky tests while processing attachment 91352 [details]:

http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2011-05-04 10:47:56 PDT
Comment on attachment 91352 [details]
Patch

Clearing flags on attachment: 91352

Committed r85759: <http://trac.webkit.org/changeset/85759>
Comment 14 WebKit Commit Bot 2011-05-04 10:48:03 PDT
All reviewed patches have been landed.  Closing bug.