[webkit-reviews] review granted: [Bug 14407] ALT + DOWN arrow key does not open select : [Attachment 100777] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 14407: ALT + DOWN arrow key does not open select
https://bugs.webkit.org/show_bug.cgi?id=14407

Attachment 100777: Patch
https://bugs.webkit.org/attachment.cgi?id=100777&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100777&action=review


Please consider implementing F4 as mentioned in bug 22232 comment 6 to end this
topic for good.

I'm going to say r=me, but I'm also requesting some non-trivial changes to be
made before landing. If you want another quick review round when these changes
are made, please feel free to post a new patch for review.

> Source/WebCore/ChangeLog:6
> +	   https://bugs.webkit.org/show_bug.cgi?id=14407
> +	   <rdar://problem/5319507>

It's not so obvious why this can't be tested. Please add a brief explanation to
the ChangeLog.

> Source/WebCore/ChangeLog:11
> +	   * WebCore.vcproj/WebCore.vcproj:
> +	   Add new file to project.

To make this comment more useful, you can name the new file.

> Source/WebCore/ChangeLog:32
> +	   * platform/win/PopupMenuWin.cpp:
> +	   (WebCore::PopupMenuWin::show):
> +	   Forward WM_SYSKEYDOWN to the popup's HWND.
> +	   (WebCore::PopupMenuWin::wndProc):
> +	   When handling up or down arrow, if the alt key is pressed, tell the
> +	   client that the value changed, and hide the menu.

Please do mention why you are making this change. I assume that you found that
it's how IE behaves when testing it, but the bug didn't ask for this.

> Source/WebCore/dom/SelectElement.h:109
> +    static bool platformHandleKeyboardEvent(SelectElementData&, Element*,
KeyboardEvent*, int& selectedListIndex);

This function's name makes it sound like it's a universal override, which it is
not - there is a ton of handling that happens regardless of what you do in this
function. Is there any more specific name that you could give it?

Also, I think that ARROW_KEYS_POP_MENU code needs to be moved into this
function in this patch to avoid having two completely different approaches to
customizing behavior. Either that, or Alt-arrows should be implemented with an
#if similar to ARROW_KEYS_POP_MENU without adding platform specific files.

> Source/WebCore/dom/SelectElementWin.cpp:40
> +    if (!event->altKey())
> +	   return false;
> +
> +    if (event->keyIdentifier() == "Down" || event->keyIdentifier() == "Up")
{

This check is incorrect - we almost certainly don't want Alt+Shift+Down and
such to be handled here. In addition to checking that alt is pressed, you
should check that no other modifier (except for possibly Caps Lock) is pressed.