Bug 84710

Summary: [EFL] [DRT] EventSender doesn't provide correct key string
Product: WebKit Reporter: Sudarsana Nagineni (babu) <naginenis>
Component: WebKit EFLAssignee: Kangil Han <kangil.han>
Status: RESOLVED FIXED    
Severity: Normal CC: d-r, gyuyoung.kim, kangil.han, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78559    
Bug Blocks: 85369    
Attachments:
Description Flags
patch
none
patch
none
patch
none
Patch
none
Patch
none
Patch none

Description Sudarsana Nagineni (babu) 2012-04-24 05:16:32 PDT
EventSender doesn't provide correct key string for special keys(e.g. function keys/arrow keys/space)
Comment 1 Kangil Han 2012-04-29 18:55:15 PDT
Created attachment 139411 [details]
patch
Comment 2 Gyuyoung Kim 2012-04-29 23:29:16 PDT
Comment on attachment 139411 [details]
patch

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

> LayoutTests/ChangeLog:8
> +        Non-visible key string, i.e. right arrow key, shouldn't be displayed on the input field.

I don't understand what is problem in this description.

> Tools/DumpRenderTree/efl/EventSender.cpp:107
> +    KeyEventInfo(const CString& keyName, EvasKeyModifier modifiers, Eina_Bool hasVisibleString)

Please do not use Eina_Bool.

> Tools/DumpRenderTree/efl/EventSender.cpp:116
> +    Eina_Bool hasVisibleString;

We decided to use standard boolean instead of Eina_Bool except for public APIs.

> Tools/DumpRenderTree/efl/EventSender.cpp:447
> +        return new KeyEventInfo(character.get()->ustring().utf8(), modifiers, EINA_TRUE);

Use true instead of EINA_TRUE.

> Tools/DumpRenderTree/efl/EventSender.cpp:449
> +    return new KeyEventInfo(keyName, modifiers, EINA_FALSE);

ditto.
Comment 3 Kangil Han 2012-04-29 23:52:44 PDT
Created attachment 139421 [details]
patch
Comment 4 Kangil Han 2012-04-29 23:54:19 PDT
Thanks for reviewing!
I've fixed all mentioned parts.
Comment 5 Kangil Han 2012-04-30 05:05:22 PDT
Created attachment 139431 [details]
patch
Comment 6 Sudarsana Nagineni (babu) 2012-05-02 06:31:49 PDT
Comment on attachment 139431 [details]
patch

Could you please rebase and upload this?
Comment 7 Kangil Han 2012-05-02 06:41:57 PDT
Yes, I will as soon as BUG 78559 is landed. ;)
Comment 8 Kangil Han 2012-05-08 18:57:13 PDT
Created attachment 140848 [details]
Patch
Comment 9 Kangil Han 2012-05-08 18:58:52 PDT
Rebase done. :)
Comment 10 Kangil Han 2012-05-08 19:16:32 PDT
Created attachment 140851 [details]
Patch
Comment 11 Kangil Han 2012-05-08 19:17:49 PDT
Rebase again since chromium-ews complained about device space. :)
Comment 12 Kangil Han 2012-05-08 23:38:36 PDT
gyuyoung, rakuco : Could I please get informal review on this patch? Thanks!
Comment 13 Gyuyoung Kim 2012-05-08 23:57:44 PDT
Comment on attachment 140851 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:387
> +    return CString();

Hmm, why do you return CString() instead of character->ustring().utf8()? It looks we need to return character itself when character is not supported by this function.

> Tools/DumpRenderTree/efl/EventSender.cpp:451
> +    return CString();

ditto.
Comment 14 Kangil Han 2012-05-09 00:31:47 PDT
In this case, blank string indicates pressed key is not a character.
So 'character->ustring().utf8()' is happened when creating KeyEventInfo.

(In reply to comment #13)
> (From update of attachment 140851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140851&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:387
> > +    return CString();
> 
> Hmm, why do you return CString() instead of character->ustring().utf8()? It looks we need to return character itself when character is not supported by this function.
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:451
> > +    return CString();
> 
> ditto.
Comment 15 Kangil Han 2012-05-10 06:21:57 PDT
gyuyoung : Any more comment(s)?
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-05-12 19:52:15 PDT
Comment on attachment 140851 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        (WebCore::singleCharacterString):

Please mention what is being changed and why here.

> Tools/DumpRenderTree/efl/EventSender.cpp:481
> +    if (keyName.isNull())
> +        return new KeyEventInfo(character.get()->ustring().utf8(), modifiers, true);
> +    if (keyName == "Return" || keyName == "Tab" || keyName == "BackSpace" || keyName == "space")
> +        return new KeyEventInfo(keyName, modifiers, true);
> +
> +    return new KeyEventInfo(keyName, modifiers, false);

This change of logic here is very confusing, it took me a while to figure out what the code was supposed to do. The code should be more explicit in what it does.

My suggestion: add another CString member, `keyString' (or something better) to KeyEventInfo which is then passed as the 4th parameter to evas_event_feed_key_{down,up} so you don't need to put additional meaning into keyName itself. You can even make keyPadNameFromJSValue and keyNameFromJSValue return a KeyEventInfo themselves if that makes the code more simple.
Comment 17 Kangil Han 2012-05-14 02:04:45 PDT
Created attachment 141667 [details]
Patch
Comment 18 Kangil Han 2012-05-14 02:08:04 PDT
(In reply to comment #16)
> (From update of attachment 140851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140851&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        (WebCore::singleCharacterString):
> 
> Please mention what is being changed and why here.
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:481
> > +    if (keyName.isNull())
> > +        return new KeyEventInfo(character.get()->ustring().utf8(), modifiers, true);
> > +    if (keyName == "Return" || keyName == "Tab" || keyName == "BackSpace" || keyName == "space")
> > +        return new KeyEventInfo(keyName, modifiers, true);
> > +
> > +    return new KeyEventInfo(keyName, modifiers, false);
> 
> This change of logic here is very confusing, it took me a while to figure out what the code was supposed to do. The code should be more explicit in what it does.
> 
> My suggestion: add another CString member, `keyString' (or something better) to KeyEventInfo which is then passed as the 4th parameter to evas_event_feed_key_{down,up} so you don't need to put additional meaning into keyName itself. You can even make keyPadNameFromJSValue and keyNameFromJSValue return a KeyEventInfo themselves if that makes the code more simple.

Good suggestion rakuco! Thanks!
Please review my latest patch if I've got your idea. :)
Comment 19 Kangil Han 2012-05-15 07:12:55 PDT
rakuco: Could I please get your review?
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-05-15 07:29:32 PDT
Comment on attachment 141667 [details]
Patch

Looks good now, thank you. If it does not cause any regression in other tests, informal r+.
Comment 21 WebKit Review Bot 2012-05-15 22:05:45 PDT
Comment on attachment 141667 [details]
Patch

Clearing flags on attachment: 141667

Committed r117218: <http://trac.webkit.org/changeset/117218>
Comment 22 WebKit Review Bot 2012-05-15 22:05:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Dominik Röttsches (drott) 2012-05-16 01:07:28 PDT
There was another bug marked as blocked by this issue in test_expectations.txt

// EventSender doesn't provide correct key string
BUGWK84710 : fast/forms/textarea-type-spaces.html = TEXT

resulting in an unexpected pass. I am removing that line from test_expectations.txt in gardening bug 86584.
Comment 24 Kangil Han 2012-05-16 01:24:00 PDT
drott: Thanks!