Bug 84710 – [EFL] [DRT] EventSender doesn't provide correct key string

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

[Sudarsana Nagineni (babu)]

EventSender doesn't provide correct key string for special keys(e.g. function keys/arrow keys/space)

[Kangil Han]

Created attachment 139411 [details]
patch

[Gyuyoung Kim]

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.

[Kangil Han]

Created attachment 139421 [details]
patch

[Kangil Han]

Thanks for reviewing!
I've fixed all mentioned parts.

[Kangil Han]

Created attachment 139431 [details]
patch

[Sudarsana Nagineni (babu)]

Comment on attachment 139431 [details]
patch

Could you please rebase and upload this?

[Kangil Han]

Yes, I will as soon as BUG 78559 is landed. ;)

[Kangil Han]

Created attachment 140848 [details]
Patch

[Kangil Han]

Rebase done. :)

[Kangil Han]

Created attachment 140851 [details]
Patch

[Kangil Han]

Rebase again since chromium-ews complained about device space. :)

[Kangil Han]

gyuyoung, rakuco : Could I please get informal review on this patch? Thanks!

[Gyuyoung Kim]

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.

[Kangil Han]

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.

[Kangil Han]

gyuyoung : Any more comment(s)?

[Raphael Kubo da Costa (:rakuco)]

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.

[Kangil Han]

Created attachment 141667 [details]
Patch

[Kangil Han]

(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. :)

[Kangil Han]

rakuco: Could I please get your review?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 141667 [details]
Patch

Looks good now, thank you. If it does not cause any regression in other tests, informal r+.

[WebKit Review Bot]

Comment on attachment 141667 [details]
Patch

Clearing flags on attachment: 141667

Committed r117218: <http://trac.webkit.org/changeset/117218>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

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.

[Kangil Han]

drott: Thanks!