Fixed Editor inserting raw bb-code does not trim trailing <br> tags inside <p>

X

Xon

Well-known member
Affected version
2.2.6
Suppose you have; the text;
Code: 
1

3
Which has the html;
HTML: 
<p>1</p>
<p><br></p>
<p>3</p>

Then on line 2 you insert a spoiler/code/ispoiler/icode/<some raw bb-code here>, then the editor will have this html;
HTML: 
<p>1</p>
<p>[ code ][ /code ]<br></p>
<p>3</p>

Note; the invisible <br> is still there at the end of line, which results in weird editor behaviour essentially if you have a lot of these added.

The problem looks to be somewhere inside wrapSelectionText or insertCode related functions
 
Suppose the editor has this contents
Code: 
<p>1</p>
<p><br/>3</p>
This renders as;
Code: 
1

3

However if you press the delete key after '1'; the editor results in
Code: 
<p>13</p>
And not the expected;
Code: 
<p>1</p>
<p>3</p>

If there is a trailing <br> in the <p>, it is effectively invisible; then you need to press the delete key multiple times after the '1' value to actually delete the newline.

It looks like normalizePaste br normalization code assumes that a leading <br> and a trailing <br> are fine.
 
Editor will delete too many lines;
HTML: 
<p><br>...</p>

Editor will delete too few lines
HTML: 
<p>...<br></p>

Delete on the first empty line causes buggy text transformations;
HTML: 
<p><br><p>
<p><span>1</span></p>

Result;
HTML: 
<p><span style="background-color: transparent;">1</span><br></p>
 
Very initial prototyping to fix this sort of behaviour;

JavaScript: 
var checkNodeMatch = function ($node, elementType) {
    return $node.is(elementType) &&
        $node.className === '' &&
        ('' + $node.attr('id')) === '' &&
        ('' + $node.attr('style')) === ''
};

$fragWrapper.children('p').each(function () {
    if (this.childNodes.length !== 1) {
        return;
    }
    var $firstChild = $(this.childNodes[0]);
    if (checkNodeMatch($firstChild, 'span')) {
        $(this).html($firstChild.html());
    }
});
$fragWrapper.children('p').each(function() {
    if (this.childNodes.length <= 1) {
        return;
    }
    var $firstChild = $(this.childNodes[0]);
    if (checkNodeMatch($firstChild, 'br')) {
        $(this).before($('<p>').append($firstChild));
    }
});
$fragWrapper.children('p').each(function() {
    if (this.childNodes.length <= 1) {
        return;
    }
    var $lastChild = $(this.childNodes[this.childNodes.length - 1]);
    if (checkNodeMatch($lastChild, 'br')) {
        $lastChild.remove();
    }
});

This mostly fixes the cases I've seen when trying to extend copy & paste support
 
So that checkNodeMatch is garbage, this what happens when I refactor stuff without enough testing.

Code: 
var checkNodeMatch = function ($node, elementType) {
    var node = $node.get(0);
    return $node.is(elementType) &&
        node.className === '' &&
        !node.hasAttribute('id') &&
        !node.hasAttribute('style')
};

This more correctly looks for it being a non-formatted node that can be removed.

This issue can probably be merged with;
xenforo.com

Fixed - Editor inserting raw bb-code does not trim trailing <br> tags inside <p>

Suppose you have; the text; 1 3 Which has the html; 1 3 Then on line 2 you insert a spoiler/code/ispoiler/icode/, then the editor will have this html; 1 [ code ][ /code ] 3 Note; the invisible is still there at the end of line, which results in weird editor behaviour essentially if...
xenforo.com xenforo.com
 
Last edited:
The following code appears affected;
XF.EditorDialogMedia.submit
XFMG.editorButton.submit

XF.Editor.insertContent is not affected for quotes/replies as those are formatted with reliably formatted but is effected for the insert attachment image/thumbnail.
 
@Chris D This is what I ended up doing to work-around the editor behaviour/bug;

