Bug 47769 – [chromium] Keyboard+mouse selection in select popups only updated after unfocused

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Yair Yogev]

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

[Yair Yogev]

it is possible that the regression window i posted is only true for Chromium, and Safari was affected even before that change.

[Jay Civelli]

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]

Created attachment 81393 [details]
Forward char event to select popups when one is showing.

[Darin Fisher (:fishd, Google)]

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]

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

Created attachment 91352 [details]
Patch

[Jay Civelli]

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

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]

The commit-queue encountered the following flaky tests while processing attachment 91352 [details]:

http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 91352 [details]
Patch

Clearing flags on attachment: 91352

Committed r85759: <http://trac.webkit.org/changeset/85759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.