1393140 - Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt()

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Comment 1

[(no longer active)]

Attachment: Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt()

Comment 2

[(no longer active)]

Comment on attachment 8900395 [details] [diff] [review]
Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt()

Hmm, I think this isn't quite right yet...

Comment 3

[(no longer active)]

Attachment: Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt()

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8901521 [details] [diff] [review]
Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt()

># HG changeset patch
># User Ehsan Akhgari <[e-mail]>
>
>Bug 1393140 - Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt(); r=masayuki
>
>diff --git a/editor/libeditor/EditorBase.cpp b/editor/libeditor/EditorBase.cpp
>index b0cd7beea94b..cedb48a2dfc4 100644
>--- a/editor/libeditor/EditorBase.cpp
>+++ b/editor/libeditor/EditorBase.cpp
>@@ -2434,22 +2434,29 @@ EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode,
>       aNode = node->GetFirstChild();
>       aOffset = 0;
>       return;
>     }
> 
>     // In some other cases, aNode is the anonymous DIV, and offset points to the
>     // terminating mozBR.  In that case, we'll adjust aInOutNode and
>     // aInOutOffset to the preceding text node, if any.
>-    if (offset > 0 && node->GetChildAt(offset - 1) &&
>-        node->GetChildAt(offset - 1)->IsNodeOfType(nsINode::eTEXT)) {
>-      NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
>-      aNode = node->GetChildAt(offset - 1);
>-      aOffset = static_cast<int32_t>(aNode->Length());
>-      return;
>+    if (offset) {
>+      nsIContent* child = node->GetLastChild();
>+      if (child) {
>+        if (child->GetPreviousSibling()) {
>+          child = child->GetPreviousSibling();
>+        }
>+        if (child->IsNodeOfType(nsINode::eTEXT)) {

I don' think that you won't change the behavior.

offset is copy of aOffset here. So, aOffset doesn't mean the offset of the last child.

I think that we need to change the arguments from a pair of a container node and offset to a node which is pointed by aNode and aOffset.

Comment 5

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> Comment on attachment 8901521 [details] [diff] [review]
> Rewrite EditorBase::FindBetterInsertionPoint() without using
> nsINode::GetChildAt()
> 
> ># HG changeset patch
> ># User Ehsan Akhgari <[e-mail]>
> >
> >Bug 1393140 - Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt(); r=masayuki
> >
> >diff --git a/editor/libeditor/EditorBase.cpp b/editor/libeditor/EditorBase.cpp
> >index b0cd7beea94b..cedb48a2dfc4 100644
> >--- a/editor/libeditor/EditorBase.cpp
> >+++ b/editor/libeditor/EditorBase.cpp
> >@@ -2434,22 +2434,29 @@ EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode,
> >       aNode = node->GetFirstChild();
> >       aOffset = 0;
> >       return;
> >     }
> > 
> >     // In some other cases, aNode is the anonymous DIV, and offset points to the
> >     // terminating mozBR.  In that case, we'll adjust aInOutNode and
> >     // aInOutOffset to the preceding text node, if any.
> >-    if (offset > 0 && node->GetChildAt(offset - 1) &&
> >-        node->GetChildAt(offset - 1)->IsNodeOfType(nsINode::eTEXT)) {
> >-      NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
> >-      aNode = node->GetChildAt(offset - 1);
> >-      aOffset = static_cast<int32_t>(aNode->Length());
> >-      return;
> >+    if (offset) {
> >+      nsIContent* child = node->GetLastChild();
> >+      if (child) {
> >+        if (child->GetPreviousSibling()) {
> >+          child = child->GetPreviousSibling();
> >+        }
> >+        if (child->IsNodeOfType(nsINode::eTEXT)) {
> 
> I don' think that you won't change the behavior.
> 
> offset is copy of aOffset here. So, aOffset doesn't mean the offset of the
> last child.

I don't think this is changing behavior, unless I'm missing something about how the code works.  Did you note the comment before the changed code?  As far as I can tell, this part of the code is trying to detect the case where the offset points to the terminating mozBR.  Because of the specific shape of the DOM tree under the anonymous DIV inside text controls, if you have a node which has a previous sibling which is a text node, then that node is a mozBR, and the old version of the code is relying on this contract.  FWIW a lot of code in various places also assumes specific things about the specific shape of the DOM tree under this anonymous DIV (which is generally one text node if the contents isn't empty followed by perhaps a mozBR)

Therefore, I chose to rewrite this as I did.  The check on whether GetPreviousSibling() exists is needed because the existence of the mozBR node is actually not quite guaranteed by our code, so we have to be careful to not assume that there is always a previous sibling when not using offsets.

> I think that we need to change the arguments from a pair of a container node
> and offset to a node which is pointed by aNode and aOffset.

I guess that would be possible also, but it would require a lot more code changes.  But I thought I'd explain the rationale behind the current patch first in case it would help clarify what it does.  FWIW I did investigate under the debugger that it doesn't change the return value because I realized that our test coverage is probably not going to be sufficient for this.  (I did find bug in a previous iteration of this patch under the debugger which wasn't caught by any tests...)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Did you note the comment before the changed code?

Yeah, I read it, but I didn't believe that since it's not guaranteed with any code.

> As far as I can tell, this part of the code is trying to detect the case where the offset points to the terminating mozBR.
> Because of the specific shape of the DOM tree under the anonymous DIV inside text controls, if you have a node which has a
> previous sibling which is a text node, then that node is a mozBR, and the old version of the code is relying on this
> contract.  FWIW a lot of code in various places also assumes specific things about the specific shape of the DOM tree
> under this anonymous DIV (which is generally one text node if the contents isn't empty followed by perhaps a mozBR)

How about the <scrollbar>s, <scrollconer> and <resizer> after mozBR in <textarea>? Won't they appear with nsINode::GetLastChild()?

Anyway, the number of children of anon-<div> isn't so many. How about to scan the siblings with a loop until reaching a text node or the first child? If you're right, the running cost won't be changed but same behavior will be guaranteed.

Comment 7

[(no longer active)]

Attachment: frame dump

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > Did you note the comment before the changed code?
> 
> Yeah, I read it, but I didn't believe that since it's not guaranteed with
> any code.

Yes, it is.  See nsTextControlFrame::CreateAnonymousContent() and nsTextControlFrame::AppendAnonymousContentTo().  That is where the editor root (aka the anonymous div) is created.  What's put in the editor root is guaranteed by a whole bunch of code in the editor.  The relevant parts of the code, off the top of my head are TextEditRules::WillInsertText() is the main place to look at.

Also, I mentioned that we have code that relies on this DOM structure, but it's pretty hard to find all such code because there's not an easy thing one can grep for, but for example I remembered that there's code in nsContentUtils that depends on this, and I found an example for you: <https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/base/nsContentUtils.cpp#7410>

BTW the comment I quoted comes from bug 240933 where I implemented the behavior that I am describing here.  Before that bug, textareas used a <br> to represent newlines and used several textnodes under their anonymous div.  See attachment 458475 [details] [diff] [review].  That bug is where all of the work to guarantee this specific DOM structure happened, but unfortunately due to the nature of our code not all of the code relevant to this is in one place together so this fact may not be quite obvious.

> > As far as I can tell, this part of the code is trying to detect the case where the offset points to the terminating mozBR.
> > Because of the specific shape of the DOM tree under the anonymous DIV inside text controls, if you have a node which has a
> > previous sibling which is a text node, then that node is a mozBR, and the old version of the code is relying on this
> > contract.  FWIW a lot of code in various places also assumes specific things about the specific shape of the DOM tree
> > under this anonymous DIV (which is generally one text node if the contents isn't empty followed by perhaps a mozBR)
> 
> How about the <scrollbar>s, <scrollconer> and <resizer> after mozBR in
> <textarea>? Won't they appear with nsINode::GetLastChild()?

Those are attached using XBL bindings.  XBL bindings generate their anonymous content outside of native anonymous subtrees always, see nsXBLBinding::GenerateAnonymousContent().  Also, it may help to look at the frame dump of a <textarea> element for example using the Layout Debugger to see how these fit together in practice.  For example I navigated to data:text/html,<textarea>hello%20world</textarea> in the Layout Debugger and entered some newlines at the end, and here is the frame dump.

> Anyway, the number of children of anon-<div> isn't so many. How about to
> scan the siblings with a loop until reaching a text node or the first child?
> If you're right, the running cost won't be changed but same behavior will be
> guaranteed.

OK, I will do that for this patch since I think we've argued over this too much at this point.  But I really want you to believe me here because otherwise you may also think that code such as the above function I linked to is incorrect whereas it's perfectly fine.

Anyways, I'll make a new version of this patch hopefully later today.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7)
> > Anyway, the number of children of anon-<div> isn't so many. How about to
> > scan the siblings with a loop until reaching a text node or the first child?
> > If you're right, the running cost won't be changed but same behavior will be
> > guaranteed.
> 
> OK, I will do that for this patch since I think we've argued over this too
> much at this point.  But I really want you to believe me here because
> otherwise you may also think that code such as the above function I linked
> to is incorrect whereas it's perfectly fine.

Now I believe you since I misunderstood about the <scrollbar> etc.

> Anyways, I'll make a new version of this patch hopefully later today.

Or, just using MOZ_ASSERT is fine for detecting unexpected tree with automated tests.

Comment 9

[(no longer active)]

Attachment: Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt(); r=masayuki

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2305bd57a97
Rewrite EditorBase::FindBetterInsertionPoint() without using nsINode::GetChildAt(); r=masayuki

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d2305bd57a97

Comment 12

[Jorg K (CEST = GMT+2)]

Hmm, that caused at least one of our Mozmill tests to fail where we do a bit of editing in a message. We'll have to analyse the failure.

Comment 13

[Jorg K (CEST = GMT+2)]

Would you mind backing this out, it breaks the editor and writing plain text messages:

Do this locally:
Start a plain text e-mail, type |xx<enter><space>|, the space goes after the xx and the enter is lost.

Comment 14

[Jorg K (CEST = GMT+2)]

Here you see it:
<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">xx <br><br></body>
The space went after the xx, not the <br>.

Comment 15

[Jorg K (CEST = GMT+2)]

If you try that in a Nightly, keep in mind that we use 
  execCommand("defaultparagraphseparator", false, "br");
for plaintext compositions. The <br>s get inserted with _moz_dirty. I'll see whether I can reproduce it in a HTML page later.

Comment 16

[Jorg K (CEST = GMT+2)]

I fail to see why that change makes sense:

-    if (offset > 0 && node->GetChildAt(offset - 1) &&
-        node->GetChildAt(offset - 1)->IsNodeOfType(nsINode::eTEXT)) {
-      NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
-      aNode = node->GetChildAt(offset - 1);
-      aOffset = static_cast<int32_t>(aNode->Length());
-      return;
+    if (offset) {
+      nsIContent* child = node->GetLastChild();
+      while (child) {
+        if (child->IsNodeOfType(nsINode::eTEXT)) {
+          NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
+          aNode = child;
+          aOffset = static_cast<int32_t>(aNode->Length());
+          return;
+        }
+        child = child->GetPreviousSibling();
+      }

In the old code, under some conditions you return
-      aNode = node->GetChildAt(offset - 1);
-      aOffset = static_cast<int32_t>(aNode->Length());
taking 'offset' into account.

In the new code, you traverse from end and return the first text node you fiend, not even looking at 'offset'.

So in TB I see this:

===
test followed by <br>, if you type into the empty line, you append here:

===

or

===
Some text followed by two <br>

Some text. If you type into the empty line, you append here:
===

That completely destroys writing e-mail in so called "plain text mode" which uses:
<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">

I haven't been able to reproduce this in Nightly FF, since I don't know under which circumstances the new code is executed.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, I realised now why I can't find an example in FF. This code only runs of IsPlaintextEditor() which we seems to set in TB for plain text e-mail.

Comment 18

[:aceman]

Comment 4 and 5 talk about not changing behavior, but the case in TB means it did change behaviour.

Jorg, right, in HTML composition mode in TB the bug does not appear.

I wonder what mode e.g. textarea (like this one in bugzilla) runs, if that is not IsPlaintextEditor(). I know there is also some rich edit mode in FF, but I haven't seen it yet.

Comment 19

[Jorg K (CEST = GMT+2)]

I think Ehsan and Masayuki were associating "plaintext editor" with "textarea". But TB uses the plaintext editor for plaintext mail, and sadly, that breaks big time with this change.

Comment 20

[:aceman]

But today's Firefox Nightly does not exhibit the problem (in textarea of bugzilla).

Comment 21

[Jorg K (CEST = GMT+2)]

That's right, that's why the landed the bug ;-) But it smashes the plaintext editor used in TB completely.

To show the problem in FF, you'd need a contenteditable and programmatically set the plaintext flag on the editor. Then you will see the bug. That can be done in a Mochitest but not with a simple test HTML test case. I tried for a while until I found this line not visible in the patch:

void
EditorBase::FindBetterInsertionPoint(nsCOMPtr<nsINode>& aNode,
                                     int32_t& aOffset)
{
...
  if (!IsPlaintextEditor()) { <<<=== 
    return;

I think it's best to back this out and redo it again keeping the TB use-case in mind.

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1ac1cb838d99
Backed out changeset d2305bd57a97 for changed behavior on request from jorgk. r=backout a=backout

Comment 23

[(no longer active)]

Hmm, do we have a new policy for patches to be backed out for Thunderbird breakage before the developer writing the patch has had a chance to look at the comments and respond that was announced somewhere and I missed it?  Was there any reason that the normal process of filing a follow-up bug describing what's wrong not followed here?

You're asking me to do all of the extra work of cloning comm-central, figuring out how to run this failing test, debugging it, etc.  You have to understand that acting like this and backing out a patch that I have worked on as part of a quite large project worked on by several people without bothering to at least wait for me to get back to you is quite disrespectful.

Comment 24

[Jorg K (CEST = GMT+2)]

Hello Ehsan,

I'm sorry you are unhappy with the events that occurred. The patch got landed on Friday night, or more precisely, the early AM hours of Saturday, that's more than four days ago at time of writing. Due to the weekend and apparently a public holiday on Monday, we would have shipped broken Dailies for a few days.

Your patch makes the plaintext editor used but Thunderbird useless, you cannot write plaintext e-mail any more. As I understand it, the M-C plaintext interface is a published interface and there were no announced plans to withdraw it or render it useless for other Gecko applications.

I looked at your rather small patch for a long time and ways to fix it, but given the urgency and severity of the breakage and your unavailability decided to ask a sheriff to back it out. Also, reading bug 1387406 comment #17 "I'll be offline tomorrow due to National holiday of Japan. So, if these patch would break something, feel free to back them our by yourself." I applied that option to the bug here.

Reading through the bug, there were some doubts of whether it would change established behaviour, and I'm afraid it did in a big way. I appreciate that this is part of a larger project of making Firefox faster, but with all due respect, I think this code executes when the user types, so shaving off milliseconds might not be so important to the success of the overall project.

Also, filing a follow-up bug would have delayed proceedings and potentially moved the onus of fixing the breakage onto someone else, which I don't find correct personally.

Now coming to what went wrong in the patch: I described it in comment #16. The simplest way to experience the damage is to download a Thunderbird Daily of 2017-09-02 from here and try writing a plaintext mail hitting <enter> a few times:
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/09/2017-09-02-03-02-05-comm-central/
Our users also noticed the malfunction, see bug 1396296.

It is not only about test failures, you don't need to compile TB or run any tests. However, since we now know that the plaintext editor is used in circumstances other than expected, it might be good to write a Mochitest which can set the plaintext editor bit and and thus simulate what's going on in TB.

I hope I've explained why backing out this change was the fastest and cleanest solution under the given circumstances. I don't understand why that should have anything to do with being disrespectful.

Kind regards, Jörg.

Comment 25

[Jorg K (CEST = GMT+2)]

Typo: ... makes the plaintext editor used *by* Thunderbird useless ...

Comment 26

[Jorg K (CEST = GMT+2)]

P.S.: If you wish I can write such a Mochitest for you, I would base it on
https://dxr.mozilla.org/mozilla-central/rev/3ecda4678c49ca255c38b1697142b9118cdd27e7/editor/libeditor/tests/test_bug1330796.html#87
Instead of setting the mail editor bit, we would set the plaintext editor bit, use
<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;" contenteditable>xx<br><br></body>
then position after the first <br> and strike a key.

A variation would be:
<body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;" contenteditable>xx<br><br>yy<br></body>

Comment 27

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #24)
> Hello Ehsan,
> 
> I'm sorry you are unhappy with the events that occurred. The patch got
> landed on Friday night, or more precisely, the early AM hours of Saturday,
> that's more than four days ago at time of writing. Due to the weekend and
> apparently a public holiday on Monday, we would have shipped broken Dailies
> for a few days.

Jorg, we have a well established process for development of Firefox which documents what types of regressions are fair game for the change to be reverted: https://developer.mozilla.org/en-US/docs/Supported_build_configurations.  And Firefox developers deserve the courtesy of being able to disconnect during a long weekend without having to monitor their needinfo queues to remind people of this process.

If Thunderbird cares about a crucial piece of functionality to not break as a result of m-c development, the burden of doing the work to make that fit into our process is on the Thunderbird team, and it can be done by writing automated tests that run on m-c.  Anything that runs off m-c is non-Tier-1 configuration that should be expected to break at any time without prior notice.  As you very well know, when we can we try to let the Thunderbird team know in advance when we expect some breakage.  I have personally tried to go out of my way to do this many times in the past, but this doesn't mean that the burden of keeping Thunderbird Daily builds working is somehow shifted onto Firefox developers.

> Your patch makes the plaintext editor used but Thunderbird useless, you
> cannot write plaintext e-mail any more. As I understand it, the M-C
> plaintext interface is a published interface and there were no announced
> plans to withdraw it or render it useless for other Gecko applications.

I have no idea what you're referring to by "published interface" and why you think there should be a process for announcing plans to withdraw it or render it useless.  As I mentioned above, where we can we have always notified Thunderbird developers in advance of changes that we expect would impact them *out of courtesy*.  And this is completely irrelevant to this bug since as you note yourself there wasn't any intentional behavior change, but a simple bug in the patch.

> I looked at your rather small patch for a long time and ways to fix it, but
> given the urgency and severity of the breakage and your unavailability
> decided to ask a sheriff to back it out. Also, reading bug 1387406 comment
> #17 "I'll be offline tomorrow due to National holiday of Japan. So, if these
> patch would break something, feel free to back them our by yourself." I
> applied that option to the bug here.

The comment you are referring to wasn't from me.  Assuming Masayuki's kindness in offering to have his changes be backed out a month ago somehow automatically applies to *my* changes without having the same consent from me, and assuming being entitled to my availability during my weekend time when I'm spending time with my family is way out of line.  Both of your assumptions here were utterly false I'm afraid.

> Reading through the bug, there were some doubts of whether it would change
> established behaviour, and I'm afraid it did in a big way. I appreciate that
> this is part of a larger project of making Firefox faster, but with all due
> respect, I think this code executes when the user types, so shaving off
> milliseconds might not be so important to the success of the overall project.

Are you trying to persuade me that bug 651120 is not worth the effort because it "shaves off milliseconds"?  Have you studied the bug, looked at what it does, seen the benchmarks?  Do you think Thunderbird doesn't benefit from that optimization?!

Sigh, sometimes working on Gecko truly feels like a thankless job.  Just for your information, the bug which you're trying to dissuade me from pursuing is probably the biggest optimization that we have made to our code DOM structure in the recent few years.

> Also, filing a follow-up bug would have delayed proceedings and potentially
> moved the onus of fixing the breakage onto someone else, which I don't find
> correct personally.

Yes, to Thunderbird developers, as one would naturally expect the onus of fixing a Thunderbird regression would normally fall onto.  But it was easier to back out the work of the Firefox developer before they had a chance to get back to answer your question and shift the responsibility to them by gating the relanding of the patch on fixing the Thunderbird regression also, is that what you're implying?

> Now coming to what went wrong in the patch: I described it in comment #16.
> The simplest way to experience the damage is to download a Thunderbird Daily
> of 2017-09-02 from here and try writing a plaintext mail hitting <enter> a
> few times:
> http://ftp.mozilla.org/pub/thunderbird/nightly/2017/09/2017-09-02-03-02-05-
> comm-central/
> Our users also noticed the malfunction, see bug 1396296.

Yes, I understand that this broke Thunderbird, I'm not disputing that.  I'm just trying to help you understand that you don't get to a) feel entitled to people's time especially during weekends and holidays, and b) feel that Thunderbird is somehow a special case where the normal Mozilla development processes don't apply, and c) shift the responsibility of keeping Thunderbird working onto Firefox developers.

In the future please refrain from such behavior.

> It is not only about test failures, you don't need to compile TB or run any
> tests. However, since we now know that the plaintext editor is used in
> circumstances other than expected, it might be good to write a Mochitest
> which can set the plaintext editor bit and and thus simulate what's going on
> in TB.

Yes, please file a separate bug and add the mochitest there.  After this back and forth, I don't feel like building TB to run the failing Mozmill test myself for this bug at hand any more to be honest.  I'm planning to look at this bug again next week and if the mochitest is added by then I can take a look at how to address the test failure with the patch applied.

> I hope I've explained why backing out this change was the fastest and
> cleanest solution under the given circumstances. I don't understand why that
> should have anything to do with being disrespectful.

That was only the fastest and cleanest solution if the only thing you care about is getting the Thunderbird Daily build to work again and making it *my* problem to deal with the actual breakage, so indeed if we're all optimizing to for that goal then nothing was wrong.  But I hope the above helps you understand that other people may have different perspectives on how their time should be spent on weekends and weekdays.  And regardless, we have processes for a reason.  Anyway, not all that interested in a huge back and forth but some types of behavior need to be addressed.  Now let's get back to the technical conversation.

Comment 28

[Jorg K (CEST = GMT+2)]

Thanks for putting things into perspective, bug 651120 is interesting, indeed. I've now provided a test that lets you see what went wrong. I can understand that coming back from a weekend and finding one's work undone can be irritating, but I hope we both agree that the issue needs to be addressed. Whether the fix happens in the bug which caused it or a follow-up is rather a technically, IMHO, especially if the regression is detected almost immediately. My goal was keeping the Core::Editor intact, not making things easy for Thunderbird. We have test failures all the time and we adapt our tests to ever-changing M-C without complaints (while of course we appreciate a heads-up). From my point of view, the problem introduced here was not a Thunderbird regression, but a Core::Editor regression, only that is wasn't detected due to lack of tests.

As for the technical discussion, I wasn't aware that the aim was removing the child arrays. In this case, the only way I see to maintain existing functionality would be to iterate to the node at offset-1, either from the front or the end, which appears slower than direct access. I haven't followed the analysis that led you to picking the last text child, but the test shows that under certain conditions that's not a valid choice.

Comment 29

[(no longer active)]

Attachment: Rewrite EditorBase::FindBetterInsertionPoint() to not use nsINode::GetChildAt() in the fast path used in Firefox

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee49f5bb7b20
Rewrite EditorBase::FindBetterInsertionPoint() to not use nsINode::GetChildAt() in the fast path used in Firefox; r=masayuki

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ee49f5bb7b20

Comment 32

[Jorg K (CEST = GMT+2)]

After that landed, or bug 1404106, we're getting editor related test failures in TB Mozmill, see bug 1404581.

Our test basically prepares a draft message with this body, so some leading spaces:
NoSpace
 OneSpace
  TwoSpaces

When we load the message back into the editor (edit draft), we get
NoSpace OneSpace
  TwoSpaces
so one line break is lost. I haven't analysed the details yet.

I'm still not clear why

-    if (offset > 0 && node->GetChildAt(offset - 1) &&
-        node->GetChildAt(offset - 1)->IsNodeOfType(nsINode::eTEXT)) {
-      NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
-      aNode = node->GetChildAt(offset - 1);
-      aOffset = static_cast<int32_t>(aNode->Length());
-      return;
+    if (offset) {
+      if (offset == static_cast<int32_t>(node->GetChildCount())) {
+        // If offset points to the last child, use a fast path that avoids calling
+        // GetChildAt() which may perform a linear search.
+        nsIContent* child = node->GetLastChild();
+        while (child) {
+          if (child->IsNodeOfType(nsINode::eTEXT)) {
+            NS_ENSURE_TRUE_VOID(node->Length() <= INT32_MAX);
+            aNode = child;
+            aOffset = static_cast<int32_t>(aNode->Length());
+            return;
+          }
+          child = child->GetPreviousSibling();
+        }
+      } else {

doesn't constitute a change in behaviour. The previous code checked exactly one node, now you loop backwards.

So it you're presented with
text<br>
and offset==2, so *after* the last child, you will insert after the text instead of the <br>, no?

Is that comment correct? 
+      if (offset == static_cast<int32_t>(node->GetChildCount())) {
+        // If offset points to the last child, use a fast path that avoids calling
The offset doesn't point to the last child, does it? It points after the last child.

Looks like I have to write another test that simulates that.

Comment 33

[Jorg K (CEST = GMT+2)]

Confirmed via local backout that this bug caused some undesired editor changes which we detect in our Mozmill suite.

Comment 34

[(no longer active)]

Yes, the loop does change the behavior here and that wasn't my preferred approach, see comment 6.

I still think the loop is pointless, but I don't see any way to both keep the loop and keep the Thunderbird code working.  To be honest in comment 7 I wasn't thinking of this weird Thunderbird "plaintext editor but really HTML under the hood" mode that we have been supporting.  I think if we choose to continue to support that, the only possible solution here is to scan the last child.  Dropping support for it and having Thunderbird rewrite their code on top of the HTML editor is another option but that feels like too heavy of a hammer here.

Comment 35

[Jorg K (CEST = GMT+2)]

Thanks for the reply, can you enlighten my a little further.

"I still think the loop is pointless, but I don't see any way to both keep the loop and keep the Thunderbird code working."

So you landed code that you thought was pointless? How so? Am I misunderstanding? Please don't refer to Core::Editor code as Thunderbird code. Thunderbird is using a published interface whose behaviour has just changed (again). Text or mail editor
https://hg.mozilla.org/mozilla-central/rev/903bd3c9bfdd#l2.27
is still a supported type. All this codes dates back to the Netscape Suite days, and yes, some of it was implemented for mail purposes. Yet, it's in the ownership of Core now so it would be nice if Core people maintained it in working order (for its original purpose). I have suggested a few times to cut all code not used by Firefox, like some serialisers or Necko's mozTXTToHTMLConv(!!) but found it was all intertwined with Core. I think I tried to remove mozTXTToHTMLConv from Core one day and found that there was one use for it somewhere in Necko. But I digress.

On a more positive note, I like your description of weird [Core::Editor] "plaintext editor but really HTML under the hood" ;-) That's what I have to tell Thunderbird users who really think there is plaintext. BTW, my ThunderHTMLedit HTML editor in a tab in Thunderbird also works for so-called plaintext e-mail to educate users.

Altogether I don't understand the patch. I thought the aim was to eliminate the child storage (bug 651120) and therefore GetChildAt(). But that's still used in your patch. Or maybe GetChildAt() will be implemented with iteration instead of direct access, so uses will be more expensive and should be avoided.

So what's the way forward here?

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that when the editor is an HTML editor (AsHTMLEditor() returns non-nullptr), such optimization should fallback to the slow path since such optimization assumes specific DOM tree created in <input>/<textarea>. I don't think that we need to take care the performance of Tb only editor mode at least for now.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> That's what I have to tell Thunderbird users who really think there is plaintext.

At least in Japanese market, plaintext mode is important because a lot of Japanese users still keep using plaintext emails (I believe).

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #35)
> So what's the way forward here?
Would you accept a patch with conditional compilation? 

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #36)
> I think that when the editor is an HTML editor (AsHTMLEditor() returns
> non-nullptr), such optimization should fallback to the slow path since such
> optimization assumes specific DOM tree created in <input>/<textarea>. I
> don't think that we need to take care the performance of Tb only editor mode
> at least for now.
Surely TB doesn't need any optimisation. I don't quite understand the comment, since this code only runs for non-HTML-editors:
https://dxr.mozilla.org/mozilla-central/rev/41286177c59c74ec37961b1edea34cc90d6f6dc5/editor/libeditor/EditorBase.cpp#2407
Or does <input>/<textarea> have it's own little plaintext editor embedded? In any case, typing happens at user speed, so I don't think optimisation will be noticed. I thought this was for the higher aim of removing child storage.

Off-topic: Yes, apparently many people in Japan use ISO-2022-JP and plaintext, at least that's what I was told when I fixed CJK e-mail composition a while back (which was inserting spurious spaces and corrupting Unicode characters).

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+2) from comment #38)
> (In reply to Jorg K (GMT+2) from comment #35)
> > So what's the way forward here?
> Would you accept a patch with conditional compilation? 
> 
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #36)
> > I think that when the editor is an HTML editor (AsHTMLEditor() returns
> > non-nullptr), such optimization should fallback to the slow path since such
> > optimization assumes specific DOM tree created in <input>/<textarea>. I
> > don't think that we need to take care the performance of Tb only editor mode
> > at least for now.
> Surely TB doesn't need any optimisation. I don't quite understand the
> comment, since this code only runs for non-HTML-editors:
> https://dxr.mozilla.org/mozilla-central/rev/
> 41286177c59c74ec37961b1edea34cc90d6f6dc5/editor/libeditor/EditorBase.cpp#2407

It returns true when it does not use HTMLEditRules. So, it returns true when the editor is HTMLEditor but in the plaintext mode. That's the case when the patch works buggy.

Comment 40

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #35)
> Thanks for the reply, can you enlighten my a little further.
> 
> "I still think the loop is pointless, but I don't see any way to both keep
> the loop and keep the Thunderbird code working."
> 
> So you landed code that you thought was pointless? How so? Am I
> misunderstanding?

Clearly you haven't read the bug, even though you take the time to post long comments here!  If you take the time to read the bug now from the beginning you'll see that my original suggested approach was different but Masayuki didn't like it and I couldn't convince him.

> Please don't refer to Core::Editor code as Thunderbird
> code. Thunderbird is using a published interface whose behaviour has just
> changed (again).

eEditorPlaintextMask *is* Thunderbird code, it just lives in m-c.

> Text or mail editor
> https://hg.mozilla.org/mozilla-central/rev/903bd3c9bfdd#l2.27
> is still a supported type. All this codes dates back to the Netscape Suite
> days, and yes, some of it was implemented for mail purposes. Yet, it's in
> the ownership of Core now so it would be nice if Core people maintained it
> in working order (for its original purpose).

I'm gonna ignore your entitled tone here as I don't want to get into another unhelpful back and forth.

But in the real world of software development, if Thunderbird has code in m-c that it wants to rely on, the right way to do that is by *submitting automated tests that run in m-c*.  Anything else (that includes complaints such as this one) is just asking for continued breakage like what we're seeing here.  (Note, I'm trying to show you an effective way to keep Thunderbird working in the face of changes to m-c, up to you how you want to take this.)

> I have suggested a few times to
> cut all code not used by Firefox, like some serialisers or Necko's
> mozTXTToHTMLConv(!!) but found it was all intertwined with Core. I think I
> tried to remove mozTXTToHTMLConv from Core one day and found that there was
> one use for it somewhere in Necko. But I digress.

eEditorPlaintextMask isn't used anywhere else but in mailnews, for the record.

> On a more positive note, I like your description of weird [Core::Editor]
> "plaintext editor but really HTML under the hood" ;-) That's what I have to
> tell Thunderbird users who really think there is plaintext. BTW, my
> ThunderHTMLedit HTML editor in a tab in Thunderbird also works for so-called
> plaintext e-mail to educate users.

I understand the importance of plaintext email, thank you very much.  The snark in your response was uncalled for.

What I'm referring to is the eEditorPlaintextMask flag, which creates an HTMLEditor which returns true from IsPlaintextEditor(), which is a unique situation that almost none of the tests in m-c ever test for.  This is the mode in which the plaintext composer in Thunderbird operates.

> Altogether I don't understand the patch. I thought the aim was to eliminate
> the child storage (bug 651120) and therefore GetChildAt(). But that's still
> used in your patch. Or maybe GetChildAt() will be implemented with iteration
> instead of direct access, so uses will be more expensive and should be
> avoided.

Post bug 651120, GetChildAt() will be a linear scan of a linked list which will run much slower than it does currently (it currently offsets into an array so it's O(1)).

Comment 41

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #36)
> I think that when the editor is an HTML editor (AsHTMLEditor() returns
> non-nullptr), such optimization should fallback to the slow path since such
> optimization assumes specific DOM tree created in <input>/<textarea>. I
> don't think that we need to take care the performance of Tb only editor mode
> at least for now.

I posted an untested patch that does this in bug 1404581.

Comment 42

[Jorg K (CEST = GMT+2)]

Hi Ehsan, first of all, thank you for the patch you posted in the other bug, I'll try it now.

In reply to your comment #40: I'm really sorry that my comment #35 has caused some friction since I intended to be nice. Yes, clearly I haven't read the bug, in fact I tried, but I didn't even understand the terminology (<scrollbar>, <scrollconer> and <resizer>, anon-<div>, XBL bindings). You know me as contributor for many years, mainly dictionary, some editor, some drag&drop (with Neil Deakin), but the stuff you're discussing here is beyond me.

I didn't to lecture you about the importance of plaintext mail and I can't see any snark remark, at least none was intended, and I liked your description of the "plaintext" editor with HTML under the hood.

What surprises me is that you consider parts of M-C to be Thunderbird code. Yes, it was originally written for the mail part of the old Netscape Suite, but it is now under the custodianship of M-C. The Thunderbird team don't have peerage, ownership and possibly not even knowledge there.

You are right, we should submit tests to guarantee this code stays working and I have done so in bug 1397412 and previously in bug 1330796. Do you have anything special in mind? I'm happy to contribute a test.

I understand now that that mailnews is the only piece of code that sets up an HTML editor with that flags, whereas M-C uses the flag for text areas and single line fields
(http://searchfox.org/mozilla-central/source/dom/html/nsTextEditorState.cpp#1431).

Comment 43

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #42)
> Hi Ehsan, first of all, thank you for the patch you posted in the other bug,
> I'll try it now.

No problem, thank you.

> In reply to your comment #40: I'm really sorry that my comment #35 has
> caused some friction since I intended to be nice. Yes, clearly I haven't
> read the bug, in fact I tried, but I didn't even understand the terminology
> (<scrollbar>, <scrollconer> and <resizer>, anon-<div>, XBL bindings). You
> know me as contributor for many years, mainly dictionary, some editor, some
> drag&drop (with Neil Deakin), but the stuff you're discussing here is beyond
> me.

You can always ask questions!  But I think even without understanding all of those details the first paragraph of comment 40 was clear.  Anyways...

> I didn't to lecture you about the importance of plaintext mail and I can't
> see any snark remark, at least none was intended, and I liked your
> description of the "plaintext" editor with HTML under the hood.
> 
> What surprises me is that you consider parts of M-C to be Thunderbird code.
> Yes, it was originally written for the mail part of the old Netscape Suite,
> but it is now under the custodianship of M-C. The Thunderbird team don't
> have peerage, ownership and possibly not even knowledge there.

Just to be clear, by "Thunderbird code" I'm making a statement of fact, not a judgement about division of work.  This code is currently not used anywhere else besides the mail composer in Thunderbird and SeaMonkey (which I'm rolling into "Thunderbird" conveniently, I suppose mailnews is the real correct term here.)  And when I say that I mean if we break this part of the editor code accidentally (as has happened twice in this bug, and many times in the past also) nothing will catch us since the code is unused in Firefox, and there's almost zero test coverage for it in m-c.

The fact that the Thunderbird team doesn't have peerage/ownership/knowledge about this code is a separate issue, which I'm not proposing to discuss.  All I'm saying is that based on the facts on the ground, we have modes of operation in the editor code that are only kept working by accident, and we have a product that relies on them working, and that's not a good combination!

> You are right, we should submit tests to guarantee this code stays working
> and I have done so in bug 1397412 and previously in bug 1330796. Do you have
> anything special in mind? I'm happy to contribute a test.

Well right now we have almost no tests, so any tests are good, but for starters I would consider porting all of the mozmill tests that currently run on c-c and examine this stuff (such as the currently failing ones) to m-c.  And from there it's a matter of how creative you want to get, but some basic tests covering at least the most basic scenarios such as typing chars, line breaks, and scripted interface access seem prudent to have as starting point.

But really, the better choice long term is for mailnews to stop using this special mode, and just use the normal HTMLEditor, and implement plaintext editing on top of that in JS.  IOW, basically only rely on the Web-exposed HTML editing functionality of the editor, since that's the functionality that's relatively well tested and is expected to continue to work well going forward into the future.  I highly doubt that there is something that mailnews needs to do which is impossible to implement on top of the normal HTMLEditor interface in pure JS, but it could be that a few things would need to be kept in core, and then you can have lots of tests for those few things.  Right now there is a vast surface to test as things stand so it's hard to know when test coverage reaches the "enough" threshold.

> I understand now that that mailnews is the only piece of code that sets up
> an HTML editor with that flags, whereas M-C uses the flag for text areas and
> single line fields
> (http://searchfox.org/mozilla-central/source/dom/html/nsTextEditorState.
> cpp#1431).

Right, that's for TextEditor, which is always a plaintext editor.  HTMLEditor is a different case, which is what I was referring to above.  See the path in nsEditingSession::SetupEditorOnWindow().

Comment 44

[Jorg K (CEST = GMT+2)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #43)
> Well right now we have almost no tests, so any tests are good, but for
> starters I would consider porting all of the mozmill tests that currently
> run on c-c and examine this stuff (such as the currently failing ones) to
> m-c.  And from there it's a matter of how creative you want to get, but some
> basic tests covering at least the most basic scenarios such as typing chars,
> line breaks, and scripted interface access seem prudent to have as starting
> point.
Hmm, that sound like (a lot of) work. You can't imagine (or maybe you can) how stretched I am keeping up with all the M-C changes. At the moment I have this editor bustage, Necko bustage and DOM bustage to look after.

I'll see what I can do. The TB project is looking into hiring more staff in the not to distant future.

Given the subsequent paragraph I'm not quoting here, it's also a tough question of how much effort we should invest here. I coincidentally wrote the failing test since we wanted to secure that the "space stuffing" using in flowed e-mail wouldn't break (IIRC). I guess it's the only test we have that actually check the content of the plaintext window after some editing. But then I get surprised every day when I find more tests I hadn't seen before ... mainly when they start failing :-(