Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CKEditor removes trailing BR tags #4986

Closed
jswiderski opened this issue Nov 23, 2021 · 2 comments · Fixed by #5168
Closed

CKEditor removes trailing BR tags #4986

jswiderski opened this issue Nov 23, 2021 · 2 comments · Fixed by #5168
Assignees
Labels
status:confirmed An issue confirmed by the development team. support:2 An issue reported by a commercially licensed client. type:feature A feature request.
Milestone

Comments

@jswiderski
Copy link
Contributor

jswiderski commented Nov 23, 2021

Type of report

Bug

Provide detailed reproduction steps (if any)

NOTE: Could be related to https://dev.ckeditor.com/ticket/11392.

  1. Clean editor contents
  2. Type Test, apply font size 72px and press Shift+Enter 3 times
  3. Type Test and press Shift+Enter 3 times
  4. Type Test and press Shift+Enter 3 times
  5. Switch to source mode

Expected result

<p><span style="font-size:72px;">Test<br><br><br>Test<br><br><br>Test<br><br><br></span><br></p>

Actual result

BR tags from the last line are removed

<p><span style="font-size:72px">Test<br /><br /><br />Test<br /><br /><br />Test</span><br /><br /><br />&nbsp;</p>

Other details

  • Browser: Any
  • OS: Any
  • CKEditor version: 4.0
  • Installed CKEditor plugins: …

NOTE: The issue is related to how editor works, how issues with styling and caret position have been solved back in the day thus it may be hard to provide a fix.

@jswiderski jswiderski added type:bug A bug. status:confirmed An issue confirmed by the development team. support:2 An issue reported by a commercially licensed client. labels Nov 23, 2021
@jacekbogdanski
Copy link
Member

jacekbogdanski commented Feb 1, 2022

The issue is related to the design decisions of how HTML is processed by the editor and touches its core parts. Line breaks elements are moved outside inline content on purpose, to prevent situations where the editor produces HTML with dangling styling positions.

As an example, a user could add a couple of empty lines at the end of the document for printing purposes and save it. Loading the same document after a couple of days or by a different person would lead to a bad user experience, as some parts of the document would force specific styling in places where line breaks were placed.

To sum up, this behavior should be considered as "by design". Although, I understand that this design decision may be opinionated and in some applications where the editor is used e.g. for template building default behavior may be unwelcome.

Thanks to additional feedback from one of our clients we see that appending additional &nbsp; character to the last br element prevents moving line breaks outside inline elements. That trick may be helpful with introducing a separate feature addressing the original issue. As an example, content like:

<p><span style="font-size:18px">foobar<br><br></span></p>

will be unwrapped into:

<p><span style="font-size:18px">foobar</span><br><br></p>

However, appending additional &nbsp; at the end of the nested br will preserve line breaks inside an inline element:

<p><span style="font-size:18px">foobar<br><br>&nbsp;</span></p>

We could provide a separate configuration option (opt-in!) deciding if line breaks should be preserved inside inline elements.

Let's check whether that approach would be feasible, as such content detection may be problematic before processing HTML content, where unwrapping line breaks happen. There are multiple places where 'br' tags are detected and treated differently, e.g.:

  • if ( tagName == 'br' ) {
    pendingBRs.push( element );
    return;
    }
  • function createBogusAndFillerRules( editor, type ) {
    function createFiller( isOutput ) {
    return isOutput || CKEDITOR.env.needsNbspFiller ?
    new CKEDITOR.htmlParser.text( '\xa0' ) :
    new CKEDITOR.htmlParser.element( 'br', { 'data-cke-bogus': 1 } );
    }
    // This text block filter, remove any bogus and create the filler on demand.
    function blockFilter( isOutput, fillEmptyBlock ) {
    return function( block ) {
    // DO NOT apply the filler if it's a fragment node.
    if ( block.type == CKEDITOR.NODE_DOCUMENT_FRAGMENT )
    return;
    cleanBogus( block );
    // Add fillers to input (always) and to output (if fillEmptyBlock is ok with that).
    var shouldFillBlock = !isOutput ||
    ( typeof fillEmptyBlock == 'function' ? fillEmptyBlock( block ) : fillEmptyBlock ) !== false;
    if ( shouldFillBlock && isEmptyBlockNeedFiller( block ) ) {
    block.add( createFiller( isOutput ) );
    }
    };
    }
    // Append a filler right after the last line-break BR, found at the end of block.
    function brFilter( isOutput ) {
    return function( br ) {
    // DO NOT apply the filer if parent's a fragment node.
    if ( br.parent.type == CKEDITOR.NODE_DOCUMENT_FRAGMENT )
    return;
    var attrs = br.attributes;
    // Dismiss BRs that are either bogus or eol marker.
    if ( 'data-cke-bogus' in attrs || 'data-cke-eol' in attrs ) {
    delete attrs [ 'data-cke-bogus' ];
    return;
    }
    // Judge the tail line-break BR, and to insert bogus after it.
    var next = getNext( br ), previous = getPrevious( br );
    if ( !next && isBlockBoundary( br.parent ) )
    append( br.parent, createFiller( isOutput ) );
    else if ( isBlockBoundary( next ) && previous && !isBlockBoundary( previous ) )
    createFiller( isOutput ).insertBefore( next );
    };
    }

@CKEditorBot
Copy link
Collaborator

Closed in #5168

@CKEditorBot CKEditorBot added this to the 4.19.0 milestone Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:confirmed An issue confirmed by the development team. support:2 An issue reported by a commercially licensed client. type:feature A feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants