Bug 35993 – [Qt] Access key modifier should be Ctrl+Alt for Darwin derived OS and Alt for the others

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Diego Gonzalez]

Return Atl Key for access key modifiers in Qt EventHandler

LayoutTests:
    fast/forms/access-key.html
    fast/forms/focus-selection-textarea.html
    fast/events/access-key-self-destruct.html
    fast/forms/legend-access-key.html

[Diego Gonzalez]

Created attachment 50448 [details]
Proposed patch

[Antonio Gomes]

(In reply to comment #1)
> Created an attachment (id=50448) [details]
> Proposed patch

LGTM, Diego.

It is similar to what Gtk does, and I agree: Alt is much more common accel modifier than Control.

[Simon Hausmann]

Comment on attachment 50448 [details]
Proposed patch

Good catch, Diego!

Ironically I changed it from Alt to Ctrl in revision 29688 :-)

Today for example fast/forms/legend-access-key.html expects ctrl on platforms where the user agent contains "Mac OS X" and alt on the others. So I think the _correct_ fix would be to return Ctrl for OS(MAC_OS_X) and Alt otherwise.

[Diego Gonzalez]

(In reply to comment #3)
> (From update of attachment 50448 [details])
> Good catch, Diego!
> 
> Ironically I changed it from Alt to Ctrl in revision 29688 :-)
> 
> Today for example fast/forms/legend-access-key.html expects ctrl on platforms
> where the user agent contains "Mac OS X" and alt on the others. So I think the
> _correct_ fix would be to return Ctrl for OS(MAC_OS_X) and Alt otherwise.

Thanks Simon :) You mean:

 unsigned EventHandler::accessKeyModifiers()
 {
+#if OS(MAC_OS_X)
+    return PlatformKeyboardEvent::CtrlKey;
+#else
     return PlatformKeyboardEvent::AltKey;
+#endif
 }

