Bug 86200 - [EFL] fast/events/arrow-keys-on-body.html does not pass
Summary: [EFL] fast/events/arrow-keys-on-body.html does not pass
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 85503
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 05:44 PDT by Mikhail Pozdnyakov
Modified: 2012-05-22 11:17 PDT (History)
4 users (show)

See Also:


Attachments
Fix. (2.86 KB, patch)
2012-05-11 06:25 PDT, Mikhail Pozdnyakov
gns: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
to be landed. (2.99 KB, patch)
2012-05-16 06:59 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-05-11 05:44:11 PDT
fast/events/arrow-keys-on-body.html wrong test result, keypress should not be emitted.
Comment 1 Mikhail Pozdnyakov 2012-05-11 06:07:56 PDT
Keypress event should be raised by noncharacter keys.
Comment 2 Mikhail Pozdnyakov 2012-05-11 06:25:34 PDT
Created attachment 141397 [details]
Fix. 

Non-characters keys have empty text value. This patch should be applied after bug85503 is landed, as bug85503 fixes windowsKeyMap population.
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-05-11 06:47:33 PDT
LGTM.
Comment 4 Gyuyoung Kim 2012-05-13 19:18:58 PDT
Comment on attachment 141397 [details]
Fix. 

Looks fine too.
Comment 5 WebKit Review Bot 2012-05-16 06:50:40 PDT
Comment on attachment 141397 [details]
Fix. 

Rejecting attachment 141397 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
fset -113 lines).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/efl/EflKeyboardUtilities.cpp
Hunk #1 succeeded at 202 (offset 12 lines).
Hunk #2 FAILED at 215.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/efl/EflKeyboardUtilities.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gustavo No..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12703792
Comment 6 Mikhail Pozdnyakov 2012-05-16 06:59:01 PDT
Created attachment 142247 [details]
to be landed.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-05-16 07:26:33 PDT
Comment on attachment 142247 [details]
to be landed.

Hmm. Can you check if your change is still needed after r117218?
Comment 8 Mikhail Pozdnyakov 2012-05-16 07:39:57 PDT
(In reply to comment #7)
> (From update of attachment 142247 [details])
> Hmm. Can you check if your change is still needed after r117218?

I guess might make sense, r117218 is touching EventSender which is part of DRT.
Changes in this bug are part of core where I think the values still should be checked.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-05-16 08:13:09 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Hmm. Can you check if your change is still needed after r117218?
> 
> I guess might make sense, r117218 is touching EventSender which is part of DRT.
> Changes in this bug are part of core where I think the values still should be checked.

r117218 completely solves the problem when the code path is reached via DRT, right? Then it would be good to check if this is something that can happen via normal user interaction as well, that is, check how ecore_evas initializes the event structure passed to evas -- I can't remember how it initializes the string member off the top of my head.
Comment 10 Mikhail Pozdnyakov 2012-05-16 10:40:40 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Hmm. Can you check if your change is still needed after r117218?
> > 
> > I guess might make sense, r117218 is touching EventSender which is part of DRT.
> > Changes in this bug are part of core where I think the values still should be checked.
> 
> r117218 completely solves the problem when the code path is reached via DRT, right? Then it would be good to check if this is something that can happen via normal user interaction as well, that is, check how ecore_evas initializes the event structure passed to evas -- I can't remember how it initializes the string member off the top of my head.

from http://docs.enlightenment.org/auto/evas/group__Evas__Event__Feeding__Group.html :

void evas_event_feed_key_down 	(Evas *  	e,
		const char *  	keyname,
		const char *  	key,
		const char *  	string,
		const char *  	compose,
		unsigned int  	timestamp,
		const void *  	data 
	) 		

Key down event feed.

Parameters:
    e	The canvas to thaw out
    keyname	Name of the key
    key	The key pressed.
    string	A String
    compose	The compose string
    timestamp	Timestamp of the mouse up event
    data	Data for canvas.

The situations like 

evas_event_feed_key_down(evas, "Left", "Left", "some garbage that will turn up in webkit PlatformKeyboardEvent", 0, 0, 0);

are quite possible.
Comment 11 Mikhail Pozdnyakov 2012-05-18 01:20:35 PDT
> The situations like 
> 
> evas_event_feed_key_down(evas, "Left", "Left", "some garbage that will turn up in webkit PlatformKeyboardEvent", 0, 0, 0);
> 
> are quite possible.
And this will lead to appearance of unneeded Keypress events.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-05-18 07:31:26 PDT
We have discussed this on IRC, and agreed to only unskip the test that is already passing. I've done so in <http://trac.webkit.org/changeset/117591>.

The rationale is that right now ecore and evas do not have a cross-backend way of representing keys (like what's present in gdk and Qt), and the current implementation in EflKeyboardUtilities.cpp is only expected to work with the ecore_x xlib backend.

We therefore do not expect people to do what was cited in comment #11, and so we have decided not to fix what is not broken.
Comment 13 Raphael Kubo da Costa (:rakuco) 2012-05-18 07:32:15 PDT
Closing for now -- if we do find some problem in the future, we can revisit this patch.