Closed Bug 322202 Opened 16 years ago Closed 14 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: dpopa, Assigned: lpgcritter+bugz)

References

()

Details

(Whiteboard: midas)

Attachments

(2 files, 2 obsolete files)

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.
Component: General → Editor
Product: Firefox → Core
Version: unspecified → 1.8 Branch
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9a1?
Whiteboard: midas
Version: 1.8 Branch → Trunk
Assignee: nobody → mozeditor
QA Contact: general
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.
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.
> 

Flags: blocking1.9a1? → blocking1.9-
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #256769 - Flags: superreview?
Attachment #256769 - Flags: review?(daniel)
Attachment #256769 - Attachment is obsolete: true
Attachment #256769 - Flags: superreview?
Attachment #256769 - Flags: review?(daniel)
Attached patch patch 2 (obsolete) — Splinter Review
Attachment #258977 - Flags: review?
Attachment #258977 - Flags: review? → review?(neil)
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.
Attachment #258977 - Flags: review?(neil) → review-
Attachment #258977 - Attachment is obsolete: true
Attached patch patch 3Splinter Review
New patch with changes to comment and checking of doesCRCreateNewP
Attachment #259383 - Flags: review?(neil)
Attachment #259383 - Flags: review?(neil) → review+
Whiteboard: midas → midas [checkin needed]
Doesn't this patch need super-review before it gets checked in? Neil might be willing to r+sr it.
Whiteboard: midas [checkin needed] → midas
Attachment #259383 - Flags: superreview?(neil)
Comment on attachment 259383 [details] [diff] [review]
patch 3

moa=glazou over irc
Attachment #259383 - Flags: superreview?(neil) → superreview+
QA Contact: editor
Assignee: mozeditor → nobody
Will this be checked in for 1.9?
Flags: blocking1.9- → blocking1.9?
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>

Flags: blocking1.9? → blocking1.9+
Keywords: checkin-needed
Priority: -- → P2
Assignee: nobody → littlecritter
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
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Depends on: 414223
This introduced bug 414223
No longer depends on: 414223
You need to log in before you can comment on or make changes to this bug.