or maybe:

 unsigned EventHandler::accessKeyModifiers()
 {
+#if OS(MAC_OS_X)
+    return PlatformKeyboardEvent::CtrlKey | PlatformKeyboardEvent::AltKey;
+#else
     return PlatformKeyboardEvent::AltKey;
+#endif
?

in the tests the modifiers for Mac OS X are Ctrl and Alt:
modifiers = ["ctrlKey", "altKey"];

[Simon Hausmann]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 50448 [details] [details])
> > Good catch, Diego!
> > 
> > Ironically I changed it from Alt to Ctrl in revision 29688 :-)
> > 
> > Today for example fast/forms/legend-access-key.html expects ctrl on platforms
> > where the user agent contains "Mac OS X" and alt on the others. So I think the
> > _correct_ fix would be to return Ctrl for OS(MAC_OS_X) and Alt otherwise.
> 
> Thanks Simon :) You mean:
> 
>  unsigned EventHandler::accessKeyModifiers()
>  {
> +#if OS(MAC_OS_X)
> +    return PlatformKeyboardEvent::CtrlKey;
> +#else
>      return PlatformKeyboardEvent::AltKey;
> +#endif
>  }
> 
> or maybe:
> 
>  unsigned EventHandler::accessKeyModifiers()
>  {
> +#if OS(MAC_OS_X)
> +    return PlatformKeyboardEvent::CtrlKey | PlatformKeyboardEvent::AltKey;
> +#else
>      return PlatformKeyboardEvent::AltKey;
> +#endif
> ?
> 
> in the tests the modifiers for Mac OS X are Ctrl and Alt:
> modifiers = ["ctrlKey", "altKey"];

The latter seems more accurate, indeed.

See also WebCore/page/chromium/EventHandlerChromium.cpp.

Interestingly they're using OS(DARWIN) instead of (MAC_OS_X).

[Diego Gonzalez]

Created attachment 50490 [details]
Proposed patch

Changed according Simon's comments. Using OS(DARWIN) to be more generic.

[Simon Hausmann]

Comment on attachment 50490 [details]
Proposed patch

Looks good to me. Please fix the changelog before landing though. Atl -> Alt and
I don't think the main title should be a question :)

> From b44596d47f9c1531843b7feae8adc058f6f0a01d Mon Sep 17 00:00:00 2001
> From: Diego Gonzalez <[e-mail]>
> Date: Wed, 10 Mar 2010 19:16:26 -0400
> Subject: [PATCH] Return Atl Key for access key modifiers in Qt EventHandler
> 
> LayoutTests:
>     fast/forms/access-key.html
>     fast/forms/focus-selection-textarea.html
>     fast/events/access-key-self-destruct.html
>     fast/forms/legend-access-key.html
> ---
>  LayoutTests/ChangeLog              |   17 +++++++++++++++++
>  LayoutTests/platform/qt/Skipped    |    4 ----
>  WebCore/ChangeLog                  |   18 ++++++++++++++++++
>  WebCore/page/qt/EventHandlerQt.cpp |    6 +++++-
>  4 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index f1c4010..74a3301 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,20 @@
> [phone]  Diego Gonzalez  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Return Atl Key for access key modifiers in Qt EventHandler
> +
> +        LayoutTests:
> +            fast/forms/access-key.html
> +            fast/forms/focus-selection-textarea.html
> +            fast/events/access-key-self-destruct.html
> +            fast/forms/legend-access-key.html
> +
> +        [Qt] Should not access key modifier be the Alt key?
> +        https://bugs.webkit.org/show_bug.cgi?id=35993
> +
> +        * platform/qt/Skipped:
> +
>  2010-03-05  Dimitri Glazkov  <[e-mail]>
>  
>          Reviewed by Sam Weinig.
> diff --git a/LayoutTests/platform/qt/Skipped b/LayoutTests/platform/qt/Skipped
> index 8399bcf..c3a9863 100644
> --- a/LayoutTests/platform/qt/Skipped
> +++ b/LayoutTests/platform/qt/Skipped
> @@ -461,12 +461,10 @@ fast/events/standalone-image-drag-to-editable.html
>  fast/events/updateLayoutForHitTest.html
>  fast/forms/input-list.html
>  fast/forms/input-selectedoption.html
> -fast/forms/access-key.html
>  fast/forms/button-cannot-be-nested.html
>  fast/forms/control-restrict-line-height.html
>  fast/forms/drag-into-textarea.html
>  fast/forms/focus-selection-input.html
> -fast/forms/focus-selection-textarea.html
>  fast/forms/input-readonly-autoscroll.html
>  fast/forms/input-text-click-outside.html
>  fast/forms/input-text-double-click.html
> @@ -1157,7 +1155,6 @@ fast/dynamic/selection-highlight-adjust.html
>  fast/encoding/char-decoding.html
>  fast/encoding/frame-default-enc.html
>  fast/encoding/invalid-xml.html
> -fast/events/access-key-self-destruct.html
>  fast/events/attempt-scroll-with-no-scrollbars.html
>  fast/events/key-events-in-input-button.html
>  fast/forms/input-align-image.html
> @@ -1165,7 +1162,6 @@ fast/forms/input-align.html
>  fast/forms/input-double-click-selection-gap-bug.html
>  fast/forms/input-radio-checked-tab.html
>  fast/forms/input-text-option-delete.html
> -fast/forms/legend-access-key.html
>  fast/forms/textarea-rows-cols.html
>  fast/frames/onlyCommentInIFrame.html
>  fast/inline/positionedLifetime.html
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index eef9d52..87319d3 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,21 @@
> [phone]  Diego Gonzalez  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Return Atl Key for access key modifiers in Qt EventHandler
> +
> +        LayoutTests:
> +            fast/forms/access-key.html
> +            fast/forms/focus-selection-textarea.html
> +            fast/events/access-key-self-destruct.html
> +            fast/forms/legend-access-key.html
> +
> +        [Qt] Should not access key modifier be the Alt key?
> +        https://bugs.webkit.org/show_bug.cgi?id=35993
> +
> +        * page/qt/EventHandlerQt.cpp:
> +        (WebCore::EventHandler::accessKeyModifiers):
> +
>  2010-03-10  Jeremy Orlow  <[e-mail]>
>  
>          Reviewed by Darin Fisher.
> diff --git a/WebCore/page/qt/EventHandlerQt.cpp b/WebCore/page/qt/EventHandlerQt.cpp
> index 3425289..d7982fa 100644
> --- a/WebCore/page/qt/EventHandlerQt.cpp
> +++ b/WebCore/page/qt/EventHandlerQt.cpp
> @@ -132,7 +132,11 @@ bool EventHandler::passMouseReleaseEventToSubframe(MouseEventWithHitTestResults&
>  
>  unsigned EventHandler::accessKeyModifiers()
>  {
> -    return PlatformKeyboardEvent::CtrlKey;
> +#if OS(DARWIN)
> +    return PlatformKeyboardEvent::CtrlKey | PlatformKeyboardEvent::AltKey;
> +#else
> +    return PlatformKeyboardEvent::AltKey;
> +#endif
>  }
>  
>  }
> -- 
> 1.6.3.3
>

[Antonio Gomes]

I committed(In reply to comment #7)
> (From update of attachment 50490 [details])
> Looks good to me. Please fix the changelog before landing though. Atl -> Alt

Diego fixed the typo, and I landed manually. Thx

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/qt/Skipped
	M	WebCore/ChangeLog
	M	WebCore/page/qt/EventHandlerQt.cpp
Committed r55847