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

RESOLVED FIXED in Firefox 57

Status

()

Core
Editor
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 months ago
+++ This bug was initially created as a clone of Bug #1393140 +++
(Assignee)

Comment 1

3 months ago
Created attachment 8905278 [details] [diff] [review]
1397412-FindBetterInsertionPoint.patch (v1)

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)
(Assignee)

Comment 2

3 months ago
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 3

3 months ago
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)

Updated

3 months ago
No longer blocks: 651120
(Assignee)

Comment 4

3 months ago
Created attachment 8905406 [details] [diff] [review]
1397412-FindBetterInsertionPoint.patch (v2).

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 5

3 months ago
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+
(Assignee)

Comment 6

3 months ago
Created attachment 8905642 [details] [diff] [review]
1397412-FindBetterInsertionPoint.patch (v2b)

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+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 7

3 months ago
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
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.