Bug: empty tags when splitting nested tags #46
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
assigned 
|
|
I added the remove empty tag logic to normalize tags as within that function we are already iterating for all nodes.
Previously we were cleaning out content on focus and blur despite what was happening, this resulted in bad markup when merging. In my opinion a better approach is to do the cleaning you need, only when you need it. Thus, I did the cleaning in merge and split. Still we have something when blur that is necessary as it has to happen after leaving a block where we inserted a new line; we will use the blur also for trimming traling spaces.
We cannot assume that the underlying HTML is clean so we should take the chance to clean up stuff once we initialize editable.
This doesn't remove spaces at the end, it just converts the last one to make it visible. I think the strategy of not having spaces at all in the end should go in a config option and shouldn't be a task of normalizeSpaces. Conflicts: src/content.js
|
While sorting out the empty tags I also fixed the way we run the sanitization logic in default behavior. Instead of having everthing on focus/blur I added what is needed where it is needed (so split/merge basically). @peyerluk what do you think? |
|
👍 |
This description follows the old guidelines, but I see that we have a conflict with the current master branch as we already refactored this test file. This needs to be fixed when merging.
|
Should be OK now for merging, I'll take care of this as several conflicts need to be solved first. Just let me know if it is OK for you or we should integrate something else. |
|
Looks great to me :) |
Conflicts: src/content.js src/core.js
matteoagosti
added a commit
that referenced
this pull request
Sep 19, 2013
Bug: empty tags when splitting nested tags
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When working with nested tags something bad can happen.
Example:
Making the initial
uitalic results inMoving the cursor after the
uand pressing enter results in the cursor at the beginning of the next block. Pressingbackspacedoesn't work because of that:The cursor is placed after the nested em tag, so beginning of host won't work. In general we should be robust against this situation by dealing with empty tags (but
br), however I wonder if we should clean these in normalizeTags (the one that merge tags due to split). If I think about it, in my opinion it doesn't make sense to have a formatting tag without any content, thus it should be deleted. @peyerluk shall I fix normalizeTags for that?