Open Bug 503838 Opened 11 years ago Updated 2 days ago
[meta] Get rid of bogus and trailing BR nodes in editor
Bug 503838 - part 1: Remove following `<br>` element for empty line when it becomes unnecessary with inserting text r=m_kato!
47 bytes, text/x-phabricator-request
|Details | Review|
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?
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.
(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.)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Priority: -- → P2
Pushed by firstname.lastname@example.org: 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
Summary: Get rid of bogus and trailing BR nodes in editor → [meta] Get rid of bogus and trailing BR nodes in editor
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.