Bug 14407 – ALT + DOWN arrow key does not open select

• Component: Accessibility
• Status: RESOLVED FIXED

[Tore B. Krudtaa]

ALT + DOWN arrow key does not open select

Likewise I would like ALT + UP arrow key to close the select.

The other browsers handles this well, hope Safari can implement this to work the same way so the endusers kan use what they are familiar with.

In Safari I'm unable to open the select list by using the keyboard.

[Alexey Proskuryakov]

Confirmed with r23677.

[David Kilzer (:ddkilzer)]

<rdar://problem/5319507>

[Jon Honeycutt]

Created attachment 100777 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 100777 [details]
Patch

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.

[Jon Honeycutt]

(In reply to comment #4)
> (From update of attachment 100777 [details])
> 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.

Added.

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

New patch coming.

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

Added a comment.

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

Done.

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

Added a comment about matching Firefox.

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

Renamed to platformHandleKeydownEvent, and modified so that if the platform function says it handled the event, we return early.

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

Moved it into the function, although I retained the #if rather than moving it to a separate file that those platforms included.

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

I did some testing with Firefox and updated the code to match what they allowed (which included Alt+Shift+Down and other modifiers).

Thanks for the review!

[Jon Honeycutt]

Created attachment 100956 [details]
Patch v2

[WebKit Review Bot]

Attachment 100956 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/PopupMenuWin.cpp:825:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 100956 [details]
Patch v2

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

> Source/WebCore/dom/SelectElementWin.cpp:37
> +    // Allow (Shift/Command) F4 and (Ctrl/Shift/Command) Alt/AltGr + Up/Down arrow to pop the menu, matching Firefox.

I'm not sure if matching Firefox here is much good. In keyboard event handling, we generally only care about matching IE - and when implementing platform conventions like this one, it seems to be even more appropriate.

The comment is quite helpful explaining why the code does what it does. I don't think that Windows has a Command key though!

[Simon Fraser (smfr)]

This broke several builds, e.g.
http://build.webkit.org/builders/WinCE%20Release%20%28Build%29/builds/10709/steps/compile-webkit/logs/stdio

[Jon Honeycutt]

(In reply to comment #9)
> This broke several builds, e.g.
> http://build.webkit.org/builders/WinCE%20Release%20%28Build%29/builds/10709/steps/compile-webkit/logs/stdio

Landed in <http://trac.webkit.org/changeset/91129>. Attempted a WinCE build fix in <http://trac.webkit.org/changeset/91149>. I don't see any other builds broken from this change at the moment.

[Alexey Proskuryakov]

*** Bug 22232 has been marked as a duplicate of this bug. ***