Closed Bug 1397412 Opened 2 years ago Closed 2 years ago

Implement Mochitest to ensure functioning of plaintext editor's EditorBase::FindBetterInsertionPoint()

Categories

(Core :: Editor, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1393140 +++
This test fails with attachment 8903033 [details] [diff] [review] from bug 1393140 applied.

In fact, once the test is finished, you can try typing into the empty line between the xx and the yy to see where the character is inserted :-(
Attachment #8905278 - Flags: review?(ehsan)
Comment on attachment 8905278 [details] [diff] [review]
1397412-FindBetterInsertionPoint.patch (v1)

Review of attachment 8905278 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/tests/test_bug1397412.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1330796

Oops, copy/paste error here. I'll fix it in v2.
Comment on attachment 8905278 [details] [diff] [review]
1397412-FindBetterInsertionPoint.patch (v1)

Review of attachment 8905278 [details] [diff] [review]:
-----------------------------------------------------------------

The test logic is mostly correct but see the comment below; it needs to be rewritten as a XUL test.  I'd like to look at a modified version if possible.  Thanks.

::: editor/libeditor/tests/test_bug1397412.html
@@ +48,5 @@
> +    .getInterface(Ci.nsIWebNavigation)
> +    .QueryInterface(Ci.nsIInterfaceRequestor)
> +    .getInterface(Ci.nsIEditingSession);
> +  var editor = editingSession.getEditorForWindow(window);
> +  editor.flags |= Ci.nsIPlaintextEditor.eEditorPlaintextMask;

This is a rather made-up mode for the editor, in practice it will never work in this setup.

The code path you are interested in is <https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/editor/composer/nsEditingSession.cpp#356> (or line 360) which you can get by using a XUL <editor> element with editortype="textmail" or "text" respectively.  You can use this test as an example <https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/editor/libeditor/tests/test_bug489202.xul#20> (there's a lot of boilerplate that you can copy from that test.  You can create a <div> or some such under the editor element there and drop the |xx<br><br>yy<br>| content inside it.  The core test logic here can be ported almost directly to that setup, but it's better to use a more real editor setup to make sure what the test exercises has some resemblance to something that the code can encounter in practice.
Attachment #8905278 - Flags: review?(ehsan)
No longer blocks: 651120
The XUL version turned out to be much nicer and allowed me to do two tests easily.

A failure looks like this:
2 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1397412.xul | body.innerHTML: got |xxt<br><br>|, expected |xx<br>t<br>|
3 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1397412.xul | body.innerHTML: got |xx<br><br>yyt<br>|, expected |xx<br>t<br>yy<br>|

A pass looks like this:
2 INFO TEST-PASS | editor/libeditor/tests/test_bug1397412.xul | body.innerHTML: got |xx<br>t<br>|, expected |xx<br>t<br>|
3 INFO TEST-PASS | editor/libeditor/tests/test_bug1397412.xul | body.innerHTML: got |xx<br>t<br>yy<br>|, expected |xx<br>t<br>yy<br>|
Attachment #8905278 - Attachment is obsolete: true
Attachment #8905406 - Flags: review?(ehsan)
Comment on attachment 8905406 [details] [diff] [review]
1397412-FindBetterInsertionPoint.patch (v2).

Review of attachment 8905406 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/tests/test_bug1397412.xul
@@ +53,5 @@
> +  body.innerHTML = initialHTML1;
> +  selection.collapse(body, 2);
> +  synthesizeKey("t", { code: "KeyT" });
> +  var actualHTML = body.innerHTML;
> +  ok(actualHTML == expectedHTML1,

Use is(), not ok().

@@ +61,5 @@
> +  body.innerHTML = initialHTML2;
> +  selection.collapse(body, 2);
> +  synthesizeKey("t", { code: "KeyT" });
> +  actualHTML = body.innerHTML;
> +  ok(actualHTML == expectedHTML2,

Ditto.
Attachment #8905406 - Flags: review?(ehsan) → review+
Changed ok() to is(). Carrying forward Ehsan's r+.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b515e9e2d5502a710e561f72af7ea3bad544144b
Attachment #8905406 - Attachment is obsolete: true
Attachment #8905642 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/903bd3c9bfdd
Implement Mochitest for EditorBase::FindBetterInsertionPoint() in plaintext editor. r=ehsan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/903bd3c9bfdd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.