Bug 86200

Keypress event should be raised by noncharacter keys.

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.

LGTM.

Comment on attachment 141397 [details] Fix. Looks fine too.

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

Created attachment 142247 [details] to be landed.

Comment on attachment 142247 [details] to be landed. Hmm. Can you check if your change is still needed after r117218?

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

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

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

> 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.

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.

Closing for now -- if we do find some problem in the future, we can revisit this patch.