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
it is possible that the regression window i posted is only true for Chromium, and Safari was affected even before that change.
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).
Created attachment 81393 [details] Forward char event to select popups when one is showing.
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.
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 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 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?
Created attachment 91352 [details] Patch
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 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)?
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.
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 on attachment 91352 [details] Patch Clearing flags on attachment: 91352 Committed r85759: <http://trac.webkit.org/changeset/85759>
All reviewed patches have been landed. Closing bug.