Last Comment Bug 700199 - EventUtils.js should use synthesized events for sendKey(), sendChar() and sendString() rather than untrusted events
: EventUtils.js should use synthesized events for sendKey(), sendChar() and sen...
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 711906 711903
Blocks: 698949
  Show dependency treegraph
 
Reported: 2011-11-06 18:07 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-12-18 20:53 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (71.35 KB, patch)
2011-11-06 22:56 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
ehsan: review+
dolske: review+
enndeakin: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-06 18:07:28 PST
spinning off from bug 698949, sendKey(), sendChar() and sendString() should use synthesized events rather than untrusted events. The synthesized events are handled by focused elements, however, the untrusted events are handled by its target element. So, it sometimes makes unreal case like test_basic_form_autocomplete.html.

The coming patch changes following tests.

> docshell/test/navigation/test_bug430723.html

tests both down and up keys' results rather than only up key's.

> editor/libeditor/text/tests/test_bug641466.html

There is no backspace key. There is only back_space key.

> toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html

This test has a bug. Test 700 doesn't set focus before dispatching key events. It causes different result than actual behavior. Test 700 should cause showing up the autocomplete popup. See bug 497541 comment 12 and later comments.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-06 22:56:55 PST
Created attachment 572399 [details] [diff] [review]
Patch
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-22 17:50:56 PST
Ehsan, smaug:

Would you review this patch? This patch is needed for landing bug 698949's patch. And this patch makes the tests stricter.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2011-11-28 11:13:32 PST
Comment on attachment 572399 [details] [diff] [review]
Patch

