Comment 202 for bug 584632

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

(In reply to Jorg K from comment #115)
> Created attachment 8584346
> Unified patch (code + test changes + twice revised new test)
>
> Thank you very much for the review and all the additional explanation.
> I hope I could address your questions in additional comments in the test.
>
> Sadly switching from "mousedown/up" to "click" as you had suggested makes
> the test fail with:
> failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure
> code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at
> chrome://specialpowers/content/specialpowersAPI.js:161

Err, sorry, I misspoke. I should have said: "remove the type value" altogether (just pass an empty object `{}'). That should do the right thing: <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#272>

> This is actually the only test I intended to submit ;-)
>
> This is for the case where the user clicks beyond the end of the line.
> There are two tests already which I had to adapt to the new selection
> behaviour which test the moving to the end
> (editor/libeditor/tests/test_selection_move_commands.xul)
> and moving from the subsequent line using left arrow
> (layout/generic/test/test_movement_by_characters.html).
>
> If you wanted to be really purist a test with a "div contenteditable" could
> be added where an empty line is first filled with some text, which is then
> removed so the range becomes empty. The code caters for this case.

I think relying on the selection movement tests is sort of OK, but I really prefer to have much better testing on this, since this code is extremely fragile and I'm pretty sure that without extensive tests, we'll end up breaking it again in the future. That being said, I guess it's not fair for me to make you do this. :-)

But at the very least, we should have tests for clicking past the end of the line with the line terminating with a bunch of inline elements that have a text node inside them. That is essentially what this bug is really about. So please add some test along those lines.

Also, to reiterate, the second test in your test case is wrong...