Code: 
        normalizeBrForEditor: function (content) {
            var asString = typeof content === 'string',
                $fragWrapper;
            if (asString) {
                $fragWrapper = $('<div />').html(content);
            } else {
                $fragWrapper = content;
            }
            var checkNodeMatch = function ($node, elementType) {
                var node = $node.get(0);
                return $node.is(elementType) &&
                    node.className === '' &&
                    !node.hasAttribute('id') &&
                    !node.hasAttribute('style')
            };

            // Workaround editor behaviour that a <br> should not be the first or last child of a <p> tag
            // <p><br>...</p>; editor can delete too many lines
            // <p>...<br></p>; editor can delete too few lines

            $fragWrapper.children('p').each(function () {
                if (this.childNodes.length !== 1) {
                    return;
                }
                var $firstChild = $(this.childNodes[0]);
                if (checkNodeMatch($firstChild, 'span')) {
                    $(this).html($firstChild.html());
                }
            });
            $fragWrapper.children('p').each(function() {
                if (this.childNodes.length <= 1) {
                    return;
                }
                var $firstChild = $(this.childNodes[0]);
                if (checkNodeMatch($firstChild, 'br')) {
                    $(this).before($('<p>').append($firstChild));
                }
            });
            $fragWrapper.children('p').each(function() {
                if (this.childNodes.length <= 1) {
                    return;
                }
                var $lastChild = $(this.childNodes[this.childNodes.length - 1]);
                if (checkNodeMatch($lastChild, 'br')) {
                    $lastChild.remove();
                }
            });

            return asString ? $fragWrapper.html() : $fragWrapper;
        },
        normalizeAfterInsert: function(ed) {
            var selected = ed.html.getSelected();
            if (/<br>\s*<\/p>/.test(selected)) {
                SV.AdvancedBBCode.EditorButtons.normalizeBrForEditor(ed.$el);
                // remove the last undo step and replace it with the corrected html version
                ed.undo_index--;
                ed.undo_stack.pop();
                ed.undo.saveStep();
            }
        }

This is the normalizeBrForEditor function I've ended using to fix all the cases I've encountered in how this bug can be triggered after copy & paste (including custom bb-code).

The normalizeAfterInsert is used when patching a number of places which insert text into the editor (XF.EditorHelpers.insertCode/XF.EditorHelpers.wrapSelectionText/editor.insertContent).
 
Are you happy for us to roll this into the core? Seems a shame for us to do the same work all over again to produce what is likely to be a similar result.
 
I've integrated that code. I've not done a deep amount testing but the cases pointed out here seem to be fine. Please speak out if the integration doesn't play well with your usage within your add-ons or has bits not relevant to these fixes or if the integration of the code would be better in different places.

Diff: 
Index: js/xf/editor.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/xf/editor.js b/js/xf/editor.js
--- a/js/xf/editor.js    (revision Staged)
+++ b/js/xf/editor.js    (date 1631194909291)
@@ -902,7 +902,7 @@
                 $output = $children;
             }
 
-            return $output.html();
+            return XF.EditorHelpers.normalizeBrForEditor($output.html());
         },
 
         watchEditorHeight: function()
@@ -1320,6 +1320,8 @@
                 ed.html.insert(html);
                 ed.undo.saveStep();
                 XF.Element.initialize(ed.$el);
+
+                XF.EditorHelpers.normalizeAfterInsert(ed);
             }
 
             this.scrollToCursor();
@@ -1939,6 +1941,8 @@
             ed.selection.restore();
             ed.placeholder.hide();
             ed.undo.saveStep();
+
+            XF.EditorHelpers.normalizeAfterInsert(ed);
         },
 
         insertCode: function(ed, type, code)
@@ -1969,6 +1973,8 @@
             ed.undo.saveStep();
             ed.html.insert(output);
             ed.undo.saveStep();
+
+            XF.EditorHelpers.normalizeAfterInsert(ed);
         },
 
         insertSpoiler: function(ed, title)
@@ -1986,6 +1992,96 @@
             XF.EditorHelpers.wrapSelectionText(ed, open, '[/SPOILER]', true);
         },
 
