Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Added Schema#limits and integrated it with data controller's modifySelection #825

Merged
merged 9 commits into from
Feb 26, 2017

Conversation

szymonkups
Copy link
Contributor

@szymonkups szymonkups commented Feb 20, 2017

Suggested merge commit message (convention)

Feature: Added Schema#limits and integrated it with data controller's modifySelection. Closes ckeditor/ckeditor5#3983.

@szymonkups szymonkups changed the title T/818 Added Schema#limits and integrated it with data controller's modifySelection Feb 20, 2017
@szymonkups
Copy link
Contributor Author

Please close together with ckeditor/ckeditor5-enter#39.

@Reinmar
Copy link
Member

Reinmar commented Feb 21, 2017

I fixed the failing tests, but there are missing tests for modifySelection():

image

@Reinmar
Copy link
Member

Reinmar commented Feb 21, 2017

Also, one important thing to notice is that you check only the immediate parent of the selection (whether it's a block or not). We don't have yet a case for that but you may have:

<caption><paragraph>^</paragraph></caption>

And the selection must not be extended to the left in this case.

@Reinmar
Copy link
Member

Reinmar commented Feb 21, 2017

More things:

  • This PR desc is wrong – the limits are integrated with all the DC's methods, not just modifySelection().
  • I'm able to paste a block content into an image caption if I paste at the end of it. If I paste in the middle the Pasting a list into an image caption crashes the editor ckeditor5#404 occurs. The first must be blocked – the insertion should not leave the limit element. The latter may be a bug in insertContent() too so you can give it a try fixing it.

@Reinmar
Copy link
Member

Reinmar commented Feb 25, 2017

I'm going to take over the PR from here, @szymonkups.

@Reinmar
Copy link
Member

Reinmar commented Feb 25, 2017

Warning for the future (and fun fact too :P). The added modifySelection() test looked like this:

test(
	'should not extend to outside of inline limit element',
	'<inlineLimit>foo[]</inlineLimit>',
	'<inlineLimit>foo[]</inlineLimit>'
);

(it was meant to test that the selection won't leave a "limit" element)

It passed. At the same time, the CC dropped below 100%. So what's wrong here?

Two things actually.

First – if the test was written before the code was modified (TDD!) you'd know that it passes without any changes in the code.

Why? Because the test is too limited. It tests the behaviour in too narrow context. Namely – the selection has no place to go wrong – there's no text outside the <inlineElement> :D.

When writing such tests I'm very often adding some additional elements or text nodes at the both ends of the content. I also try many variations and one or two more complicated scenarios where more things could go wrong. This, even though might be a bit exaggerated at the beginning (because you can have 100% CC a lot earlier), makes sure the algorithm stays stable in the future, when some inevitable refactoring touches it.

@Reinmar
Copy link
Member

Reinmar commented Feb 26, 2017

Okey dokey, I added a lot of tests for all 3 functions – modifySelection(), insertContent() and deleteContent(). I had to reimplement modifySelection() and I took this opportunity to change the semantics a bit. Now, the focus will be placed in the first position where text is allowed. This simplified the implementation a lot and handled a lot of cases, including some from #710. In case of other functions, I made sure that limit elements are never left or changed (e.g. merged with other elements in case of deleteContent()).

Based on my manual tests, all works fine now :).

@Reinmar Reinmar merged commit e3c3e33 into master Feb 26, 2017
@Reinmar Reinmar deleted the t/818 branch February 26, 2017 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate various algorithms with schema#limits
2 participants