Open Bug 503838 Opened 11 years ago Updated 2 days ago

[meta] Get rid of bogus and trailing BR nodes in editor

Categories

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

defect

Tracking

()

People

(Reporter: peterv, Unassigned)

References

(Depends on 3 open bugs, Blocks 5 open bugs)

Details

(Keywords: leave-open, meta, Whiteboard: h2review-noted)

Attachments

(1 file)

Editor currently adds bogus and trailing BR nodes in the source but keeping them in sync through various editor operations is tricky, and a source of bugs. We should get rid of them, using some other mechanism to deal with the problems they try to solve. One of the issues that blocks this is that we can't position the caret after the last line, because layout doesn't create line boxes for empty lines.
A while back I experimented with using :after to add a BR node at the end of the editor content, but all the caret and selection code would need to be updated so that they know that the (collapsed) selection can be positioned after the last character of the node that precedes the generated content node.
Same as Bug 414223?
Blocks: 414223
This is HTML editor, right?  The plaintext editor bug is bug 240933.

Also note bug 205946, bug 333024.
The ::after approach would be difficult to make work properly if we're in contenteditable and the author already has ::after content.

HTML5 says

> The current text editing caret (the one at the caret position in a focused
> editing host) is expected to act like an inline replaced element with the
> vertical dimensions of the caret and with zero width for the purposes of the
> CSS rendering model.
>
> This means that even an empty block can have the caret inside it, and that when
> the caret is in such an element, it prevents margins from collapsing through
> the element.

We don't have to do it that way, but I do think that something giving the same effect would make sense.

