[webkit-reviews] review denied: [Bug 19465] Cursor sometimes gets 'stuck' in textareas when trying to navigate with arrow keys : [Attachment 21642] Updated patch for stuck cursor with test case

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jonathon Jongsma
<jonathon.jongsma at collabora.co.uk>'s request for review:
Bug 19465: Cursor sometimes gets 'stuck' in textareas when trying to navigate
with arrow keys
https://bugs.webkit.org/show_bug.cgi?id=19465

Attachment 21642: Updated patch for stuck cursor with test case
https://bugs.webkit.org/attachment.cgi?id=21642&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks!

This looks good.

Normally, we require that the patch includes the output of the test was well as
the input. So there should be files with "expected" in their names in the
patch. You generate those by running run-webkit-tests.

Also, we try to design the tests so they are self explanatory. Someone should
be able to tell what successful results look like.

Further, we often make the tests "text only" so they can be used
cross-platform. That's done by making sure the actual text of the result
reflects success (so a dump of the render tree is not necessary) and adding a
call to layoutTestController.dumpAsText. This tells the test engine to dump
only the text rather than the render tree.

There are many examples to show how this is done. One is
editing/selection/anchor-focus1.html -- just to pick out a specific example.

By the way, I'm not absolutely sure you need the runEditingTest() machinery. I
think that sets up some things you really don't need if you're testing input in
a textarea. You'd have to find a different way to do
moveSelectionForwardBySentenceCommand() and moveSelectionForwardByLineCommand()
if you didn't use editing.js -- I think you could just make keyboard events for
arrow keys for those.

It's not good to use the "by sentence" in the test, since that's very platform
dependent. It would be better to move to the end of the line than to move
forward by a sentence. Maybe the easiest thing would be to move forward by a
line and then backward by a character?

review- because this doesn't include expected test results, but please consider
my other comments too.