718487 - SimpleTest/EventUtils.js: Improve sendKey() to correctly handle arguments beginning with VK_

Status: RESOLVED INCOMPLETE

(Testing :: Mochitest, defect)

Description

[Serge Gautherie (:sgautherie)]

NB: See _computeKeyCodeFromChar() and synthesizeKey().

(In reply to Ehsan Akhgari [:ehsan] from bug 717868 comment #12)
> make sendKey a bit smarter here so
> that it won't choke when you pass something prefixed with "VK_" to it? 

I think sendKey() should rather throw an error, also if not passed a "non-character key".

It should be possible to check whether the resulting "DOM_VK_" actually exists.

sendKey() documentation should not require aKey to be lowercase, or should throw if not the case.

***

80  * For now this method only works for English letters (lower and upper case)
81  * and the digits 0-9.
82  */
83 function sendChar(aChar, aWindow) {

sendChar() should check aChar value and throw if it is not supported.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> sendKey() documentation should not require aKey to be lowercase, or should
> throw if not the case.

In case it is actually wanted to be lowercase, update existing callers:
http://mxr.mozilla.org/mozilla-central/search?string=sendKey(&case=on

Comment 2

[(no longer active)]

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> NB: See _computeKeyCodeFromChar() and synthesizeKey().
> 
> (In reply to Ehsan Akhgari [:ehsan] from bug 717868 comment #12)
> > make sendKey a bit smarter here so
> > that it won't choke when you pass something prefixed with "VK_" to it? 
> 
> I think sendKey() should rather throw an error, also if not passed a
> "non-character key".

Throwing an error is not going to be helpful, since that's more or less the existing behavior.  We have no way of knowing whether the thrown exception is handled gracefully or not.

I propose we should make sendKey more lenient by making it check whether the argument passed to it begins with "VK_", and prepend "VK_" to it if it's not.

> It should be possible to check whether the resulting "DOM_VK_" actually
> exists.

The EventUtils functions don't have access to SimpleTest.is and such, so we don't have a good way of testing that.

> sendKey() documentation should not require aKey to be lowercase, or should
> throw if not the case.

That is just bad documentation which needs to be fixed.

> 80  * For now this method only works for English letters (lower and upper
> case)
> 81  * and the digits 0-9.
> 82  */
> 83 function sendChar(aChar, aWindow) {
> 
> sendChar() should check aChar value and throw if it is not supported.

Like I said, throwing exceptions doesn't buy us much.

Therefore, I propose that for the purpose of this bug, we should only focus on making sendKey more lenient.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to Ehsan Akhgari [:ehsan] from comment #2)

> Throwing an error is not going to be helpful, since that's more or less the
> existing behavior.  We have no way of knowing whether the thrown exception
> is handled gracefully or not.

I disagree. I don't think it really matters how the exception is actually handled.

synthesizeKey() throws: this (eventually) helped notice the error and fixed it in bug 717868 :-)

send*() don't (check nor) throw: impossible to notice bugs like bug 718546 from test results/logs :-(

> The EventUtils functions don't have access to SimpleTest.is and such, so we
> don't have a good way of testing that.

I meant internal argument value validation, not SimpleTest.*().

NB:
These functions could return a boolean (which could be tested by SimpleTest.*()), but that may be overkill.
In case we would want to do it, we should probably encapsulate that in new SimpleTest functions.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> In case we would want to do it, we should probably encapsulate that in new
> SimpleTest functions.

PS: Or, send*() throws -> SimpleTest encapsulation function try+catch if need be ;-)

Comment 5

[(no longer active)]

Given the choice between an API which lets you use it badly and one that does not, I would always go for the second one.  :-)

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Given the choice between an API which lets you use it badly and one that
> does not, I would always go for the second one.  :-)

In short, there are 3 options:
1- Lenient: not throw, as currently.
2- Normal: throw and let callers (not) deal with it.
3- Overkill: Throw and add a SimpleTest encapsulation to explicitly report the exception there.
   NB: SimpleTest must let the test continue, it can/must not finish().

I already voted for option 2:
it should be obvious enough that something is wrong (even if the harness doesn't notice that exception) as the char/string/key won't be sent, thus the test can be expected to fail explicitly soon after (through the harness)...

You enforced option 1. Do you change your decision?

Comment 7

[(no longer active)]

What I suggested is have an API which tries to do the right thing (prepending "VK_" if it's not already there), so that things like sendKey("BACK_SPACE") and sendKey("VK_BACK_SPACE") would both work.  This doesn't fit into any of the 3 alternatives you have proposed.

But I'm not the module owner for this code, and I don't think this discussion will go anywhere without the module owner chiming in.  Ted, what do you think?

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> What I suggested is have an API which tries to do the right thing
> (prepending "VK_" if it's not already there), so that things like
> sendKey("BACK_SPACE") and sendKey("VK_BACK_SPACE") would both work.  This
> doesn't fit into any of the 3 alternatives you have proposed.

Currently, it is documented 'and works) as
"the part that comes after "DOM_VK_" in the KeyEvent constant name for this key."
which seems fine to me.

Fwiw, I prefer to support one set of values only (but either one).

Comment 9

[Nils M.]

Is this bug still relevant? What is the final decision about the way to go?

Comment 10

[Carsten Book [:Tomcat]]

(In reply to Nils M. from comment #9)
> Is this bug still relevant? What is the final decision about the way to go?

setting n-i for ehsan since he can maybe answer this questions best

Comment 11

[(no longer active)]

It's unclear whether this bug serves any purpose still.  Let's close it as incomplete.