Closed Bug 911201 Opened 11 years ago Closed 10 years ago

Bogus <br> is inserted at the beginning of contenteditable body with non-editable block

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: pkoszulinski, Assigned: ehsan.akhgari)

References

(
URL
)

Details

Attachments

(2 files)

bogus-br.html
132 bytes, text/html
Details
Do not create a bogus br node for editable areas containing non-editable block elements; r=roc
3.20 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file bogus-br.html — 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130803193343

Steps to reproduce:

1. Open bogus-br.html sample.
2. Open console and execute:
    document.body.innerHTML = '<div contenteditable="false">bar!bar!</div>';
3. Log document.body.innerHTML.


Actual results:

1. Body contains: '<br><div contenteditable="false">bar!bar!</div>'
2. Bogus <br> is visible inside body element - there's an empty line before "bar!bar1".


Expected results:

1. There should be no bogus <br> inserted.
2. This issue is not reproducible when non-editable div is loaded with a page. It only happens when content is created dynamically (via insertHTML or document.write()).
3. It does not happen in other than body editables.

This issue quite badly affects new CKEditor's feature on which we're currently working on: http://dev.ckeditor.com/ticket/9764
Not sure if this belongs in Editor or some DOM component.
Component: Untriaged → Editor
Product: Firefox → Core
Attachment #797864 - Attachment mime type: text/plain → text/html
FWIW, I can repro this against Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 ID:20110413222027 CSet: fca718600ca0 too.
Whiteboard: [dupeme]
There was a ticket reported on CKEditor's dev site for issue caused by this bug: https://dev.ckeditor.com/ticket/10854
Candidates for dupes are bug 656626 - but it's about appending BR at the end and bug 744408 - but it seems to be about selection source.

It may also just not be a dupe :)
(In reply to Zbigniew Braniecki [:gandalf] from comment #4)
> Candidates for dupes are bug 656626 - but it's about appending BR at the end
> and bug 744408 - but it seems to be about selection source.
> 
> It may also just not be a dupe :)

Both these tickets are about trailing block bogus <br>s - no one likes them, but they don't pollute the content "visually" and in fact they are needed to fill empty blocks (but should removed if block is not empty any more).

This issue is a bigger problem though, because the content is moved one line lower because of the <br>. To see a real case:

1. go to: http://ckeditor.com/ckeditor_4.3_beta/samples/plugins/image2/image2.html
2. switch to source mode and set it to:

<figure class="caption"><img alt="Saturn V" src="assets/image1.jpg" width="200" />
<figcaption>Roll out of Saturn V on launch pad</figcaption>
</figure>

3. switch back to wysiwyg mode

See that there's a leading bogus <br> added.

What's more, and this is an extension of this ticket, this <br> will be added dynamically by Firefox if we removed it.

1. Open console and remove the <br> by: CKEDITOR.instances.editor1.editable().getFirst().remove();
2. Click on the image (BTW. it reveals another issue - https://bugzilla.mozilla.org/show_bug.cgi?id=714137)
3. Click the body (somewhere below the image) - <br> is recreated, content jumps one line.

This happens because while image is selected there's a hidden container in which we store selection. We call it a "fake selection" and thanks to it we're able to select every element on the page. But when user moves selection somewhere else, that hidden container is removed and then Firefox decides to prepend a <br>. Since there's no editable place in the <body>, that <br> makes it possible to place the selection somewhere, but as we've seen it has nasty side effects. In our opinion, if there are only non-editable blocks inside element, it should not be possible to place selection there (but the body should still be focusable).
Is there any chance to at least have a feedback whether this is considered as a Gecko bug or not? Workarounding this behaviour requires a significant effort and ugly hacks, so if there's a chance to have this fixed in Gecko, we would rather avoid spending our time on this.
Eshan, please see comment 5 (when you return).
Flags: needinfo?(ehsan.akhgari)
OS: Linux → All
Hardware: x86 → All
Whiteboard: [dupeme]
Version: 23 Branch → Trunk
Sorry for the delay here, I was on vacation!

The br element that you see here is only created by us for empty editable regions, so this behavior is very surprising.  I ran this under the debugger and I can confirm that by the time that we create the br node, the <body> element that ckeditor uses is indeed empty.  Out of curiosity, I tested the HTML in comment 5 in <http://www-archive.mozilla.org/editor/midasdemo/> which is a dumb editable widget and the bug doesn't exist there, so I think this is caused by ckeditor somehow (but I don't know a lot about its implementation to be able to say anything more conclusively.)

The other thing that I tried was modifying the HTML to use a <div> instead of <figure> and it seems like ckeditor rewrites the HTML to make it use a <p> element instead.  My best guess is some code in ckeditor which doesn't know how to handle figure/figcaption.  Is that a plausible theory?
Flags: needinfo?(ehsan.akhgari) → needinfo?(pkoszulinski)
Hey,

Thanks for the reply.

I mentioned CKEditor multiple times because this bug affects the way how we use contenteditable=true and contenteditable=false attributes. However, as I described in https://bugzilla.mozilla.org/show_bug.cgi?id=911201#c0 this bug can be reproduced without CKEditor, on a native contenteditable element.

The reason why you couldn't reproduce this bug on http://www-archive.mozilla.org/editor/midasdemo/ is that Midas uses the designMode, not contenteditable and there are differences between these modes. For instance, contenteditable=false does not have any effect in document with designMode turned to true, while it disables editing in an element with the contenteditable=true attribute.

Additionally, the <br> that appears in the DOM has a _moz_editor_bogus_node="TRUE" attribute (the attribute is invisible in the body.innerHTML result), which is a strong indication that it was the browser who inserted it.

Why does Firefox insert this <br>? My guess is that Firefox does not consider the <body>'s content (<div contenteditable="false">foo</div>) to be anything editable (and that's correct), so perhaps it takes a shortcut and decides that there's no content in the <body> at all (and that's incorrect).

