240933 - Plaintext editor should stop using <br> all over

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The way the plaintext editor breaks up the text into line-length textnodes with
<br> nodes interspersed leads to some serious performance issues (see bug 237735).

Why do we do this instead of just keeping a single textnode (split up as needed
when editing happens and we have to create transactions for undo) and letting it
wrap due to layout wrapping it like it would any other chunk of text?  In more
detail

1) Would using such a single node be slower or faster, in general, on editor
   operations?

2) How much work would it be on the editor end to convert plaintext editor to
   such an approach?

3) What issues block doing so?  Can they be reproduced outside the context of
   editor?  Can we file them as bugs blocking this one?

Comment 1

[Kathleen :Brade]

I'm not sure which (if any) of the following bugs should be dependencies or
blockers to this bug but likely some or all have information in them which may
be useful to those who aren't already cc'd on the bugs:
 * bug 85505 (Frame code and Caret code need work to allow caret in more places)
 * bug 92921 (composer should not replace CRs by <br> inside a <pre>)
 * bug 240933 (Invisible <br> at end of cited pre confuses Enter)


Here is a tracking bug:
 * bug 108120 (mozilla 1.0 editor text widget tracking bug (eg textarea))

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that fixing this would allow the patches I posted in bug 221820 to actually
work, giving us a big footprint and perf win on pages with lots of textfields...

Could some of the editor folks actually answer my questions from comment 0? 
Especially question 3?  I guess bug 85505 is on the list?

Comment 3

[Zook Valem]

Boris, if I'm wrong with bug #205946 and #255166 then I apologize and feel free
to correct it since this fit the summary.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Those might be related in the end (not sure enough on editor guts to say), but
note that this bug is about the _plaintext_ editor, not the HTML editor (which
is what those bugs are about).

Comment 5

[Zook Valem]

Ah!  Didn't noticed the "plaintext" the first time.  I remember seeing Daniel
Glazman's blog that mentioned about problems with the insertion of the <br> tag
in NVU.

>>Those might be related in the end (not sure enough on editor guts to say), ...
Me too, we can still take them out once we know enough about it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This seems to be the major contributor to some of our remaining textarea perf
issues...

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

In fact, if \n is used instead of <br> in nsPlainTexteditor for line breaks, the
perf are far worse...

Comment 8

[Boris Zbarsky [:bzbarsky]]

The perf where?  In reflow, or in changing the value in the editor?  See bug
190147 for an example where the <br> thing makes things a good bit slower.

If you have profile data or testcases showing that \n is slower, I'd love to see it.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #8)

> If you have profile data or testcases showing that \n is slower, I'd love to
see it.

Make a VERY long HTML document consisting of a chain of span elements only
interrupted by \n. Open it in the editor and type a char.
Take the same document with s/\n/<br>/g and edit it, type a char, see the
difference.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, the editing perf.  If that's happening when the span is -moz-pre-wrap then
we just need to fix that issue (know not to dirty the further lines, which is
all <br> gives us).  Note that the -moz-pre-wrap is key, since we're talking
about plaintext editing here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I files bug 310087 on the perf stuff.

Comment 12

[Zook Valem]

How would this affect some other "<br>" bugs that had some hacking as a 
workaround to their bugs?  Like hiding the "<br>" or something for example.

Comment 13

[Gérard Talbot]

*** Bug 334258 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

So I took a look at this, and there are some performance issues with editing large textareas.  Bug 367177 should help with that some, and lay the groundwork for helping more.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Sort of start on this

This is the patch comment 14 is about.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

So what is missing here? The patch does still apply cleanly to trunk.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I think we need a fix for bug 485446 before we can take this patch.  We also need more changes to eliminate <br>; this patch just does it for the initial value set, not for editing.

You can test by applying this patch, loading the editing view for a wikipedia article (esp. one with lots of spellcheck selections in it) and trying to type.  When I tried that back in 2007, it was a _lot_ slower with the patch than without, both to load and to edit.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems to me things would work nicely if the content for a textarea looked like:
-- <textarea>
  -- anonymous <div> (styled with display:pre-wrap etc)
    -- text node containing the entire textarea value, verbatim
    -- a single <br> node, always
where the text node is the editable content, not the <div>, so the selection is always constrained to the text node.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Can editor handle a root content that's not an element?  If not, would it make sense to do:

  <textarea>
    anonymous <div>
      anonymous <span>, editable root
        text
      <br>

or some such?  It's more memory due to all of the span's continuations, I guess, and might suck on reflow for the same reason....

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #19)
> Can editor handle a root content that's not an element?  If not, would it make
> sense to do:
> 
>   <textarea>
>     anonymous <div>
>       anonymous <span>, editable root
>         text
>       <br>
> 
> or some such?  It's more memory due to all of the span's continuations, I
> guess, and might suck on reflow for the same reason....

Yeah. Making sure that editor can handle root content that's a text node shouldn't be too hard, I imagine.

It would be nice if we could get rid of the anonymous <div> by making <textarea> be display:inline-block, but I guess we need people to be able to do things like <textarea style="display:inline"> and <textarea style="display:block"> and get something reasonable to happen.

Comment 21

[(no longer active)]

Attachment: WIP

bz's patch, unbitrotted.

Comment 22

[(no longer active)]

Attachment: Part 2: update the a11y test

This patch updates the accessibility test to make sure that a11y only expects two children, one text node and one blank (br) node.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 456801 [details] [diff] [review]
Part 2: update the a11y test

fine with me, br is exposed as '\n' by accessible text interfaces, so everything must be ok.

Comment 24

[(no longer active)]

So, the first problem with the WIP 1 patch is a failure in <http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug288789.html?force=1>.  It seems that when we create a text node containing line feed characters (like "\n\u05d0a\u05d1\n\n", we'll end up with a frames like this:

                        line 0x25286f90: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0xa100] {9441,0,0,840} <
                          Text(0)@0x25286f48[0,1,F]  next=0x252875e0 next-continuation=0x252875e0 {9441,0,0,840} [state=00000000c0624000] [content=0x208b8b10] sc=0x1eb19b0 pst=:-moz-non-element<
                            "\n"
                          >
                        >
                        line 0x25287630: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x1a100] {8112,840,1329,900} ca={60,840,9381,900} <
                          Text(0)@0x252875e0[1,1,F]  next=0x252874e8 prev-continuation=0x25286f48 next-continuation=0x252874e8 {9002,840,439,900} [state=0000000080224004] [content=0x208b8b10] sc=0x1eb19b0 pst=:-moz-non-element<
                            "\u05d0"
                          >
                          Text(0)@0x252874e8[2,1,F]  next=0x25287538 prev-continuation=0x252875e0 next-continuation=0x25287538 {8534,840,468,840} [state=00000000c0024000] [content=0x208b8b10] sc=0x1eb19b0 pst=:-moz-non-element<
                            "a"
                          >
                          Text(0)@0x25287538[3,2,F]  next=0x25287658 prev-continuation=0x252874e8 next-continuation=0x25287658 {8112,840,422,900} [state=00000000c0424000] [content=0x208b8b10] sc=0x1eb19b0 pst=:-moz-non-element<
                            "\u05d1\n"
                          >
                        >
                        line 0x252876a8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0xa100] {9441,1740,0,900} <
                          Text(0)@0x25287658[5,1,T]  next=0x25286ed0 prev-continuation=0x25287538 {9441,1740,0,900} [state=0000000080624004] [content=0x208b8b10] sc=0x1eb19b0 pst=:-moz-non-element<
                            "\n"
                          >
                        >

