700199 - EventUtils.js should use synthesized events for sendKey(), sendChar() and sendString() rather than untrusted events

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

Attachment: Patch

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[(no longer active)]

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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

> @@ +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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

smaug: ping

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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]

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] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34b8fe028357

I'll file bugs for the reviewed comments.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed out from inbound because the frequency of bug 504586 rose too high especially on WinXP debug.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f720be43486d

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f720be43486d