Review of attachment 572399 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/test/browser/browser_ruleview_editor.js
@@ +40,5 @@
>      initial: "explicit initial",
>      start: function() {
>        is(span.inplaceEditor.input.value, "explicit initial", "Explicit initial value should be used.");
>        span.inplaceEditor.input.value = "Test Value";
> +      EventUtils.sendKey("return");

Do we know that the input is necessarily focused at this point?

@@ +73,5 @@
>      advanceChars: ":",
>      start: function() {
>        let input = span.inplaceEditor.input;
>        for each (let ch in "Test:") {
> +        EventUtils.sendChar(ch);

Ditto here.

@@ +90,5 @@
>      element: span,
>      initial: "initial text",
>      start: function() {
>        span.inplaceEditor.input.value = "Test Value";
> +      EventUtils.sendKey("escape");

And here.

::: editor/libeditor/text/tests/test_bug641466.html
@@ +28,5 @@
>      element.selectionEnd = 4;
> +    synthesizeKey("VK_BACK_SPACE", {});
> +    synthesizeKey("VK_BACK_SPACE", {});
> +    synthesizeKey("VK_BACK_SPACE", {});
> +    synthesizeKey("VK_BACK_SPACE", {});

I'm curious, how did this test pass before?

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +105,3 @@
>   */
> +function sendKey(aKey, aWindow) {
> +  keyName = "VK_" + aKey.toUpperCase();

Can you also check here to make sure that the symbol exists on nsIDOMKeyEvent, something along the lines of:

ok(keyName in Components.interfaces.nsIDOMKeyEvent, "The " + keyName + " key event exists");

This will prevent people like me to use "backspace" instead of "back_space" for example.  :-)

@@ +346,5 @@
>        utils.sendKeyEvent(aEvent.type, keyCode, 0, modifiers);
>      } else {
>        var keyDownDefaultHappened =
>            utils.sendKeyEvent("keydown", keyCode, 0, modifiers);
> +      // XXX Shouldn't dispatch keypress event if the key is a modifier key.

Make this a todo(false, "...") check please?

::: toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html
@@ +729,5 @@
>          // Turn our attention to form9 to test the dropdown - bug 497541
>          uname = $_(9, "uname");
>          pword = $_(9, "pword");
> +        uname.focus();
> +        sendString("form9userAB");

Can you ask dolske to also take a quick look at this change please?
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-28 21:57:01 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Comment on attachment 572399 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Review of attachment 572399 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/test/browser/browser_ruleview_editor.js
> @@ +40,5 @@
> >      initial: "explicit initial",
> >      start: function() {
> >        is(span.inplaceEditor.input.value, "explicit initial", "Explicit initial value should be used.");
> >        span.inplaceEditor.input.value = "Test Value";
> > +      EventUtils.sendKey("return");
> 
> Do we know that the input is necessarily focused at this point?

The test suite sets focus to the "element" before testing and starts the test from focus event. So, we don't need to touch them.
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/CssRuleView.jsm#1117

> ::: editor/libeditor/text/tests/test_bug641466.html
> @@ +28,5 @@
> >      element.selectionEnd = 4;
> > +    synthesizeKey("VK_BACK_SPACE", {});
> > +    synthesizeKey("VK_BACK_SPACE", {});
> > +    synthesizeKey("VK_BACK_SPACE", {});
> > +    synthesizeKey("VK_BACK_SPACE", {});
> 
> I'm curious, how did this test pass before?

The key name begins with "VK_", therefore, EventUtils.js sends a key events whose keycode and charcode are 0. That becomes empty string input event for nsPlaintextEditor::HandleKeypress(). However, I have no idea why they cause removing the existing characters...

> ::: testing/mochitest/tests/SimpleTest/EventUtils.js
> @@ +105,3 @@
> >   */
> > +function sendKey(aKey, aWindow) {
> > +  keyName = "VK_" + aKey.toUpperCase();
> 
> Can you also check here to make sure that the symbol exists on
> nsIDOMKeyEvent, something along the lines of:
> 
> ok(keyName in Components.interfaces.nsIDOMKeyEvent, "The " + keyName + " key
> event exists");
> 
> This will prevent people like me to use "backspace" instead of "back_space"
> for example.  :-)

Yes, it's done in synthesizeKey() which will be called by sendKey().
# Added by this patch.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-29 05:07:54 PST
> @@ +346,5 @@
>>        utils.sendKeyEvent(aEvent.type, keyCode, 0, modifiers);
>>      } else {
>>        var keyDownDefaultHappened =
>>            utils.sendKeyEvent("keydown", keyCode, 0, modifiers);
>> +      // XXX Shouldn't dispatch keypress event if the key is a modifier key.
> 
> Make this a todo(false, "...") check please?

Hmm, it's impossible to use todo() (and also SimpleTest) in the EventUtils.js. Anyway, I'll fix it ASAP. So, we should land this patch without the todo().
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-29 05:10:46 PST
Comment on attachment 572399 [details] [diff] [review]
Patch

Justin:

Would you check the change for test_basic_form_autocomplete.html?

As I said in comment 0, current test #700 is buggy.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-12-01 05:07:33 PST
smaug: ping
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-12-01 06:44:32 PST
Comment on attachment 572399 [details] [diff] [review]
Patch

looks ok to me.
But since this is focus handling related, would be good to get comments
from Neil.
Comment 9 Neil Deakin 2011-12-05 06:36:40 PST
Comment on attachment 572399 [details] [diff] [review]
Patch

Review of attachment 572399 [details] [diff] [review]:
-----------------------------------------------------------------

test_bug430723.html seems like it should be using waitForFocus.

One additional possibility is to replace instances of sendKey( with synthesizeKey('VK_' + , and add a line to the beginning of synthesizeKey: aEvent = aEvent || { }, so that there aren't two similar-looking and possibly confusing apis.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-12-07 18:47:51 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/34b8fe028357

I'll file bugs for the reviewed comments.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-12-07 23:45:19 PST
backed out from inbound because the frequency of bug 504586 rose too high especially on WinXP debug.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-12-16 05:41:51 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f720be43486d
Comment 13 Matt Brubeck (:mbrubeck) 2011-12-17 09:05:39 PST
https://hg.mozilla.org/mozilla-central/rev/f720be43486d

Note You need to log in before you can comment on or make changes to this bug.