Closed Bug 551704 Opened 13 years ago Closed 13 years ago

ContentEditable is inconsistently rendered

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+
status2.0 --- wanted

People

(Reporter: bmccann, Assigned: ehsan.akhgari)

References

(
URL
)

Details

Attachments

(5 files, 1 obsolete file)

Part 1: Don't initialize the selection needlessly when the editor is initialized
2.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2: Don't replace newlines with BR nodes for preformatted editable fields
9.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 3: Don't copy invisible BR nodes to the clipboard for editable elements
8.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 4: Set the selection to the beginning of the document when we're entering the design mode
3.12 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 5: Move breakIsVisible to nsIHTMLEditor_MOZILLA_2_0_BRANCH to make sure that nsIHTMLEditor does not change for Gecko 2.0
8.68 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

The contentEditable is unusable if it is empty, so you need to put some text in it.  E.g. you can put a <br /> tag in it and now it is usable.  But the browser is very inconsistent in its treatment of that tag.

Reproducible: Always

Steps to Reproduce:
Using the following HTML:
<!DOCTYPE html>
<html>
  <body>
    <div id="input1" style="width:250px; height:200px; border:1px solid black; outline:none; white-space:pre; cursor:text" contentEditable="true"><br /></div>
  </body>
</html>

1. Type in the contentEditable.  Notice that it is treated as if no <br /> tag is present.
2. Click in the contentEditable, hit Ctrl-A followed by Ctrl-C to select all text.  Open a text editor and hit Ctrl-V.  Notice that it is treated as if you just copied the <br />.
Actual Results:  
In the first instance it is treated as if there is no line break in the contentEditable.  In the second instance it is treated as if there is indeed a line break in the contentEditable.

Expected Results:  
The two cases should behave similarly.
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
I should note also that this is not a problem in WebKit.
blocking2.0: --- → ?
Version: unspecified → Trunk
This is one of a few contentEditable bugs that has made it difficult/impossible to implement Google Spreadsheet's formula highlighting in Firefox.  We'd like to turn the feature on in FF 3.7 assuming enough of the issues have been addressed in the browser.  Thanks!
http://googledocs.blogspot.com/2010/05/formula-highlighting-in-spreadsheets.html
Assignee: nobody → ehsan
blocking2.0: final+ → betaN+
Please try this: click on the URL I just pasted in this bug.  This is an HTML document with a contenteditable element without a BR tag. It is perfectly usable to enter and format text, and a newline is not copied when you select its contents and copy them to the clipboard.

