Bug 47769

Summary: [chromium] Keyboard+mouse selection in select popups only updated after unfocused
Product: WebKit Reporter: Yair Yogev <progame+wk>
Component: FormsAssignee: Jay Civelli <jcivelli>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dglazkov, eric, fishd, jcampan, jcivelli, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
testcase
none
Forward char event to select popups when one is showing.
none
Addressing fishd comment
none
Patch none

Yair Yogev
Reported 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
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
DetailsFormatted DiffDiff
Addressing fishd comment (3.51 KB, patch)
2011-02-07 23:46 PST, Jay Civelli 		no flags
DetailsFormatted DiffDiff
Patch (3.27 KB, patch)
2011-04-27 14:43 PDT, Jay Civelli 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yair Yogev
Comment 1 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.
Jay Civelli
Comment 2 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).
Jay Civelli
Comment 3 2011-02-06 00:54:16 PST
Created attachment 81393 [details] Forward char event to select popups when one is showing.
Darin Fisher (:fishd, Google)
Comment 4 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.
Jay Civelli
Comment 5 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).
Eric Seidel (no email)
Comment 6 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?
Ojan Vafai
Comment 7 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?
Jay Civelli
Comment 8 2011-04-27 14:43:10 PDT
Created attachment 91352 [details] Patch
Jay Civelli
Comment 9 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).
Ojan Vafai
Comment 10 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)?
Jay Civelli
Comment 11 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.
WebKit Commit Bot
Comment 12 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.
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2011-05-04 10:48:03 PDT
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.