Which I think confuses our caret navigation code.  Are those frames the expected frame tree?  I'm not sure if this is a bug in our text frame generation code, or caret movement code, but it seems to me that the "\n" chars in the text frames generated should be "" because we have line frames there, which indicate, well, a line break...


This bug can be seen in reality by loading the test case, focusing the textarea, pressing Ctrl+Home, and pressing Left.  Left should take the caret to the beginning (right-most position) on the second line, while it just moves the caret to the beginning of the "\n" in the first text frame.  Pressing Left once again will make the caret pass that characters, and make its way to the beginning of the second line.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, the '\n' characters there are expected. You should expect every character in the DOM to be assigned to some text frame.

Comment 26

[Boris Zbarsky [:bzbarsky]]

The "line" thing in frame dumps is not a line frame; it's a line box (in the CSS sense).  The frames in the line box have the nsBlockFrame it's a line for as their parent.

So this sounds like a caret navigation bug (and one that should be reproducible when using "browse with caret" inside a <pre>).

Comment 27

[(no longer active)]

Attachment: Part 3: Fix caret movement on preformatted textframes without brFrame's

(In reply to comment #26)
> So this sounds like a caret navigation bug (and one that should be reproducible
> when using "browse with caret" inside a <pre>).

This is exactly right.  I finally have a fix which I'm pretty happy with.  I also augmented it with a full caret movement test.  I've pushed this to try server to make sure that this fix doesn't break any other code.

Comment 28

[(no longer active)]

Attachment: Part 3: Fix caret movement on preformatted textframes without brFrame's

Fixed a small bug in the previous version.

Comment 29

[(no longer active)]

Comment on attachment 457211 [details] [diff] [review]
Part 3: Fix caret movement on preformatted textframes without brFrame's

The try server run for this patch resulted in a bunch of failures in test_movement_by_characters.html and test_backspace_delete.html.  I'll look into those more closely tomorrow.

Comment 30

[(no longer active)]

So, I think that the parts of the test_movement_by_characters.html added in bug 394752 are actually wrong.

Consider this test case:

data:text/html,<div contenteditable=true></div><script>document.querySelector("div").innerHTML="<pre>aa\nbb</pre>";</script>

If you put the caret right before the first "a", and press Right twice, the caret would end up *after* the \n character, but would still appear on the first line.  So, now if you enter "d" for example, the "d" character would end up at the beginning of the second line ("aa\ndbb"), while the user most definitely expects it to end up at the end of the first line ("aad\nbb").  The same thing happens when moving backwards using the Left key.

My patch fixes this problem, so I'm adjusting the test to actually test the right thing.

Comment 31

[(no longer active)]

Attachment: Part 3: Fix caret movement on preformatted textframes without brFrame's

So, this patch fixes the bugs which caused the Thai caret positioning to fail.  They were all things which I forgot to include from the original file.  It also modifies the other test as I explained in comment 30.

Comment 32

[(no longer active)]

Attachment: Part 1: Do not split multiline text into textframes separated by BR elements

This patch fixes a bug in bz's original patch.  That patch adjusted the default selection for a textarea to the last character in the textarea.  But if that last character is a linefeed, we need to adjust the selection to the bogus BR added after it.  In this patch, I add that BR element if needed and adjust the selection to be collapsed over that element.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Why is the |outString->Assign(tString);| in that !pre branch still needed?  (For that matter, we probably don't even need the PromiseFlatString() if we replace .get() with BeginReading().)

Also, as I recall this patch didn't change what gets inserted when the user hits enter; not sure whether it changes what happens on paste.

Comment 34

[(no longer active)]

(In reply to comment #33)
> Why is the |outString->Assign(tString);| in that !pre branch still needed? 
> (For that matter, we probably don't even need the PromiseFlatString() if we
> replace .get() with BeginReading().)

I'm not still sure in which case that branch is actually useful.  nsHTMLEditRules has its own implementation of WillInsertText (which doesn't propagate anything to the base class), and plain text controls always use the pre-formatted white-space model <http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#145>.  I suspect that code path can just be removed, but I'm not sure.  Now that you've asked, I'll just write a patch which removes that code and submit it to the try server to see what happens!  ;-)

> Also, as I recall this patch didn't change what gets inserted when the user
> hits enter; not sure whether it changes what happens on paste.

It takes care of paste, because the paste operation just consists of a huge InsertText call.  But yes, handling line break insertions is still remaining.  Other than that, there might still be other bugs in the existing parts, and I should also look at the perf issues here.  In other words, this is nowhere near completion yet!  :-)

Comment 35

[(no longer active)]

Attachment: Part 4: Remove non-preformatted text handling code

See comment 34.  Submitted this patch to the try server.

Comment 36

[(no longer active)]

Comment on attachment 457480 [details] [diff] [review]
Part 4: Remove non-preformatted text handling code