Does that address your concerns here?  Or are there more issues to solve?
URL: data:text/html,<div id="input1" style...
I also wanted to ask you to try this in a nightly build of Firefox, available from http://nightly.mozilla.org/.
Thanks Ehsan.  Sounds goods to me.  Doesn't work in 3.6.10, but I'll try to test in a nightly version soon.  It's a bit tricky for me to install a nightly version because I don't have access to a Windows machine at the moment.  I'm going to do some testing with a nightly version after Bug 389321 is fixed.  You can mark this bug as fixed for now and I'll let you know if I still have any trouble when I test the other bug.
(In reply to comment #5)
> Thanks Ehsan.  Sounds goods to me.  Doesn't work in 3.6.10, but I'll try to
> test in a nightly version soon.  It's a bit tricky for me to install a nightly
> version because I don't have access to a Windows machine at the moment.  I'm
> going to do some testing with a nightly version after Bug 389321 is fixed.  You
> can mark this bug as fixed for now and I'll let you know if I still have any
> trouble when I test the other bug.

You can try a nightly version on Mac or Linux as well.  They should have the same result as far as this bug is concerned...

But if you can't test this right now, I'd rather keep this open for tracking purposes, and wait until you test a nightly with the fix to bug 389321...
It would be very helpful if you can test tonight's nightly to see if this problem still occurs for you now that bug 389321 has been fixed.
Oh, you know what?  I was reading comment 0 wrong for some reason.  I managed to reproduce the bug, I'll start working on it tomorrow.
URL: data:text/html,<div id="input1" style... → data:text/html,<div id="input1" style...
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
The first problem here is that nsEditor::Init calls BeginDocument, which can mess us up because it can initialize the selection in the HTML document for contenteditable fields even if the editable area is not focused.  Initializing the selection when needed should be handled in nsEditor::PostCreate by a call to InitializeSelection.
Attachment #488560 - Flags: review?(roc)
With bug 240933, we don't need to replace newline characters with BR nodes for preformatted editable fields.  This patch removes the code responsible for that, and adds a test to make sure that no BR node is added.
Attachment #488569 - Flags: review?(roc)
Chrome removes the BR node in the test case as soon as you enter some text under it.  This behavior is wrong, I think, unless the user performs a Select All before editing.

Opera preserves the BR node, but doesn't copy a newline character to the clipboard the way we do, which makes sense, since the user doesn't see a newline character anyway.  I think we should adopt this behavior.
This patch makes sure that we don't copy invisible BR nodes in editable areas to the clipboard.  Unfortunately it's not trivial to make nsHTMLEditor::IsVisBreak not depend on the editor, so that's why we should retrieve the document's HTML editor to be able to call this method on it.

Note that this doesn't modify the behavior of innerHTML, and those invisible BR nodes will continue to be exposed through innerHTML.
Attachment #488704 - Flags: review?(bzbarsky)
Whiteboard: [has patch][needs review bz][needs review roc]
Whiteboard: [has patch][needs review bz][needs review roc] → [has patch][needs review bz]
There's also one more case we need to handle for compatibility reasons, and that is putting the selection at the beginning of the document when entering design mode.  This patch does that, and adds the appropriate tests for it.
Attachment #489002 - Flags: review?(bzbarsky)
Comment on attachment 488704 [details] [diff] [review]
Part 3: Don't copy invisible BR nodes to the clipboard for editable elements

r=me, but file a followup to make this not need editor objects?
Attachment #488704 - Flags: review?(bzbarsky) → review+
Comment on attachment 489002 [details] [diff] [review]
Part 4: Set the selection to the beginning of the document when we're entering the design mode

r=me
Attachment #489002 - Flags: review?(bzbarsky) → review+
This patch moves breakIsVisible to a branch interface.  Once we branch for 2.0, we can just back this one patch out, and everything will continue to work on trunk.
Attachment #489377 - Flags: review?(bzbarsky)
With the IDL file actually added to the patch!
Attachment #489377 - Attachment is obsolete: true
Attachment #489378 - Flags: review?(bzbarsky)
Attachment #489377 - Flags: review?(bzbarsky)
Comment on attachment 489378 [details] [diff] [review]
Part 5: Move breakIsVisible to nsIHTMLEditor_MOZILLA_2_0_BRANCH to make sure that nsIHTMLEditor does not change for Gecko 2.0

r=me
Attachment #489378 - Flags: review?(bzbarsky) → review+
Whiteboard: [has patch][needs review bz] → [needs landing]
bmccann@google.com: it would be great if you can test tonight's nightly build which contains the fixes to this bug to see if all of the issues you were seeing here have been fixed.  If not, please let us know so that we can fix any remaining issues for Firefox 4.  Thanks!
(In reply to comment #14)
> Comment on attachment 488704 [details] [diff] [review]
> Part 3: Don't copy invisible BR nodes to the clipboard for editable elements
> 
> r=me, but file a followup to make this not need editor objects?

I filed bug 611103 for that.  I also filed bug 611101 to merge nsIHTMLEditor_MOZILLA_2_0_BRANCH to nsIHTMLEditor once we branch.
Just to note that for Thunderbird tests to pass after this change, I had to back out a change that I had previously done as a result of bug 592592.

Here's what I originally did when bug 592592 landed:

http://hg.mozilla.org/comm-central/rev/958cb6ba12fb

Here's what I've just done:

http://hg.mozilla.org/comm-central/rev/199544727de7

I think from the descriptions that this is probably right, and when bug 592592 landed it was possibly just a regression that we didn't pick up on?

Ehsan, if you can confirm my thinking, that would be good. Thanks.
(In reply to comment #22)
> Ehsan, if you can confirm my thinking, that would be good. Thanks.

This looks sane to me.
Depends on: 622371
Depends on: 629845
The test says:
    var expectedHTML = "abc<br>def<br>";
    var expectedText = "abc\ndef";

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug551704.html?force=1#114

Why is a trailing break expected in HTML but not in plain text?
See Also: → 1119503
You need to log in before you can comment on or make changes to this bug.