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

RESOLVED FIXED in mozilla11

Status

()

Core
Event Handling
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

Trunk
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

71.35 KB, patch
smaug
: review+
Ehsan
: review+
Dolske
: review+
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
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.
Created attachment 572399 [details] [diff] [review]
Patch
Attachment #572399 - Flags: review?(ehsan)
Attachment #572399 - Flags: review?(bugs)
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 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?
Attachment #572399 - Flags: review?(ehsan) → review+
(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.
> @@ +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 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.
Attachment #572399 - Flags: review?(dolske)
smaug: ping

Comment 8

6 years ago
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.
Attachment #572399 - Flags: review?(enndeakin)
Attachment #572399 - Flags: review?(bugs)
Attachment #572399 - Flags: review+
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.
Attachment #572399 - Flags: review?(enndeakin) → review+
Attachment #572399 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/34b8fe028357

I'll file bugs for the reviewed comments.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
backed out from inbound because the frequency of bug 504586 rose too high especially on WinXP debug.
Whiteboard: [inbound]
https://hg.mozilla.org/integration/mozilla-inbound/rev/f720be43486d
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/f720be43486d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Depends on: 711903
Keywords: dev-doc-needed
Depends on: 711906
You need to log in before you can comment on or make changes to this bug.