So, this patch resulted in a single test failure on the try server in editor/libeditor/text/tests/test_texteditor_keyevent_handling.html.  The failure was on a test which was generating a VK_TAB keypress on a non-tabbable textarea to make sure that 4 spaces are appended to its value.  This made me curious, and I looked into this more deeply.  It turns out that if the DOM node passed to nsEditor::IsPreformatted is not an element (for example, if it's a textnode), it always returns false.  The code was passing the selection node to IsPreformatted.  If the textarea is empty, the selection node is the anonymous DIV which has the |white-space: pre| style set on it.  Otherwise, the selection node is a text node, which causes IsPreformatted to return false.  So, effectively, the isPre check could have been renamed to !isEmpty.  Which means that textareas take two different code paths when typing some text into them based on whether they're empty or not!

To see how broken this behavior is, load data:text/html,<textarea></textarea>, copy a tab character to the clipboard, and paste it twice.  You can inspect that the first time you paste the tab character, it will be pasted intact, and the second time it will be converted to four spaces!

Now, we have the issue of whether we should convert tabs to spaces.  The HTML5 spec doesn't requires us to do so, and neither Webkit nor Opera do this.  I think we should just stop converting tabs into spaces.  Bug 577240 is filed for this.

I'll submit a revised version of this patch which corrects the failing test and also adds another test to make sure that the behavior is the same with empty textareas.

Comment 37

[(no longer active)]

Attachment: Part 4: Remove non-preformatted text handling code

Patch based on comment 36.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      // If we're dealing with a text frame and moving backward positions us at
+      // the end of that line, decrease the offset by one to make sure that
+      // we're placed before the linefeed character on the previous line.
+      if (offset < 0 && jumpedLine &&
+          aPos->mDirection == eDirPrevious &&
+          current->GetType() == nsGkAtoms::textFrame &&
+          current->GetStyleText()->NewlineIsSignificant() &&
+          current->HasTerminalNewline()) {
+        --aPos->mContentOffset;
+      }

We don't need to check GetType(), HasTerminalNewline only returns true on text frames.

But I don't understand why we're doing this here. I presume this is for a case where the caret is at the beginning of a line, we press left-arrow, and the caret hint changes but the offset does not. It seems to me that GetFrameFromDirection should be fixed instead to skip terminal preformatted newlines so that we do get an offset change in this case.

Comment 39

[(no longer active)]

Attachment: Part 3.1: Position the selection before the terminating newline when clicking to the right of the line

Without this patch, when you have a text area with some content like this: "abc\ndef" and you click on the first line towards the right of "c", the selection is put after the linefeed, so whatever you type will end up at the beginning of the second line.

Comment 40

[(no longer active)]

Attachment: Part 3: Fix caret movement on preformatted textframes without brFrame's

(In reply to comment #38)
> +      // If we're dealing with a text frame and moving backward positions us
> at
> +      // the end of that line, decrease the offset by one to make sure that
> +      // we're placed before the linefeed character on the previous line.
> +      if (offset < 0 && jumpedLine &&
> +          aPos->mDirection == eDirPrevious &&
> +          current->GetType() == nsGkAtoms::textFrame &&
> +          current->GetStyleText()->NewlineIsSignificant() &&
> +          current->HasTerminalNewline()) {
> +        --aPos->mContentOffset;
> +      }
> 
> We don't need to check GetType(), HasTerminalNewline only returns true on text
> frames.

Done.  This new patch removes that line.

> But I don't understand why we're doing this here. I presume this is for a case
> where the caret is at the beginning of a line, we press left-arrow, and the
> caret hint changes but the offset does not. It seems to me that
> GetFrameFromDirection should be fixed instead to skip terminal preformatted
> newlines so that we do get an offset change in this case.

Oh, I should have probably read this comment more closely before trying to come up with another way to fix this.  So, what you said is not actually true, because we do want to change the offset in that case.  Let's consider this case: "abc\n|def".  Here the offset is 4.  If you press left-arrow, we change the offset to 3, to make sure that the selection ends up *before* the linefeed.  Basically this is an invariant that this patch (and attachment 457979 [details] [diff] [review]) try to ensure: a selection never appears after a linefeed character.  And this is more than just about caret positioning, basically without this invariant, we should rework the entire modification operations in the editor, because they just blindly try to operate at the insertion point.

Does this address your concerns?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #40)
> Oh, I should have probably read this comment more closely before trying to
> come up with another way to fix this.  So, what you said is not actually true,
> because we do want to change the offset in that case.

I understand that. My main concern is about *where* in the code we adjust the offset.

> Basically this is an invariant that this patch (and attachment 457979 [details] [diff] [review]
> [details]) try to ensure: a selection never appears after a linefeed
> character.  And this is more than just about caret positioning, basically
> without this invariant, we should rework the entire modification operations in
> the editor, because they just blindly try to operate at the insertion point.

In your example, it's quite OK for the caret to be at offset 4, as long as the caret position hint is "hint right"; then we're on the second line and inserting text will insert text at the start of the second line, which is what we want.

So more precisely, the invariant we need is that the caret content and offset should not be after a preformatted newline with a "hint left" hint.

+        // If we're at the end of a line, decrease offset by two, otherwise we'll
+        // end up after the linefeed character
+        if (GetStyleText()->NewlineIsSignificant() &&
+            mTextRun->GetChar(iter.GetSkippedOffset()) == '\n') {
+          --(*aOffset);

I don't understand this. aOffset is the offset of the \n already, i.e. "before" the \n. That is surely what we want to return?

What I want to see is a patch that changes our behaviour in the code where we currently detect we're at the start of the line and decide to not adjust the offset and just switch from "hint right" to "hint left". Instead of doing that, we should be bumping the offset like we normally would.

Comment 42

[(no longer active)]

Attachment: Part 5: Inject linefeed characters instead of BR elements when the user presses Enter

With this patch, pressing Enter on a textarea injects a newline character instead of a BR node.

Comment 43

[(no longer active)]

Attachment: Part 6: Put the caret on the next line's BR element or text node if we have a newline at the end of a textframe and a BR on the next line

So, we have this case which we should handle correctly where we have a text frame with a line break at the end, and either a text frame or a BR frame on the next line.  This patch fixes AdjustCaretFrameForLineEnd to take that case into consideration as well.  There might be an easier way to do what I did (using better APIs to get the "next" frame on the next line), but this is the best I could come up with.

Comment 44

[(no longer active)]

Attachment: Part 7: Put the selection on the moz BR element if the user presses Enter at the end of a textarea

This changes the existing logic about putting the selection on the moz BR node to make it work with the new no-BR world.  The only practical effect of this patch is to take care of scrolling when you press Enter at the end of a text area in a case where you'll be adding the N+1 line where the textarea has rows=N.

Comment 45

[(no longer active)]

Attachment: Part 8: Avoid creating multiple textnodes when adding text to textareas

The existing editor text insertion code would create an additional text node in some cases where the selection root is on the anonymous DIV.  This causes all sorts of weird problems in the no-BR world, and this patch manually adjusts things so that the selection is put on a text node if one exists before attempting to insert text into the textarea.

Comment 46

[(no longer active)]

(In reply to comment #41)
> > Basically this is an invariant that this patch (and attachment 457979 [details] [diff] [review]
> > [details]) try to ensure: a selection never appears after a linefeed
> > character.  And this is more than just about caret positioning, basically
> > without this invariant, we should rework the entire modification operations in
> > the editor, because they just blindly try to operate at the insertion point.
> 
> In your example, it's quite OK for the caret to be at offset 4, as long as the
> caret position hint is "hint right"; then we're on the second line and
> inserting text will insert text at the start of the second line, which is what
> we want.

It's OK for the caret to be at offset 4, sure.  But I'm talking about the selection.

> So more precisely, the invariant we need is that the caret content and offset
> should not be after a preformatted newline with a "hint left" hint.

Well, with this patch, that invariant is already satisfied.  But that's not really the main issue here.  We need to make sure that the selection is collapsed to before the linefeed character.

> +        // If we're at the end of a line, decrease offset by two, otherwise
> we'll
> +        // end up after the linefeed character
> +        if (GetStyleText()->NewlineIsSignificant() &&
> +            mTextRun->GetChar(iter.GetSkippedOffset()) == '\n') {
> +          --(*aOffset);
> 
> I don't understand this. aOffset is the offset of the \n already, i.e. "before"
> the \n. That is surely what we want to return?

No.  At this point, aOffset would be 4 in my example.  We need to decrement it to make sure that it's set to the position before the linefeed character.

> What I want to see is a patch that changes our behaviour in the code where we
> currently detect we're at the start of the line and decide to not adjust the
> offset and just switch from "hint right" to "hint left". Instead of doing that,
> we should be bumping the offset like we normally would.

Such an approach would only take care of the caret positioning side of things, but the selection would be collapsed to after the linefeed character, which would give us all sorts of problems as I mentioned in comment 40.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #46)
> > +        // If we're at the end of a line, decrease offset by two, otherwise
> > we'll
> > +        // end up after the linefeed character
> > +        if (GetStyleText()->NewlineIsSignificant() &&
> > +            mTextRun->GetChar(iter.GetSkippedOffset()) == '\n') {
> > +          --(*aOffset);
> > 
> > I don't understand this. aOffset is the offset of the \n already, i.e. "before"
> > the \n. That is surely what we want to return?
> 
> No.  At this point, aOffset would be 4 in my example.  We need to decrement it
> to make sure that it's set to the position before the linefeed character.

No, it would be at offset 3. The character at offset 3 is \n, the character at offset 4 is 'd'.

> > What I want to see is a patch that changes our behaviour in the code where we
> > currently detect we're at the start of the line and decide to not adjust the
> > offset and just switch from "hint right" to "hint left". Instead of doing that,
> > we should be bumping the offset like we normally would.
> 
> Such an approach would only take care of the caret positioning side of things,
> but the selection would be collapsed to after the linefeed character, which
> would give us all sorts of problems as I mentioned in comment 40.

If the selection is at offset 4 with "hint right", then the caret displays at the start of the second line. If you press left-arrow and we bump the offset by -1 instead of just changing the hint, we'll end up at offset 3, the caret will display in the right place and inserted text will go before the \n. What's the problem?

Comment 48

[(no longer active)]

Attachment: Part 3: Fix caret movement on preformatted textframes without brFrame's

(In reply to comment #47)
> (In reply to comment #46)
> > > +        // If we're at the end of a line, decrease offset by two, otherwise
> > > we'll
> > > +        // end up after the linefeed character
> > > +        if (GetStyleText()->NewlineIsSignificant() &&
> > > +            mTextRun->GetChar(iter.GetSkippedOffset()) == '\n') {
> > > +          --(*aOffset);
> > > 
> > > I don't understand this. aOffset is the offset of the \n already, i.e. "before"
> > > the \n. That is surely what we want to return?
> > 
> > No.  At this point, aOffset would be 4 in my example.  We need to decrement it
> > to make sure that it's set to the position before the linefeed character.
> 
> No, it would be at offset 3. The character at offset 3 is \n, the character at
> offset 4 is 'd'.

Hmm, you're right.  In fact, with attachment 457979 [details] [diff] [review], this code is no longer needed, so I removed it from the patch.

> > > What I want to see is a patch that changes our behaviour in the code where we
> > > currently detect we're at the start of the line and decide to not adjust the
> > > offset and just switch from "hint right" to "hint left". Instead of doing that,
> > > we should be bumping the offset like we normally would.
> > 
> > Such an approach would only take care of the caret positioning side of things,
> > but the selection would be collapsed to after the linefeed character, which
> > would give us all sorts of problems as I mentioned in comment 40.
> 
> If the selection is at offset 4 with "hint right", then the caret displays at
> the start of the second line. If you press left-arrow and we bump the offset by
> -1 instead of just changing the hint, we'll end up at offset 3, the caret will
> display in the right place and inserted text will go before the \n. What's the
> problem?

That's exactly what this patch does, except for the hint, which is set to "hint right", but since we're not at a frame boundary, it doesn't matter (the HINTRIGHT default is coming from <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1237>.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #48)
> That's exactly what this patch does, except for the hint, which is set to "hint
> right", but since we're not at a frame boundary, it doesn't matter (the
> HINTRIGHT default is coming from
> <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1237>.

Right, but I'm unconvinced this is in the right place. Where does the current code decide to switch from HINTRIGHT to HINTLEFT? Can we put your code there instead? Why isn't there corresponding code to handle an eDirNext case? Why are we checking offset < 0 *and* aPos->mDirection?

Comment 50

[(no longer active)]

Attachment: Part 9: Simplify text control frame's DOM point to offset conversion code

Comment 51

[(no longer active)]

(In reply to comment #49)
> Right, but I'm unconvinced this is in the right place. Where does the current
> code decide to switch from HINTRIGHT to HINTLEFT?

<http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1237> (for left/right movement)

> Can we put your code there instead?

Not really.  A lot of the information I need isn't present there.

> Why isn't there corresponding code to handle an eDirNext case?

Because in the eDirNext case, we'll be positioned after the linefeed character, with HINTRIGHT, which means that both the caret and the selection are ending up in the correct spots.

The only remaining case (when trying to pick up the correct offset for things like clicking past the right side of a line (after its linefeed character) is handled in attachment 457979 [details] [diff] [review].

> Why are we checking offset < 0 *and* aPos->mDirection?

Negative offset is a special value meaning the end of a frame.  I'm not 100% sure if that can only happen for eDirPrevious.  If that's the case, the aPos->mDirection check can be dropped.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ok.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (iter.GetSkippedOffset() <= PRUint32(trimmed.GetEnd()) &&
+        !(GetStyleText()->NewlineIsSignificant() &&
+          mTextRun->GetChar(iter.GetSkippedOffset()) == '\n')) {

This can't be quite right. if iter.GetSkippedOffset == trimmed.GetEnd(), it might be invalid to dereference mTextRun at iter.GetSkippedOffset(), that might be after the last character in the textrun.

Comment 54

[(no longer active)]

Attachment: Part 9: Simplify text control frame's DOM point to offset conversion code

The previous version didn't handle single-line text controls correctly.  This patch fixes that problem.

Comment 55

[(no longer active)]

Attachment: Part 3: Fix caret movement on preformatted textframes without brFrame's

(In reply to comment #53)
> +    if (iter.GetSkippedOffset() <= PRUint32(trimmed.GetEnd()) &&
> +        !(GetStyleText()->NewlineIsSignificant() &&
> +          mTextRun->GetChar(iter.GetSkippedOffset()) == '\n')) {
> 
> This can't be quite right. if iter.GetSkippedOffset == trimmed.GetEnd(), it
> might be invalid to dereference mTextRun at iter.GetSkippedOffset(), that might
> be after the last character in the textrun.

True.  Fixed in this version.

Comment 56

[(no longer active)]

Attachment: Part 10: Only hide/show the caret once when placeholder transactions finish

When we load a very large textarea, and press a key and hold it, we're spending ~20% of the time in nsCaret::SetCaretVisible.  This consists of two calls to that function, each at 10%.  We can avoid having to call it twice per keypress by hiding/showing the caret from nsEditor::EndPlaceholderTransaction.

Comment 57

[(no longer active)]

Attachment: Part 11: Optimize setting the bidi flag for modifications to large DOM text nodes

Another performance bottleneck for editing large text nodes is that we spend lots of time in nsTextFragment::SetBidiFlag.  Given the way that this function works, this is totally unnecessary, I think.  We should only have to look at the new part of the buffer to see if it contains characters which should trigger bidi resolution on.

This patch implements that idea.  It shouldn't be any less accurate than the current implementation, as far as I can tell.

With this patch, we'll basically avoid spending any significant time to set the bidi flag.  It gives us another easy 10% improvement.

Comment 58

[(no longer active)]

There is another serious performance bottleneck here which I don't have an idea how to solve.  We spend around 15% of our time (after part 10 and 11) in nsTextFrame::GetChildFrameContainingOffset.  Basically, what that function does is a linear function on the text frame continuations to find the text frame which contains a given offset.  This hurts really bad when we're editing things at the end of a large textarea.  I don't think this algorithm can be improved unless we have another data structure, which would cost us memory (at least 8 bytes per text frame if we want to use a hashtable, which we would need to destroy if any of the continuations change, and 12 bytes per text frame if we want to use some sort of a tree which can be updated upon changes continuation relationships).

One thing to note here is that we call this function twice on any character entry, once for showing the caret, and once for scrolling the selection into view.  This means that a brain-dead memoization optimization could probably cut this cost into half.

While we're on the same topic, I'm not really sure why we don't cache the frame in the caret, so that we don't have to do this computation on *every* caret timer event.

Roc, bz, I'd appreciate your thoughts here.

Comment 59

[(no longer active)]

Bug 85505 doesn't seem to be relevant any more (was it ever?).  I've already taken care of the caret movement issues in this bug.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Ehsan and I just talked over comment 58 on irc (or rather I sounding-boarded).  The conclusion is that we should indeed just cache the last frame we got back from GetChildFrameContainingOffset (ideally in nsWeakFrame form, but failing that with an explicit notification if it dies).  Then we can start the search there instead of at the primary frame and hopefully win.

Comment 61

[(no longer active)]

(In reply to comment #60)
> Ehsan and I just talked over comment 58 on irc (or rather I sounding-boarded). 
> The conclusion is that we should indeed just cache the last frame we got back
> from GetChildFrameContainingOffset (ideally in nsWeakFrame form, but failing
> that with an explicit notification if it dies).  Then we can start the search
> there instead of at the primary frame and hopefully win.

I've filed bug 580869 to implement that idea.

There's another perf issue which I haven't talked about yet, and that is nsLineBox::IndexOf taking about 15% of our time, called from FindContainingAndNextLine called by nsCaret::GetChildFrameContainingOffset.  If bug 580869 proves to be useful, I'll file another bug to cache the found frame in that function as well.

Comment 62

[(no longer active)]

Having said that, I have a very good feeling that if all I talked about in comment 61 works out, all of the performance issues for landing this patch would be solved, and we can land it for the next beta!  w00t!

Comment 63

[Simon Montagu :smontagu]

Comment on attachment 459200 [details] [diff] [review]
Part 11: Optimize setting the bidi flag for modifications to large DOM text nodes

Nice!

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In part 6, I don't understand why we need to change AdjustCaretFrameForLineEnd here. Currently it always keeps the caret on the same line, we're just moving it back through trailing text frames that are invisible and irrelevant.

Comment 65

[(no longer active)]

(In reply to comment #64)
> In part 6, I don't understand why we need to change AdjustCaretFrameForLineEnd
> here. Currently it always keeps the caret on the same line, we're just moving
> it back through trailing text frames that are invisible and irrelevant.

I guess I kind of misused that function, in order to be able to use the knowledge about the case where the frame is not actually moved, and also to save us some computation on getting the next line box.  Do you have a better suggestion on where this code should live?

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you explain in more detail what part 6 is trying to do?

Would an example be abc\n<br>, where the caret node is the text node and the offset is 4?

Comment 67

[(no longer active)]

(In reply to comment #66)
> Can you explain in more detail what part 6 is trying to do?
> 
> Would an example be abc\n<br>, where the caret node is the text node and the
> offset is 4?

There are only 4 characters in the text node, so I'm not sure what this means.  The rationale behind this patch is for cases where we have \n\n somewhere in the text node.  This confuses the caret movement code when the textarea is modified (or sometimes when trying to move the caret).

Unfortunately, I've lost track of the exact failure that I was seeing when I wrote this patch, but I'm pretty sure that it had something to do with two consecutive linefeeds.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to know why part 6 is necessary. Can we just skip it for now and revive it later when you find the bug again?

Comment 69

[(no longer active)]

Comment on attachment 458467 [details] [diff] [review]
Part 6: Put the caret on the next line's BR element or text node if we have a newline at the end of a textframe and a BR on the next line

(In reply to comment #68)
> I'd like to know why part 6 is necessary. Can we just skip it for now and
> revive it later when you find the bug again?

Sure.  I'm going to reorder my patch queue so that part 6 isn't built, and will continue testing the code until I find the bug again.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In part 7, isn't this code used by HTML editing as well? Seems like the HTML editor still needs to insert <br>s sometimes.

Comment 71

[(no longer active)]

(In reply to comment #70)
> In part 7, isn't this code used by HTML editing as well? Seems like the HTML
> editor still needs to insert <br>s sometimes.

No, the HTML editor just has a dummy implementation:

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#1774

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 458473 [details] [diff] [review]
Part 7: Put the selection on the moz BR element if the user presses Enter at the end of a textarea

   // if we are at the end of the document, we need to insert 
   // a special mozBR following the normal br, and then set the
   // selection to stick to the mozBR.

Can you change this comment? since we're not doing this anymore.

Comment 73

[(no longer active)]

Attachment: Part 7: Put the selection on the moz BR element if the user presses Enter at the end of a textarea

(In reply to comment #72)
> Comment on attachment 458473 [details] [diff] [review]
> Part 7: Put the selection on the moz BR element if the user presses Enter at
> the end of a textarea
> 
>    // if we are at the end of the document, we need to insert 
>    // a special mozBR following the normal br, and then set the
>    // selection to stick to the mozBR.
> 
> Can you change this comment? since we're not doing this anymore.

Done.

Comment 74

[(no longer active)]

Attachment: Rolled up patch

Comment 75

[(no longer active)]

Attachment: Part 12: Avoid injecting textnodes as children of br nodes

I ran across another edge case where the selection would end up on the mozBR node, in which case we'd just attempt to inject a new text node under a br element, which of course doesn't work.  This patch (which is similar to part 8) adjusts the target node in that case.

Comment 76

[(no longer active)]

Attachment: Part 12: Avoid injecting textnodes as children of br nodes

With tests this time.

Comment 77

[(no longer active)]

Attachment: Part 13: Consider text frame continuations containing only newlines editable

Currently, nsEditor::IsEditable makes an incorrect assumption that zero-width text frames are not editable.  But this assumption is invalid in face of text nodes containing "\n".  This patch fixes that problem.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

       if (resultFrame->GetSize().width > 0) 
         return PR_TRUE;  // text node has width
+      if (resultFrame->HasTerminalNewline() &&
+          resultFrame->GetStyleText()->NewlineIsSignificant()) {
+        PRInt32 offset, offsetEnd;
+        resultFrame->GetOffsets(offset, offsetEnd);
+        if (offset + 1 == offsetEnd) {
+          return PR_TRUE; // text node is a significant newline
+        }
+      }

I think checking the width here is really bogus, it's entirely possible (although weird) to have zero-width characters that are editable. I think we should be checking whether the textframe contains any characters that were not colllapsed away. Basically, pass the text frame's offsets into gfxSkipCharsIterator::SetOriginalOffset and see if the corresponding GetSkippedOffsets are the same.

Comment 79

[(no longer active)]

Attachment: Part 13: Consider text frame continuations containing only newlines editable

(In reply to comment #78)
>        if (resultFrame->GetSize().width > 0) 
>          return PR_TRUE;  // text node has width
> +      if (resultFrame->HasTerminalNewline() &&
> +          resultFrame->GetStyleText()->NewlineIsSignificant()) {
> +        PRInt32 offset, offsetEnd;
> +        resultFrame->GetOffsets(offset, offsetEnd);
> +        if (offset + 1 == offsetEnd) {
> +          return PR_TRUE; // text node is a significant newline
> +        }
> +      }
> 
> I think checking the width here is really bogus, it's entirely possible
> (although weird) to have zero-width characters that are editable. I think we
> should be checking whether the textframe contains any characters that were not
> colllapsed away. Basically, pass the text frame's offsets into
> gfxSkipCharsIterator::SetOriginalOffset and see if the corresponding
> GetSkippedOffsets are the same.

Like this?

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, but that code would be better off as a method of nsTextFrame, say HasAnyNoncollapsedCharacters. I don't think we need the "if (resultFrame->GetSize().width > 0)" check at all.

Comment 81

[(no longer active)]

(In reply to comment #80)
> Yes, but that code would be better off as a method of nsTextFrame, say
> HasAnyNoncollapsedCharacters. I don't think we need the "if
> (resultFrame->GetSize().width > 0)" check at all.

Oh, I'm silly.  This doesn't work correctly.

What happens is that when a text frame has only a single newline character inside it ("\n"), calling SetOriginalOffset(offsetEnd) will try to set the original offset to 2, which is more than the char count, so we assert:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/ehsanakhgari/moz/src/gfx/thebes/gfxSkipChars.cpp, line 92

And on second thought, I don't get why comment 78 is even supposed to work.  The newline character is collapsed, right?  So, why would that check help us at all?

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In whitespace:pre-wrap/pre/pre-line, newlines are never collapsed.

(In reply to comment #81)
> (In reply to comment #80)
> > Yes, but that code would be better off as a method of nsTextFrame, say
> > HasAnyNoncollapsedCharacters. I don't think we need the "if
> > (resultFrame->GetSize().width > 0)" check at all.
> 
> Oh, I'm silly.  This doesn't work correctly.

You mean the latest version of patch #13?

> What happens is that when a text frame has only a single newline character
> inside it ("\n"), calling SetOriginalOffset(offsetEnd) will try to set the
> original offset to 2, which is more than the char count, so we assert:

A textframe containing one characters should return an offsetEnd of 1, surely?

Comment 83

[(no longer active)]

(In reply to comment #82)
> In whitespace:pre-wrap/pre/pre-line, newlines are never collapsed.
> 
> (In reply to comment #81)
> > (In reply to comment #80)
> > > Yes, but that code would be better off as a method of nsTextFrame, say
> > > HasAnyNoncollapsedCharacters. I don't think we need the "if
> > > (resultFrame->GetSize().width > 0)" check at all.
> > 
> > Oh, I'm silly.  This doesn't work correctly.
> 
> You mean the latest version of patch #13?

Yes.

> > What happens is that when a text frame has only a single newline character
> > inside it ("\n"), calling SetOriginalOffset(offsetEnd) will try to set the
> > original offset to 2, which is more than the char count, so we assert:
> 
> A textframe containing one characters should return an offsetEnd of 1, surely?

Let's say the content node has "\n\n", and we're on the text frame for the second line.  In that case, mOriginalStringToSkipCharsOffset will be 1, offset will be 0 and offsetEnd would be 1.  When I try to set the original offset to offsetEnd, the assertion is triggered.

This happens, for example, in the test case inside that patch, when pressing backspace for the first time.

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #83)
> Let's say the content node has "\n\n", and we're on the text frame for the
> second line.  In that case, mOriginalStringToSkipCharsOffset will be 1, offset
> will be 0 and offsetEnd would be 1.

No, for that second text frame, offset should be 1 and offsetEnd should be 2.

Comment 85

[(no longer active)]

That's true before the second newline character is deleted.  I think this has gotten confusing.  Here's what happens: when pressing backspace, after we delete the second newline character, nsTextEditRules::AfterEdit is called, which triggers CreateBogusNodeIfNeeded, which calls nsEditor::IsEditable on the text node.  We grab the primary frame for it, which has content offset set to 0 and offsetEnd set to 1.  skipChars.mCharCount is 1 at this point, and iter.mOriginalStringToSkipCharsOffset is also 1.  So, when we try to set offsetEnd as the iterator's original offset, SetOffsets is called with offset 2, which is greater than mSkipChars->mCharCount, hence the assertion.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds like mSkipChars is out of date? Did CharacterDataChanged and EnsureTextRun get called?

Comment 87

[(no longer active)]

(In reply to comment #86)
> Sounds like mSkipChars is out of date? Did CharacterDataChanged and
> EnsureTextRun get called?

CharacterDataChanged is called, and it marks the second text frame (the continuation of the primary frame) as needing reflow.  Then, IsEditable calls GetRenderedText, which calls EnsureTextRun.  EnsureTextRun seems to be creating a textrun object for the continuation frame (which is scheduled to be reflown), and I _think_ that this is what's going wrong.  Do you have an idea what the correct fix would look like?

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #85)
> skipChars.mCharCount is 1 at this point, and
> iter.mOriginalStringToSkipCharsOffset is also 1

The latter is wrong.

In fact, this code looks completely wrong:
      *aSkipIter = gfxSkipCharsIterator(*aSkipChars, GetContentLength());
Passing GetContentLength() there maks no sense at all. We should just be passing zero, since the skipCharsBuilder we have here is one we constructed from scratch, no special offset is required.

However, I think we shouldn't be calling GetRenderedText here at all. Like I said earlier, we should just have a method HasAnyNoncollapsedCharacters in nsTextFrame. It can call EnsureTextRun, get the gfxSkipChars, and do the analysis directly.

Comment 89

[(no longer active)]

Attachment: Part 13: Consider text frame continuations containing only newlines editable

This patch fixes the assertion.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 468927 [details] [diff] [review]
Part 13: Consider text frame continuations containing only newlines editable

Great!

Comment 91

[(no longer active)]

Just as I was patting on my own shoulder about tomorrow's landing of this bug, I discovered one more issue.  :(  More patches to come.

Comment 92

[(no longer active)]

Attachment: Part 6: Put the caret on the next line's BR element or text node if we have a newline at the end of a textframe and a BR on the next line

Finally found out the reason that I wrote part 6 in the first place.  The bug which this patch solves can be observed by focusing an empty textarea and pressing Enter once (or pressing Enter twice and Backspace once).

This patch is the same as the old version of part 6, except that it now comes with a unit test.

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. So after we press Enter once, the caret node is still the text node of course and the caret offset is 1? And since we have just one text frame, that frame remains the caret frame... hmm.

+            if (nextLine->GetChildCount() &&
+                (nextLine->mFirstChild->GetType() == nsGkAtoms::brFrame ||
+                nextLine->mFirstChild->GetType() == nsGkAtoms::textFrame)) {

Why check this? Shouldn't we move the caret to the next line no matter what's on the next line? In fact, shouldn't we assert that there *is* a next line, since if there isn't, we're going to display the caret on the current line *after* the terminal newline, which is always wrong?

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we can't ensure there is a next line, since we don't control the DOM. This is a sticky problem ... we almost want to create an extra empty line just to put the caret there. For this bug, I suggest we warn if there is no next line.

Comment 95

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #93)
> Why check this? Shouldn't we move the caret to the next line no matter what's
> on the next line? In fact, shouldn't we assert that there *is* a next line,
> since if there isn't, we're going to display the caret on the current line
> *after* the terminal newline, which is always wrong?

Could a maxlength on a textarea (bug 535043) potentially prevent Enter from moving to the next line?

Comment 96

[(no longer active)]

(In reply to comment #93)
> Hmm. So after we press Enter once, the caret node is still the text node of
> course and the caret offset is 1? And since we have just one text frame, that
> frame remains the caret frame... hmm.

Yes, exactly.

> +            if (nextLine->GetChildCount() &&
> +                (nextLine->mFirstChild->GetType() == nsGkAtoms::brFrame ||
> +                nextLine->mFirstChild->GetType() == nsGkAtoms::textFrame)) {
> 
> Why check this? Shouldn't we move the caret to the next line no matter what's
> on the next line?

Should we?  Please note that this same code is responsible for caret movement on caret browsing as well, so how can we actually be sure that the frame on the next line is either a BR frame or a text frame?

> In fact, shouldn't we assert that there *is* a next line,
> since if there isn't, we're going to display the caret on the current line
> *after* the terminal newline, which is always wrong?

We're checking nextLine and nextLine->GetChildCount().  Do you mean something more than that?

Comment 97

[(no longer active)]

(In reply to comment #94)
> Maybe we can't ensure there is a next line, since we don't control the DOM.
> This is a sticky problem ... we almost want to create an extra empty line just
> to put the caret there.

That's the sole purpose of the terminating bogus BR node, right?  (Well, that, and the fact that all of the editor code relies on its existence, and taking it out would be a huge project!)

> For this bug, I suggest we warn if there is no next line.

Hmm, like an NS_WARNING?  I don't think that's going to be useful.  And there *are* going to be cases where such a warning would be triggered (although they may be far fetched), like:

data:text/html,x<span style="white-space:pre">&#10;</span><span style="white-space:pre">&#10;</span>y

with caret browsing turned on.

Comment 98

[(no longer active)]

(In reply to comment #95)
> (In reply to comment #93)
> > Why check this? Shouldn't we move the caret to the next line no matter what's
> > on the next line? In fact, shouldn't we assert that there *is* a next line,
> > since if there isn't, we're going to display the caret on the current line
> > *after* the terminal newline, which is always wrong?
> 
> Could a maxlength on a textarea (bug 535043) potentially prevent Enter from
> moving to the next line?

No, but it should (and it's not a regression from this bug).  Filed bug 590554 for that.

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #97)
> (In reply to comment #94)
> > Maybe we can't ensure there is a next line, since we don't control the DOM.
> > This is a sticky problem ... we almost want to create an extra empty line just
> > to put the caret there.
> 
> That's the sole purpose of the terminating bogus BR node, right?  (Well, that,
> and the fact that all of the editor code relies on its existence, and taking it
> out would be a huge project!)

Yeah. I'm just thinking.

> > For this bug, I suggest we warn if there is no next line.
> 
> Hmm, like an NS_WARNING?  I don't think that's going to be useful.  And there
> *are* going to be cases where such a warning would be triggered (although they
> may be far fetched), like:
> 
> data:text/html,x<span style="white-space:pre">&#10;</span><span
> style="white-space:pre">&#10;</span>y
> 
> with caret browsing turned on.

If there's no next line, then the caret is going to appear in completely the wrong place. The user could press Enter and the caret would stay on the same line. That's clearly wrong. I think it's worth having a warning for stuff as bad as that.

(In reply to comment #96)
> (In reply to comment #93)
> > Why check this? Shouldn't we move the caret to the next line no matter
> > what's on the next line?
> 
> Should we?  Please note that this same code is responsible for caret movement
> on caret browsing as well, so how can we actually be sure that the frame on
> the next line is either a BR frame or a text frame?

We can't. But what if there's a contenteditable node which has a text frame followed by an image. The caret is at the end of the text frame and the user presses Enter. The image moves to the next line, and the caret should too.

Comment 100

[(no longer active)]

Attachment: Part 6: Put the caret on the next line's BR element or text node if we have a newline at the end of a textframe and a BR on the next line

(In reply to comment #99)
> > > For this bug, I suggest we warn if there is no next line.
> > 
> > Hmm, like an NS_WARNING?  I don't think that's going to be useful.  And there
> > *are* going to be cases where such a warning would be triggered (although they
> > may be far fetched), like:
> > 
> > data:text/html,x<span style="white-space:pre">&#10;</span><span
> > style="white-space:pre">&#10;</span>y
> > 
> > with caret browsing turned on.
> 
> If there's no next line, then the caret is going to appear in completely the
> wrong place. The user could press Enter and the caret would stay on the same
> line. That's clearly wrong. I think it's worth having a warning for stuff as
> bad as that.

Agreed.  Added the warning in this version of the patch.

> (In reply to comment #96)
> > (In reply to comment #93)
> > > Why check this? Shouldn't we move the caret to the next line no matter
> > > what's on the next line?
> > 
> > Should we?  Please note that this same code is responsible for caret movement
> > on caret browsing as well, so how can we actually be sure that the frame on
> > the next line is either a BR frame or a text frame?
> 
> We can't. But what if there's a contenteditable node which has a text frame
> followed by an image. The caret is at the end of the text frame and the user
> presses Enter. The image moves to the next line, and the caret should too.

The caret does move to the next line in that situation with this patch.

Comment 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #100)
> The caret does move to the next line in that situation with this patch.

Can you explain why?

Comment 102

[(no longer active)]

(In reply to comment #101)
> (In reply to comment #100)
> > The caret does move to the next line in that situation with this patch.
> 
> Can you explain why?

The frame hint would be set to "right", so we'll just attach to the next frame (which is the image frame) and the code in question doesn't kick in.

Comment 103

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why doesn't that work for BR and textframes on the second line, too?

Comment 104

[(no longer active)]

(In reply to comment #103)
> Why doesn't that work for BR and textframes on the second line, too?

Because BR frames are line participants, so this check <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp#112> passes and we end up not returning null from FindContainingLine, therefore we won't take the code path to figure out the caret frame using the frame hint.

Comment 105

[(no longer active)]

Attachment: Part 6: Collapse the selection in textarea's to the trailing BR if needed after every edit operation

So, I tried taking out the AdjustCaretFrameForLineEnd call and here's what happens in this test case: data:text/html,<textarea>

If you focus the editor and press Enter, nsCaret::DrawAtPositionWithHint is called with the textnode as aNode and offset set to 1.  nsFrameSelection::GetFrameForNodeOffset returns the text frame generated for the text node, and we use that text frame to draw the caret on, which results in the caret to appear on the first line (i.e., not move as a result of pressing Enter).

If you press Enter once more, nsCaret::DrawAtPositionWithHint is called with the anonymous DIV node as aNode and offset set to 1 (i.e., the selection is on the br node).  In this case, nsFrameSelection::GetFrameForNodeOffset selects the br frame (based on the frame hint which is set to "right" in both cases) and therefore the br frame is correctly selected (and AdjustCaretFrameForLineEnd is practically a no-op even if I don't take it out).

This made me suspect that we're not setting the selection on the br node when the user presses Enter for the first time (which should be accomplished by attachment 462580 [details] [diff] [review]).  Indeed this is what's happening.  nsTextEditRules::CreateTrailingBRIfNeeded is called from AfterEdit, which is called after nsTextEditRules::DidInsertBreak is executed, so DidInsertBreak will not set the selection on the br element after pressing Enter for the first time.  I created a patch to fixed this problem.  With that patch, the caret correctly moves down one line after the user presses Enter.

However, when you press backspace for example the caret would jump up two lines, which is another manifestation of the problem at the second paragraph of this comment.  This made me think that we probably need to collapse the selection to the BR node on every edit, so I decided to move the entire thing to AfterEdit.  With that, the problem that the old version of part 6 tried to solve never happens in the first place.

This is the patch which does that.

Comment 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 471300 [details] [diff] [review]
Part 6: Collapse the selection in textarea's to the trailing BR if needed after every edit operation

Nice! I'm glad we found the underlying problem here :-)

Comment 107

[(no longer active)]

(In reply to comment #106)
> Comment on attachment 471300 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=471300
> Part 6: Collapse the selection in textarea's to the trailing BR if needed after
> every edit operation
> 
> Nice! I'm glad we found the underlying problem here :-)

Yep!

So, I ran mochitest-plain, reftest and crashtests locally with this patch and they all passed.   I also tested this a bit manually, and I couldn't find any more problems.  I'm pushing a try server job right now, but given the assumption that it succeeds, do you think I'm fine with trying to land this tomorrow morning?

Comment 108

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes!

Comment 109

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b73bb8fcce79
http://hg.mozilla.org/mozilla-central/rev/612cf52bab79
http://hg.mozilla.org/mozilla-central/rev/e2e545fc9e23
http://hg.mozilla.org/mozilla-central/rev/70e1ac222929
http://hg.mozilla.org/mozilla-central/rev/3d505a4c64e1
http://hg.mozilla.org/mozilla-central/rev/1768a64485e5
http://hg.mozilla.org/mozilla-central/rev/9200a13105c9
http://hg.mozilla.org/mozilla-central/rev/b35bf5d83667
http://hg.mozilla.org/mozilla-central/rev/665df2e1e79f
http://hg.mozilla.org/mozilla-central/rev/02ee8dc174a4
http://hg.mozilla.org/mozilla-central/rev/1cf6271f4ec6
http://hg.mozilla.org/mozilla-central/rev/d453fc45427b
http://hg.mozilla.org/mozilla-central/rev/117eb39cd47f
http://hg.mozilla.org/mozilla-central/rev/d044bc01d126

Comment 110

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

just wow... congrats Ehsan, i'll try that in BlueGriffon tomorrow.

Comment 111

[(no longer active)]

(In reply to comment #110)
> just wow... congrats Ehsan, i'll try that in BlueGriffon tomorrow.

I'm very interested to hear if it improves things for BlueGriffon, and also if you can find any regressions (perf or correctness).

Comment 112

[Gary [:streetwolf52]]

The writing caret in text boxes sometimes "lags" behind and will only follow your input when you stop typing.  I'm assuming this fix might be causing this.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre - Build ID: 20100902141223

Comment 113

[(no longer active)]

(In reply to comment #112)
> The writing caret in text boxes sometimes "lags" behind and will only follow
> your input when you stop typing.  I'm assuming this fix might be causing this.
> 
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100901
> Firefox/4.0b6pre - Build ID: 20100902141223

Can you please file a bug with a web page on which you've been seeing this behavior?  Thanks!

Comment 114

[(no longer active)]

I've backed this out because of bug 593211 and bug 593160.  :(

Comment 115

[Gary [:streetwolf52]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=593223]Bug 593223

Comment 116

[(no longer active)]

Attachment: Part 14: Disable the failing test on D2D

See bug 593160 comment 9.

Comment 117

[(no longer active)]

Relanded:

http://hg.mozilla.org/mozilla-central/rev/c611f052ec92
http://hg.mozilla.org/mozilla-central/rev/4605f4ef1a22
http://hg.mozilla.org/mozilla-central/rev/2d960a2dd288
http://hg.mozilla.org/mozilla-central/rev/3d1067e0318f
http://hg.mozilla.org/mozilla-central/rev/5c877925d6f5
http://hg.mozilla.org/mozilla-central/rev/5c29a10e3006
http://hg.mozilla.org/mozilla-central/rev/75768c38273c
http://hg.mozilla.org/mozilla-central/rev/3db69b4c7b1d
http://hg.mozilla.org/mozilla-central/rev/1f29b4fb184c
http://hg.mozilla.org/mozilla-central/rev/c3236786510b
http://hg.mozilla.org/mozilla-central/rev/0b6a82cdbdd0
http://hg.mozilla.org/mozilla-central/rev/5aac96c242ec
http://hg.mozilla.org/mozilla-central/rev/ccde84eb8c58
http://hg.mozilla.org/mozilla-central/rev/3e5864a81c08
http://hg.mozilla.org/mozilla-central/rev/055edd12d609

Comment 118

[IU]

According to Mecurial, this got backed out.  How come it still indicates resolved fixed?

Comment 119

[pal-moz]

this seems to cause to delete all <br> in textarea, eg. Movable Type

[str]
edit text like

aaaa
bbbb
cccc

then, save text.
after that text is 

aaaabbbbcccc

Comment 120

[Nochum Sossonko [:Natch]]

(In reply to comment #118)
> According to Mecurial, this got backed out.  How come it still indicates
> resolved fixed?

Where do you see this? AFAICT this landed and stuck.

(In reply to comment #119)
File a bug with a testcase and make it block this one please?

Comment 121

[pal-moz]

I cannot create a testcase.

Comment 122

[Boris Zbarsky [:bzbarsky]]

> I cannot create a testcase.

"testcase" can include a link to a website that shows the problem.  Or does it require a login?

In any case, please file a separate bug and cc me and ehsan so we can sort this out there?

Comment 123

[IU]

(In reply to comment #120)
> Where do you see this? AFAICT this landed and stuck.
> 
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=ccaffbc6a970

I suppose I'm probably reading it wrong.  I assumed that everything listed under the above changeset got backed out.  Ignore then.

Comment 124

[Boris Zbarsky [:bzbarsky]]

Yeah, that's some confusing display.  The backout part is just the topmost changeset before the merge...

Comment 125

[pal-moz]

(In reply to comment #122)

login needed.

anyone/developers using Movable Type (Blog system) can help ?
http://www.movabletype.org/

Comment 126

[(no longer active)]

pal-moz, I really need more information in order to solve this regression that you're talking about.  Can you please provide any more information?  Steps to reproduce the problem would be extremely useful.  Thanks!

Comment 127

[Boris Zbarsky [:bzbarsky]]

I have a movable type blog, fwiw.  Please file that separate bug and give precise steps to reproduce, and I can either look into it or tell Ehsan my login info.

Comment 128

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I just did a totally fresh build of BlueGriffon with this patch in. No problem so far. I'm still seeing <br> inserted in <pre> elements when I type on the CR key.

Comment 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This didn't change the HTML editor at all. Only <textarea>s.

Comment 130

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #129)
> This didn't change the HTML editor at all. Only <textarea>s.

Fine. Ehsan, if you want to extend your patch to <pre> elements in the
HTML editor, you're welcome :-)

No visible impact on BlueGriffon.

Comment 131

[(no longer active)]

(In reply to comment #130)
> (In reply to comment #129)
> > This didn't change the HTML editor at all. Only <textarea>s.

Well, that's only partially true.  A lot of the code which is shared between the two has changed, but yes, the behavior of HTML editors should not be changed.

> Fine. Ehsan, if you want to extend your patch to <pre> elements in the
> HTML editor, you're welcome :-)

That should not be very hard after this patch.  Want to file a bug on that? :)

Comment 132

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #131)

> > Fine. Ehsan, if you want to extend your patch to <pre> elements in the
> > HTML editor, you're welcome :-)
> 
> That should not be very hard after this patch.  Want to file a bug on that? :)

Filed loooooong ago. That's bug 92921.

Comment 133

[(no longer active)]

(In reply to comment #132)
> (In reply to comment #131)
> 
> > > Fine. Ehsan, if you want to extend your patch to <pre> elements in the
> > > HTML editor, you're welcome :-)
> > 
> > That should not be very hard after this patch.  Want to file a bug on that? :)
> 
> Filed loooooong ago. That's bug 92921.

I'll take that, but since we're too close to feature freeze, and that would in fact entail web facing features, I'm afraid it has to wait for Gecko Next.

Comment 134

[pal-moz]

(In reply to comment #126)
> pal-moz, I really need more information in order to solve this regression that
> you're talking about.  Can you please provide any more information?  Steps to
> reproduce the problem would be extremely useful.  Thanks!

see com#119

[STR]

(login needed)
edit/create new entry.
input text in textarea, like below.
 aaaaa
 bbbbb
 ccccc
save entry.
now text in textarea is
 aaaabbbbccccdddd


(In reply to comment #127)
> I have a movable type blog, fwiw.  Please file that separate bug and give
> precise steps to reproduce, and I can either look into it or tell Ehsan my
> login info.

you cannot reproduce this ?

I'm using Movable Type v4.27
if fixed/WFM with Movable Type v5, I'll update.

Comment 135

[Boris Zbarsky [:bzbarsky]]

pal-moz, PLEASE file a separate bug like I asked you to instead of putting all that stuff in this bug?

Comment 136

[pal-moz]

(In reply to comment #135)
> pal-moz, PLEASE file a separate bug like I asked you to instead of putting all
> that stuff in this bug?

filed, bug 596455