This is a tricky case in fact, because the content really isn't editable and there's no place to put the caret. This is an edge case and I don't expect Firefox to do anything special. An editor like CKEditor can handle this situation, and for example we implemented a "fake selection" mechanism which let's us marking the <div> as selected while the real selection is being hidden. So we expect Firefox to simply do nothing. Especially, it should not insert that <br>, because it breaks the content.
Flags: needinfo?(pkoszulinski)
Thanks Piotrek, I now fully understand the issue.  Here is a simple test case.

We add this br element when we detect an empty editable region.  I believe the original reasoning for this has been for reserving enough vertical height and also to make sure that we position the blinking caret correctly.  The second reason has been a non-issue as of a few years ago, and the first problem may go away with bug 1098151, but we have a lot of stuff relying on the existence of this node internally so we can't easily remove it.

That being said, I'm testing a smaller fix that should help fix the particular problem you have been experiencing.  I will submit a patch for review once the testing is done!
Assignee: nobody → ehsan.akhgari
URL: data:text/html,<body contenteditable ...
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
The reasons for inserting the <br> filler (bogus <br> as we call it) are of course totally understandable for me. When CKEditor is performing some actions (e.g. custom enter key behaviour) it must insert it too.

As for bug 1098151, thanks for mentioning it because I will actually have comments there :).
We no longer rely on the bogus br node for positioning the caret.
If an editable region has a non-editable block element, we can
probably rely on the block element to reserve the vertical
height, so in those cases we should be able to get away without
a bogus br node.
Attachment #8529926 - Flags: review?(roc)
Comment on attachment 8529926 [details] [diff] [review]
Do not create a bogus br node for editable areas containing non-editable block elements; r=roc

Review of attachment 8529926 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsTextEditRules.cpp
@@ +1153,5 @@
>         bodyChild;
>         bodyChild = bodyChild->GetNextSibling()) {
>      if (mEditor->IsMozEditorBogusNode(bodyChild) ||
>          !mEditor->IsEditable(body) || // XXX hoist out of the loop?
> +        (mEditor->IsEditable(bodyChild) || mEditor->IsBlockNode(bodyChild))) {

remove extra parens
Attachment #8529926 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/585dbcf446ea
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
QA Whiteboard: [good first verify]
Not sure if at all related, but a similar bug exists (see bug 744408 and bug 779067) and is reproducible in Google Hangout (in Gmail, Inbox, G+, etc.).

Steps to repro:
1. start a chat with someone, type in some message
2. delete the message using backspace
3. click anywhere outside the chat frame (e.g. open an email, click on an empty spot, or anywhere outside the chat frame)
4. click inside the area where previous messages are displayed, note that the mesage input area regains focus and cursor is shown
5. type a message, note that a BR is added above the message
6. this is repeatable to insert as many BRs as repeated

This has been the behavior for some time according to the bugs, and is still present at version 39.
Flags: needinfo?(ehsan)
Thanks Tim, but can you please file a separate bug for that, including the above steps to reproduce?  Thanks!
Flags: needinfo?(ehsan)
Thanks, Ehsan. I've attached the repro steps to one of the existing bugs (bug 744408) that is very similar (likely the same), and cc'd you in the bug. Let me know if that's not what you had in mind. Thanks!
See Also: → 744408
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: