Bug 85369

Summary: [EFL] [DRT] keyboard-related tests do not pass
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 84710, 85479, 85485, 85503, 86836, 86837, 86838    
Bug Blocks:    
Attachments:
Description Flags
patch
gyuyoung.kim: commit-queue-
patch v2 (drt changes)
none
patch v3 (fixed review comments) none

Mikhail Pozdnyakov
Reported 2012-05-02 06:11:28 PDT
PlatformKeyboardEventEfl implementation should be fixed: - event.keyIdentifier, event.keyCode, event.charCode should have proper values - PlatformKeyboardEvent::getCurrentModifierState should be implemented in order to unskip: fast/events/js-keyboard-event-creation.html fast/events/key-events-in-input-button.html fast/events/key-events-in-input-text.html fast/events/keydown-numpad-keys.html fast/events/onsearch-enter.html fast/events/option-tab.html fast/events/special-key-events-in-input-text.html fast/forms/button-enter-click.html fast/forms/button-spacebar-click.html fast/forms/enter-clicks-buttons.html fast/forms/input-search-press-escape-key.html fast/forms/input-text-enter.html fast/forms/textinput-not-fired-on-enter-in-input.html http/tests/misc/isindex-with-no-form.html
Attachments
patch (8.67 KB, patch)
2012-05-18 01:17 PDT, Mikhail Pozdnyakov 		gyuyoung.kim: commit-queue-
DetailsFormatted DiffDiff
patch v2 (drt changes) (3.10 KB, patch)
2012-05-23 00:22 PDT, Mikhail Pozdnyakov 		no flags
DetailsFormatted DiffDiff
patch v3 (fixed review comments) (2.78 KB, patch)
2012-05-24 06:28 PDT, Mikhail Pozdnyakov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1 2012-05-18 01:17:38 PDT
Created attachment 142660 [details] patch Fixes the left testcases from the list.
Gyuyoung Kim
Comment 2 2012-05-18 02:58:58 PDT
Comment on attachment 142660 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=142660&action=review > LayoutTests/ChangeLog:3 > + [EFL] PlatformKeyboardEventEfl implementation fix This patch implements new functionality, fix potential bug and refactoring code. Please do not mix multiple patches into a patch. > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:228 > + return "\x1b"; To be consistent with other condition, let's use String(""). > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:444 > + // From GTK. Don't allow text insertion for nodes that cannot edit. It looks this condition is clear. So, it seems to me "From GTK" is not needed.
Mikhail Pozdnyakov
Comment 3 2012-05-18 03:05:55 PDT
(In reply to comment #2) > (From update of attachment 142660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142660&action=review > > > LayoutTests/ChangeLog:3 > > + [EFL] PlatformKeyboardEventEfl implementation fix > > This patch implements new functionality, fix potential bug and refactoring code. Please do not mix multiple patches into a patch. Thanks for review. Splitting it..
Mikhail Pozdnyakov
Comment 4 2012-05-23 00:22:56 PDT
Created attachment 143483 [details] patch v2 (drt changes)
Gyuyoung Kim
Comment 5 2012-05-23 00:59:58 PDT
Comment on attachment 143483 [details] patch v2 (drt changes) View in context: https://bugs.webkit.org/attachment.cgi?id=143483&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:455 > + modifiers |= EvasKeyModifierShift; As we discussed on irc, I think we need to know why shift should be adjust into modifiers before landing this patch.
Mikhail Pozdnyakov
Comment 6 2012-05-23 01:10:43 PDT
(In reply to comment #5) > (From update of attachment 143483 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143483&action=review > > > Tools/DumpRenderTree/efl/EventSender.cpp:455 > > + modifiers |= EvasKeyModifierShift; > > As we discussed on irc, I think we need to know why shift should be adjust into modifiers before landing this patch. Probable reason is that VK_ codes do not distinguish between lows and caps, and having of SHIFT modifier is the only way to understand the case
Gyuyoung Kim
Comment 7 2012-05-23 01:28:05 PDT
Comment on attachment 143483 [details] patch v2 (drt changes) View in context: https://bugs.webkit.org/attachment.cgi?id=143483&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:454 > + if ((character->length() == 1 ) && ('A' <= charCode && charCode <= 'Z')) I'm not sure if this is webkit style, personally, I would prefer operand to operator in comparison condition. For example, (charCode >= 'A' && charCode <= 'Z') >>> Tools/DumpRenderTree/efl/EventSender.cpp:455 >>> + modifiers |= EvasKeyModifierShift; >> >> As we discussed on irc, I think we need to know why shift should be adjust into modifiers before landing this patch. > > Probable reason is that VK_ codes do not distinguish between lows and caps, and having of SHIFT modifier is the only way to understand the case If Virtual key codes only can distinguish them, I'm ok. But, in my opinion, it is good to add comment for this to here.
Gyuyoung Kim
Comment 8 2012-05-23 01:30:58 PDT
(In reply to comment #7) >> If Virtual key codes only can distinguish them Oops, wrong typing. If virtual key codes only can distinguish between lows and caps when there is shift modifier in modifier, I'm ok.
Mikhail Pozdnyakov
Comment 9 2012-05-23 03:41:08 PDT
(In reply to comment #8) > (In reply to comment #7) > > >> If Virtual key codes only can distinguish them > Oops, wrong typing. > > If virtual key codes only can distinguish between lows and caps when there is shift modifier in modifier, I'm ok. Here is the actual reason described: From d15ea3fef52046f49dbca9a8f7633799255c7750 Mon Sep 17 00:00:00 2001 From: "darin@apple.com" <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc> Date: Thu, 29 Nov 2007 00:43:14 +0000 Subject: [PATCH] Reviewed by Adam Roben. * DumpRenderTree/mac/EventSendingController.mm: (-[EventSendingController keyDown:withModifiers:]): Send capital letters through as lowercase letters with the shift key down rather than sending them as if they were highly unusual "capital letter keys".
Gyuyoung Kim
Comment 10 2012-05-23 19:31:00 PDT
(In reply to comment #7) > > Tools/DumpRenderTree/efl/EventSender.cpp:454 > > + if ((character->length() == 1 ) && ('A' <= charCode && charCode <= 'Z')) > > I'm not sure if this is webkit style, personally, I would prefer operand to operator in comparison condition. For example, > (charCode >= 'A' && charCode <= 'Z') Any opinions ?
Raphael Kubo da Costa (:rakuco)
Comment 11 2012-05-23 19:58:50 PDT
Comment on attachment 143483 [details] patch v2 (drt changes) View in context: https://bugs.webkit.org/attachment.cgi?id=143483&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:442 > - int charCode = JSStringGetCharactersPtr(character)[0]; > + const char charCode = JSStringGetCharactersPtr(character)[0]; Why change this? >> Tools/DumpRenderTree/efl/EventSender.cpp:454 >> + if ((character->length() == 1 ) && ('A' <= charCode && charCode <= 'Z')) > > I'm not sure if this is webkit style, personally, I would prefer operand to operator in comparison condition. For example, > (charCode >= 'A' && charCode <= 'Z') I second Gyuyoung's comment, and there's also an extra space after "1". >>>> Tools/DumpRenderTree/efl/EventSender.cpp:455 >>>> + modifiers |= EvasKeyModifierShift; >>> >>> As we discussed on irc, I think we need to know why shift should be adjust into modifiers before landing this patch. >> >> Probable reason is that VK_ codes do not distinguish between lows and caps, and having of SHIFT modifier is the only way to understand the case > > If Virtual key codes only can distinguish them, I'm ok. But, in my opinion, it is good to add comment for this to here. Adding a comment would be good for future reference.
Mikhail Pozdnyakov
Comment 12 2012-05-24 06:19:26 PDT
(In reply to comment #11) > (From update of attachment 143483 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143483&action=review > > > Tools/DumpRenderTree/efl/EventSender.cpp:442 > > - int charCode = JSStringGetCharactersPtr(character)[0]; > > + const char charCode = JSStringGetCharactersPtr(character)[0]; > > Why change this? Did it to avoid unnecessary conversions each time we compare 'charCode' with a symbol. Will remove it from the patch as it is not related to actual fix of DRT.
Mikhail Pozdnyakov
Comment 13 2012-05-24 06:28:34 PDT
Created attachment 143814 [details] patch v3 (fixed review comments)
Mikhail Pozdnyakov
Comment 14 2012-05-24 06:32:29 PDT
(In reply to comment #10) > (In reply to comment #7) > > > Tools/DumpRenderTree/efl/EventSender.cpp:454 > > > + if ((character->length() == 1 ) && ('A' <= charCode && charCode <= 'Z')) > > > > I'm not sure if this is webkit style, personally, I would prefer operand to operator in comparison condition. For example, > > (charCode >= 'A' && charCode <= 'Z') > > Any opinions ? Let's do this way. Fixed.
Raphael Kubo da Costa (:rakuco)
Comment 15 2012-05-24 07:01:40 PDT
Comment on attachment 143814 [details] patch v3 (fixed review comments) Looks good, thanks.
WebKit Review Bot
Comment 16 2012-05-24 12:37:26 PDT
Comment on attachment 143814 [details] patch v3 (fixed review comments) Clearing flags on attachment: 143814 Committed r118404: <http://trac.webkit.org/changeset/118404>
WebKit Review Bot
Comment 17 2012-05-24 12:37:39 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.