+        normalizeBrForEditor: function (content)
+        {
+            var asString = typeof content === 'string',
+                $fragWrapper;
+
+            if (asString)
+            {
+                $fragWrapper = $('<div />').html(content);
+            }
+            else
+            {
+                $fragWrapper = content;
+            }
+
+            var checkNodeMatch = function ($node, elementType)
+            {
+                var node = $node.get(0);
+
+                return ($node.is(elementType)
+                    && node.className === ''
+                    && !node.hasAttribute('id')
+                    && !node.hasAttribute('style'));
+            };
+
+            // Workaround editor behaviour that a <br> should not be the first or last child of a <p> tag
+            // <p><br>...</p>; editor can delete too many lines
+            // <p>...<br></p>; editor can delete too few lines
+
+            $fragWrapper.children('p').each(function ()
+            {
+                if (this.childNodes.length !== 1)
+                {
+                    return;
+                }
+
+                var $firstChild = $(this.childNodes[0]);
+
+                if (checkNodeMatch($firstChild, 'span'))
+                {
+                    $(this).html($firstChild.html());
+                }
+            });
+
+            $fragWrapper.children('p').each(function()
+            {
+                if (this.childNodes.length <= 1)
+                {
+                    return;
+                }
+
+                var $firstChild = $(this.childNodes[0]);
+
+                if (checkNodeMatch($firstChild, 'br'))
+                {
+                    $(this).before($('<p>').append($firstChild));
+                }
+            });
+
+            $fragWrapper.children('p').each(function()
+            {
+                if (this.childNodes.length <= 1)
+                {
+                    return;
+                }
+
+                var $lastChild = $(this.childNodes[this.childNodes.length - 1]);
+
+                if (checkNodeMatch($lastChild, 'br'))
+                {
+                    $lastChild.remove();
+                }
+            });
+
+            return asString ? $fragWrapper.html() : $fragWrapper;
+        },
+
+        normalizeAfterInsert: function(ed)
+        {
+            var selected = ed.html.getSelected();
+
+            if (/<br>\s*<\/p>/.test(selected))
+            {
+                XF.EditorHelpers.normalizeBrForEditor(ed.$el);
+                // remove the last undo step and replace it with the corrected html version
+                ed.undo_index--;
+                ed.undo_stack.pop();
+                ed.undo.saveStep();
+            }
+        },
+
         isPreviewAvailable: function($textarea)
         {
             if (!$textarea.data('preview-url') && !$textarea.closest('form').data('preview-url'))
 
Those patches look good!

There look to be a couple spots calling ed.html.insert directly. Most of them look OK-ish, but some of them insert effectively arbitrary data into the editor so probably should ensure they go via an XF wrapper function.

Functions I quickly spotted are;
XFMG.editorButton.initializeDialog.submit
XF.EditorDialogMedia.submit
FroalaEditor.PLUGINS.xfSmilie
FroalaEditor.PLUGINS.xfInsertGif

Maybe using XF.insertIntoEditor or similar which eventually calls one of the patched functions?
 
New documented edge case;

HTML: 
<p>word1 word2<br></p>
Putting the cursor after the 2, and deleting via backspace then results in;
HTML: 
<p>word1 <br></p>
This causes the whitespace to be collapse, eating the whitespace between word1 and the newline character.
 
@Chris D part of the problem is the single line copy & paste fragment is haven't the wrapping <p> tags stripped so normalizeBrForEditor isn't doing anything. The editor then behaves differently for inline copy & paste vs block copy & paste.

The other part is paste.afterCleanup event is only working on the content being inserted and not the full editor. So the inline copy & paste can put the editor into a bad state.

Block copied text is;
HTML: 
word1 word2<br>
word1 word2
Editor before;
HTML: 
<p><br></p>
Editor after;
HTML: 
<p>word1 word2</p><p>word1 word2</p>

Inline copied text is;
HTML: 
word1 word2
Editor before;
HTML: 
<p><br></p>
Editor after;
HTML: 
<p>word1 word2<br></p>

Calling XF.EditorHelpers.normalizeBrForEditor(ed.$el) in the the paste.after event should fix this.

I think leaving the call to normalizeBrForEditor inside normalizePaste will work, as this code should be very lightweight compared to what is already been done.
 
Xon said:
Those patches look good!

There look to be a couple spots calling ed.html.insert directly. Most of them look OK-ish, but some of them insert effectively arbitrary data into the editor so probably should ensure they go via an XF wrapper function.

Functions I quickly spotted are;
XFMG.editorButton.initializeDialog.submit
XF.EditorDialogMedia.submit
FroalaEditor.PLUGINS.xfSmilie
FroalaEditor.PLUGINS.xfInsertGif

Maybe using XF.insertIntoEditor or similar which eventually calls one of the patched functions?
Click to expand...
Erring on the side of caution slightly and just leaving those calls to ed.html.insert there and calling the normalizeAfterInsert after each. Doing that on the basis that the direct inserts may have been chosen deliberately and I really don't want to start having to test and unpack all of these scenarios right now.

I think there will be a much greater project of editor scrutiny in the future.

Xon said:
@Chris D part of the problem is the single line copy & paste fragment is haven't the wrapping <p> tags stripped so normalizeBrForEditor isn't doing anything. The editor then behaves differently for inline copy & paste vs block copy & paste.

The other part is paste.afterCleanup event is only working on the content being inserted and not the full editor. So the inline copy & paste can put the editor into a bad state.

Block copied text is;
HTML: 
word1 word2<br>
word1 word2
Editor before;
HTML: 
<p><br></p>
Editor after;
HTML: 
<p>word1 word2</p><p>word1 word2</p>

Inline copied text is;
HTML: 
word1 word2
Editor before;
HTML: 
<p><br></p>
Editor after;
HTML: 
<p>word1 word2<br></p>

Calling XF.EditorHelpers.normalizeBrForEditor(ed.$el) in the the paste.after event should fix this.

I think leaving the call to normalizeBrForEditor inside normalizePaste will work, as this code should be very lightweight compared to what is already been done.
Click to expand...
(y) seems to do the trick
 
Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.7).

Change log:
Resolve a number of rich text editor quirks when pasting various content.
Click to expand...
There may be a delay before changes are rolled out to the XenForo Community.
 

Similar threads

X
Heading bb-code does not round-trip as expected
Replies
1
Views
552
Xon
X
TickTackk
Fixed HTML to BB Code does not convert code blocks to [ICODE] or [CODE]
Replies
2
Views
2K
XF Bug Bot
XF Bug Bot
Back
Top Bottom