Bug 98754

Summary: F4 key should open the picker popup on Windows
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, mifenton, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
removed extra {
none
Patch
none
Patch none

Description Keishi Hattori 2012-10-09 05:37:58 PDT
F4 key should open the picker popup on Windows
Comment 1 Keishi Hattori 2012-10-09 07:30:46 PDT
Created attachment 167751 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-09 08:22:59 PDT
Comment on attachment 167751 [details]
Patch

Attachment 167751 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14214770

New failing tests:
fast/forms/date/calendar-picker-key-operations.html
platform/chromium/fast/forms/date/date-suggestion-picker-key-operations.html
platform/chromium/fast/forms/time/time-suggestion-picker-key-operations.html
Comment 3 Keishi Hattori 2012-10-09 19:22:39 PDT
Created attachment 167894 [details]
removed extra {
Comment 4 Kent Tamura 2012-10-09 20:11:29 PDT
Comment on attachment 167894 [details]
removed extra {

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

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:197
> +    } else if (m_pickerIndicatorIsVisible && event->keyIdentifier() == "Down" && event->getModifierState("Alt")) {
>          m_pickerIndicatorElement->openPopup();

Should we check nullness of m_pickerIndicatorElement here too?

> Source/WebCore/rendering/RenderThemeChromiumWin.cpp:814
> +    return true;

We apply the same key bindings to Chromium-Windows and Chromium-Linux.  Please add this function to RenderThemeChromiumLinux too.
Comment 5 Keishi Hattori 2012-10-09 22:15:33 PDT
Created attachment 167922 [details]
Patch
Comment 6 Kent Tamura 2012-10-09 22:36:31 PDT
Comment on attachment 167922 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +        * html/BaseMultipleFieldsDateAndTimeInputType.cpp:
> +        (WebCore::BaseMultipleFieldsDateAndTimeInputType::handleKeydownEvent):
> +        * rendering/RenderTheme.cpp:
> +        (WebCore::RenderTheme::shouldOpenPickerWithF4Key):
> +        (WebCore):
> +        * rendering/RenderTheme.h:
> +        (RenderTheme):
> +        * rendering/RenderThemeChromiumLinux.cpp:
> +        (WebCore::RenderThemeChromiumLinux::shouldOpenPickerWithF4Key): Returns true if we want to enable the F4 key binding on this platform.
> +        (WebCore):
> +        * rendering/RenderThemeChromiumLinux.h:
> +        * rendering/RenderThemeChromiumWin.cpp:
> +        (WebCore):
> +        (WebCore::RenderThemeChromiumWin::shouldOpenPickerWithF4Key):
> +        * rendering/RenderThemeChromiumWin.h:
> +        (RenderThemeChromiumWin):

Please write per-file / per-function comments as possible
Comment 7 Keishi Hattori 2012-10-11 05:53:10 PDT
Created attachment 168203 [details]
Patch
Comment 8 WebKit Review Bot 2012-10-11 07:04:07 PDT
Comment on attachment 168203 [details]
Patch

Clearing flags on attachment: 168203

Committed r131054: <http://trac.webkit.org/changeset/131054>
Comment 9 WebKit Review Bot 2012-10-11 07:04:12 PDT
All reviewed patches have been landed.  Closing bug.