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

fix/1273: Fix child node length checks #2532

Merged
merged 1 commit into from
Aug 29, 2017
Merged

Conversation

BoardJames
Copy link

Fixes a typo in the previous pull request #2482 (review)

@codecov
Copy link

codecov bot commented Aug 25, 2017

Codecov Report

Merging #2532 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2532   +/-   ##
======================================
  Coverage    30.1%   30.1%           
======================================
  Files         174     174           
  Lines        5288    5288           
  Branches      905     905           
======================================
  Hits         1592    1592           
  Misses       3133    3133           
  Partials      563     563
Impacted Files Coverage Δ
blocks/library/freeform/old-editor.js 1.51% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3427c8...eca539c. Read the comment docs.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably this won't work if the caret is within an empty element which is the fourth child of its body? Wondering if this is something we'd want to use recursion or loops, like in Editable:

isStartOfEditor() {
const range = this.editor.selection.getRng();
if ( range.startOffset !== 0 || ! range.collapsed ) {
return false;
}
const start = range.startContainer;
const body = this.editor.getBody();
let element = start;
while ( element !== body ) {
const child = element;
element = element.parentNode;
if ( element.firstChild !== child ) {
return false;
}
}
return true;
}

Or @iseulde's suggestion at #2482 (comment)

@BoardJames
Copy link
Author

@aduth can you give an example of the content that could produce such an effect?

For example if I artifiically set the content to contain a span like:
tinyMCE.activeEditor.setContent('<p><span></span><br/></p>');
then the span is automatically removed.

The closest I can get to the situation you describe is by toggling on 'bold' in which case the first backspace removes the bold formatting (a behaviour I think is actually good).

@aduth
Copy link
Member

aduth commented Aug 28, 2017

Okay, on second glance, I see you're testing against the specific pattern of <p><br data-mce-bogus></p>.

What are your thoughts on using tinymce.DOM.isEmpty instead, which accounts for many other types of content? (Edit: I see you responded at #2482 (comment))

https://github.com/tinymce/tinymce/blob/cb7bba98a85b635e09283578b35d8c9436af77db/src/core/src/main/js/dom/DOMUtils.js#L1460-L1533

@BoardJames BoardJames merged commit 29ef52c into master Aug 29, 2017
@ntwb ntwb deleted the fix/1273-correct-typo branch August 29, 2017 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants