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
Created attachment 50448 [details] Proposed patch
(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.
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.
(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"];
(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).
Created attachment 50490 [details] Proposed patch Changed according Simon's comments. Using OS(DARWIN) to be more generic.
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 <diego.gonzalez@openbossa.org> > 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 @@ > +2010-03-10 Diego Gonzalez <diego.gonzalez@openbossa.org> > + > + 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 <dglazkov@chromium.org> > > 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 @@ > +2010-03-10 Diego Gonzalez <diego.gonzalez@openbossa.org> > + > + 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 <jorlow@chromium.org> > > 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 >
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