503838 - (better-invisible-br-handling) [meta] Get rid of bogus and trailing BR nodes in editor

Status: NEW

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

Description

[Peter Van der Beken [:peterv]]

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.

Comment 1

[Peter Van der Beken [:peterv]]

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.

Comment 2

[José Jeria]

Same as Bug 414223?

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is HTML editor, right?  The plaintext editor bug is bug 240933.

Also note bug 205946, bug 333024.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 13

[(no longer active)]

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.)

Comment 14

[Pavel Šimerda]

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

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

<br> (the workaround)

3. Put back xxx.

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

4. Hit Enter.

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

5. Hit Backspace.

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

6. Hit Backspace thrice.

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

7. Hit Backspace.

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

8. 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.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 16

[Emilio Cobos Álvarez [: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.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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...

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 503838 - part 1: Remove following `<br>` element for empty line when it becomes unnecessary with inserting text r=m_kato!

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/67b2b97539bd

Comment 23

[Mathew Hodson]

https://hg.mozilla.org/mozilla-central/rev/67b2b97539bd was backed out due to the regression in bug 1636747.

Comment 24

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

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

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In most cases, the fix of bug 1923251 fixes this bug. However, we still use bogus line breaks to make the last collapsible white-space visible before a block boundary. For fixing the bug, we need to use   for the white-space. However, for doing that we should use the new WIP white-space normalizer (bug 1658699).