322202 - [MIDAS] hitting ENTER when focus inside an empty paragraph (containing the "I dont need it" BR tag) results in strange output.

Status: RESOLVED FIXED

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

Description

[dpopa]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

[MIDAS] hitting ENTER when focus inside an empty paragraph (containing the "I dont need it" BR tag) results in strange output.


Reproducible: Always

Steps to Reproduce:
1. go to the http://www.mozilla.org/editor/midasdemo/ demo page
2. check the "View HTML Source" checkbox 
3. enter the following string the editor (an empty paragraph containing the I dont need it BR tag. You can easy get to this html by having document.execCommand("insertBrOnReturn", false, false) in FF 1.5 and hitting enter at the end of some paragraph.):
[--BEGIN SAMPLE HTML-->
<p><br /><p>
<--END SAMPLE HTML----]

4. go back to rich mode (uncheck the "View HTML Source" checkbox).
5. click inside the editor EXACTLY on where the empty P should be, to set the focus/insertion point to this empty paragraph
6. hit ENTER.

Actual Results:  
The HTML source is now (I added some new lines for clarity):
[--BEGIN result-->
<br>
<p><br></p>
<p><br></p>
<--END RESULT--]

If in step 5 you click below the empty P (actually if you click below the BODY tag :D ), the resulting HTML is slightly different:
[--BEGIN result-->
<br>
<p><br></p>
<--END RESULT--]

In either cases, the result is not what any user expect.

Expected Results:  
[--BEGIN expected result-->
<p><br></p>
<p><br></p>
<--END expected RESULT--]


This is a major issue for any CMS trying to use Mozilla rich editing capabilities.
We've waited for so long for the document.execCommand("insertBrOnReturn", false, false) command to actually work ( http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLDocument.cpp#3598 ).

Now we get this rather annoying bug.

Comment 1

[José Jeria]

Also see this with latest trunk build/Windows XP

STEPS (easier) TO REPRODUCE:

1. Visit https://bugzilla.mozilla.org/attachment.cgi?id=119342
2. Put the cursor in the end of "Text in a box!" paragraph
3. Press enter a couples of times. 

Note that it not only adds a <p>, but also a <br> when doing this

Comment 2

[Julian Hall]

I'm also seeing similar behaviour if you put the focus into the window programatically (e.g. "editor.contentWindow.focus()") without moving the selection inside the first paragraph (e.g. with "editor.contentWindow.getSelection().collapse (editor.contentDocument.firstChild, 0)").

Also if you don't put a "<p></p>" pair into the document, or if the user removes them via select-all+delete midas seems to always insert <br> regardless of the setting of insertbronreturn.  Don't know if this is related or not.

FF 1.5, WinXP.

Comment 3

[dpopa]

Well, the focus is there, the selection is there, but you can not see it because the empty paragraph or the empty editable body has the line-height equal to zero, funny, isn't it?
Also, focus inside any empty tag tricks you to think there's no selection, but there is, trust me.
Also, selection can be programaticallty set, by accident, in funny little places like in between TD and TR.

(In reply to comment #2)
> I'm also seeing similar behaviour if you put the focus into the window
> programatically (e.g. "editor.contentWindow.focus()") without moving the
> selection inside the first paragraph (e.g. with
> "editor.contentWindow.getSelection().collapse
> (editor.contentDocument.firstChild, 0)").
> 
> Also if you don't put a "<p></p>" pair into the document, or if the user
> removes them via select-all+delete midas seems to always insert <br> regardless
> of the setting of insertbronreturn.  Don't know if this is related or not.
> 
> FF 1.5, WinXP.
> 

Comment 4

[Loune]

Attachment: patch

This should fix the bug where the BRs are added outside <p>s when pressing enter on empty lines. BRs inside <p>s eg <p><br /></p> are normal for blank lines (if they weren't there the para would collapse to nothing.

Comment 5

[Loune]

Attachment: patch 2

Comment 6

[[e-mail]]

Comment on attachment 258977 [details] [diff] [review]
patch 2

>+    // we are at the beginning and the end (empty para), newBRneeded not needed!
I'm not sure this comment is correct, I was able to reproduce the bug using a paragraph containing an image for example.

I can see that the existing code was doing the wrong thing when aNode == aPara because it was inserting a <br> before aNode, and your code also works correctly in two alternative cases, one of which it happened to work before anyway, but which now uses your code instead.

However, I did find one bug in your code, and that is that you don't take into account the doesCRCreateNewP preference.

Comment 7

[Loune]

Attachment: patch 3

New patch with changes to comment and checking of doesCRCreateNewP

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Doesn't this patch need super-review before it gets checked in? Neil might be willing to r+sr it.

Comment 9

[[e-mail]]

Comment on attachment 259383 [details] [diff] [review]
patch 3

moa=glazou over irc

Comment 10

[José Jeria]

Will this be checked in for 1.9?

Comment 11

[Dan POPA]

This is also happening with contentEditable elements:

Reproducing in 2008012104 with the following content:

<div contentEditable="true">
	<p><br></p>
</div>


Pressing ENTER will result in:
<div contentEditable="true">
	<br>
	<p><br></p>
	<p><br></p>
</div>

Comment 12

[Dan POPA]

Attachment: Open in FF3 and follow instructions to reproduce the bug for contenteditable too.

Comment 13

[Reed Loden [:reed]]

Checking in editor/libeditor/html/nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <--  nsHTMLEditRules.cpp
new revision: 1.344; previous revision: 1.343
done

Comment 14

[José Jeria]

This introduced bug 414223