For example, we could have blocks create a line-of-inlines if necessary to hold the caret. We would treat any line containing the caret as non-empty.
We would have to reflow on some caret moves, but that would probably be OK, especially if for plaintext editing we only had to reflow when the caret moves to the last line.
Blocks: 519440
(In reply to comment #1)
> all the caret and selection code would need to be
> updated so that they know that the (collapsed) selection can be positioned
> after the last character of the node that precedes the generated content node.

The ability to position the collapsed selection after the last character of a text node but inside the container element has to work no matter what approach we take, if we're going to eliminate bogus nodes from the DOM.

Just implementing something based on the caret position, like the spec text quoted in comment #4, is not enough because there is nowhere for the user to click on when you have an empty editable block. In particular, it seems to me that if you have, e.g., "1\n2\n" in a textarea or contenteditable element that's overflow:auto and set to two lines high, there should be a scrollbar and the user should be able to scroll an empty third line into view, even if the caret is not currently there. Only creating that third line when the caret is put there (by keyboard, presumably, since you can't click in the line before it exists) sounds really weird.

So I suggest ignoring the spec text for now. Instead of depending on the caret position, just change the reflow of any block that's an editing root (i.e. an editable block that does not have an editable parent). When we reflow such a block, if there isn't a line of inlines at the end, or if there is a line of inlines that ends in a forced line break (<br>, \n), then create a trailing empty line and make sure it has normal height as if there was content there. We do not need to create any actual frame to be on the line.
So... that will violate that invariant you told me we had about not having empty lines.  I'm fine with that, but we might need to audit consumers a tad.
It would, however it would only violate it for the last line, and I think we could probably prevent that empty line existing during the reflow of the block. I.e., at the beginning of reflow, remove that empty line if it exists, and then at the end of reflow add that empty line if it's needed.
Actually we could probably get away with not creating an actual nsLineBox, but just making nsBlockFrame::ComputeCombinedArea and nsBlockFrame::ComputeFinalSize add extra height for an imaginary last line.

We'd have to have special logic to position the caret in that line that doesn't exist, but we'll have to have that even if there was an nsLineBox there.
That "special logic" will be unfortunately complex ... it will have to handle 'text-align', 'text-indent', and possibly other things, to place the caret correctly. However I think the only way to avoid that "special logic" would be to create an empty text frame on the last line, which would also require changes to some invariants.
Actually that approach is really hard in general. Consider
<div contenteditable><span style="font-size:200%">\n|</span></div>
where | represents the caret. You really want the last line to be sized as if the span had wrapped into that line, but since it hasn't, figuring out what the line height should be is really hard. Adding vertical-align into the mix makes things even worse. And of course, if the caret moves away we still want to show the blank line with the right height. Ugh.

I have a different idea for handling text in <textarea>, I'll put that in bug 240933.
The editing spec currently requires that empty blocks be filled in with a <br>, which is then removed when it's no longer necessary.  So if you have "<p>foo|</p>" and hit enter, you get "<p>foo</p><p>|<br></p>"; but then if you type "bar", it becomes "<p>foo</p><p>bar|</p>", with the <br> disappearing.  This is basically how WebKit behaves, and it's the best I was able to come up with.  Gecko would just have to remove the <br>'s when they're no longer necessary.

We can't make editable and non-editable content render differently, because that breaks WYSIWYG.  If you write a blog post using contenteditable and then publish the exact same HTML, it has to render the same even though it's no longer editable.
So, basiclly in Gecko terminology, we want to keep the bogus BR, but not the trailing one, right?  (Of course the bogus BR attribute must be removed.)
Depends on: 1098151

Hello. I recently filed a bug #1615852 about spurious <br> and other inconsistencies. I am a bit worried about leaking the internal layout-fixing workarounds to the Javascript applications. I'm pretty sure that must create all sorts of issues unexpected both by the users and by the frontend developers.

I shared the following test code in the other bug:

<!DOCTYPE html>
<html>
<head>
    <style>
        #source { width: 10em; background-color: lightgreen; }
        #target { margin-top: 1em; background-color: lightgrey; }
    </style>
</head>
<body>
    <div id="source" contenteditable="true">xxx</div>
    <div id="target"></div>

    <script>
        source = document.getElementById("source");
        target = document.getElementById("target");

        function display() {
            target.innerText = source.innerHTML;
        }

        source.addEventListener("input", display);
        display();
    </script>
</body>
</html>

The aforementioned workaround both has unwanted side effects and doesn't work reliably. Try the following steps and check if you're getting the same (often wrong) behavior as I do:

  1. Load the above document.

xxx

  1. Put the carret after xxx and hit Backspace thrice.

<br> (the workaround)

  1. Put back xxx.

xxx<br> (looks good in my case but becomes xxx\n<br> in Google Sheets)

  1. Hit Enter.

<div>xxx</div><div><br></div> (does this make sense?)

  1. Hit Backspace.

<div>xxx</div> (inconsistent, <br> is lost now)

  1. Hit Backspace thrice.

<div><br></div> (inconsistent, <br> is now back but enclosed in divs)

  1. Hit Backspace.

`` (<br> is now lost and the workaround failed completely, layout is of course broken)

  1. Type xxx.

xxx (just for completeness, you are now back to the initial state)


Thus my conclusion that the workaround is both broken and has unexpected side effects.

Emilio, now with bug 1098151 fixed, what does it mean for this bug? I assume there is still some additional work needed?

Flags: needinfo?(emilio)

Maybe? Do we insert <br> for flex / grid / other layout modes? Does it matter for those?

But makoto or masayuki are the best people to address this. I'd probably wait a bit in case bug 1098151 causes any regression, anyhow.

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

But makoto or masayuki are the best people to address this. I'd probably wait a bit in case bug 1098151 causes any regression, anyhow.

Flags: needinfo?(masayuki)

Empty block like normal <p> element (at least) should keep having <br> element for web-compat and backward compatibility, but we should remove it when the block becomes not-empty like the other browsers. I also hit this bug when I wrote a WPT a couple of the days ago...

Flags: needinfo?(masayuki)

Sigh, I hit this bug when I try to change somewhere in editor classes... I'll try to fix this first.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Priority: -- → P2

HTMLEditor does not remove padding <br> element for last empty line when
it becomes unnecessary.

This patch removes it when inserting some text into a text node and it's
followed by a padding <br> element.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/67b2b97539bd
part 1: Remove following `<br>` element for empty line when it becomes unnecessary with inserting text r=m_kato
Blocks: 1360388
Regressions: 1636747
Regressions: 1638726
Blocks: 1615852
Depends on: 1645257
Depends on: 1596374

This looks like a semi-meta bug, should we add [meta] to the title?

(In reply to Kagami :saschanaz from comment #24)

This looks like a semi-meta bug, should we add [meta] to the title?

Well, this page is too long to discuss something. So, it's fine to make this a meta bug and working on blocker bugs.

Summary: Get rid of bogus and trailing BR nodes in editor → [meta] Get rid of bogus and trailing BR nodes in editor
Keywords: meta
Whiteboard: h2review-noted

Resetting assignee which I don't work on in this several months.

Assignee: masayuki → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.