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

Node#isEmpty considers nbsp-only node as non-empty #4466

Open
aduth opened this issue Jun 27, 2018 · 5 comments
Open

Node#isEmpty considers nbsp-only node as non-empty #4466

aduth opened this issue Jun 27, 2018 · 5 comments
Assignees
Labels

Comments

@aduth
Copy link

aduth commented Jun 27, 2018

Do you want to request a feature or report a bug?

Bug. (Or verification of intent).

What is the current behavior?

When a new block is created, it is padded with an   non-breaking space character. The behavior of Node#isEmpty disregards whitespace, but not specifically the non-breaking space character.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via fiddle.tinymce.com or similar.

Demo: http://fiddle.tinymce.com/Pugaab

Enter some content and press Enter.

More directly, it could be reproduced by a test case on creating an empty text node.

const documentNode = Node.create( '#document-fragment' );
const textNode = Node.create( '#text' );
textNode.value = String.fromCharCode( 160 );
documentNode.append( textNode );
console.log( documentNode.isEmpty() );
// false

Possible solution: If changing the regular expression to use \s instead of , isEmpty correctly returns true.

Before:

const whiteSpaceRegExp = /^[ \t\r\n]*$/;

After:

const whiteSpaceRegExp = /^[\s\t\r\n]*$/;

What is the expected behavior?

The logged result should show "Is empty: true" since it is for the newly-created empty paragraph.

Which versions of TinyMCE, and which browser / OS are affected by this issue? Did this work in previous versions of TinyMCE?

TinyMCE 4.7.13
Unaware of previous working state.

@spocke
Copy link
Member

spocke commented Aug 9, 2018

An element is only considered empty if there is no caret positions or just white space. However nbsp is considered contents since it's a padded paragraph. Not exactly sure why you need this but you can disable the padding behavior of paragraphs by setting for example valid_elements: '*[*]' in tiny then all empty paragraphs will be keep as is.

We need a really good reason to break the isEmpty functionality and consider padded block elements as empty. So would be nice if we could get a use case where this is needed.

@aduth
Copy link
Author

aduth commented Aug 13, 2018

Here's a more direct example, where a brand new (empty) initialization of the editor reports as being non-empty:

http://fiddle.tinymce.com/1ygaab/1

To me, it seems like the semantics of being empty should trump whether there's a padded node there. If I initialized the editor with no content, should it not be considered empty?

I'll dig to see if there was a case in Gutenberg we encountered this. We decided against the tree format, so it may not be a pressing need at the moment, but still seems like an unexpected behavior.

@aduth
Copy link
Author

aduth commented Aug 13, 2018

This was originally discovered in WordPress/gutenberg#7583, which was a closed experiment of using format: 'tree' (considered failed due to non-performant behavior of tree in repeated calls), where a new block's tree node was reported as non-empty.

@androb androb added the WordPress tickets requested by WordPress core team label Aug 23, 2018
@Afraithe
Copy link
Member

Hey Aduth, we are looking into this.

Trying to understand what/how you need this.

Wouldn't adding something like editor.isEmpty() not work better in this case?

As you said, your not using the format: tree functionality, so how does this manifest for you?

@aduth
Copy link
Author

aduth commented Aug 29, 2018

As far as prioritization, this isn't actively affecting us in Gutenberg. I do think it's a legitimate issue to consider for anyone who might want to use the tree nodes result. We've / I've explored it on a few occasions, and it seems like it would be a more compatible option, but based on previous findings we're not exploring it further at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants