Soft Break support #54

alexeckermann · 2018-04-24T03:27:05Z

Feature: Soft Break support in the Enter plugin. Optional usage, enabled by plugin inclusion.

Usage

The user will need to include the SoftBreakPlugin, as they would the EnterPlugin, by importing ckeditor5-enter/src/softbreak.

Additional information

Used the enter plugin as a template to build out the implementation of the soft break feature.
The enter plugin will not respond to an enter keypress if event.shiftKey is truthy.
The soft break plugin adds br tags to the writer view using typical conversion definitions. The model element name is also br.
The view downcaster will create an empty element for the view. However, not a self-closing tag like   but rather an empty  . Being an EmptyElement object it should not be able to receive child nodes.
~~One area where I am not 100% sure of is the schema definition of br. It has been defined as: { isObject: false, isBlock: false, allowWhere: '$text' }. Let me know if this is technically correct.~~
- Changed to: { isObject: true, isBlock: false, allowWhere: '$text' } based on documentation.
Apologies for not following the commit message convention initially.

coveralls · 2018-04-24T03:31:41Z

Coverage remained the same at 100.0% when pulling cec771c on alexeckermann:soft-breaks into 90ce459 on ckeditor:master.

alexeckermann · 2018-04-24T03:32:45Z

This PR addresses #2

Reinmar · 2018-05-04T13:53:39Z

In https://github.com/ckeditor/ckeditor5-enter/issues/2#issuecomment-386607029 I described some changes that we need to make in this plugin to introduce the soft line break feature.

@alexeckermann I'm not sure you're willing to give it a try because the scope grew significantly. If not, @pomek can take it over from here.

Reinmar · 2018-05-04T13:56:39Z

PS. Amazing job, @alexeckermann with all these changes. It's a good start for the rest of the changes.

Reinmar · 2018-05-04T14:09:14Z

The view downcaster will create an empty element for the view. However, not a self-closing tag like   but rather an empty  . Being an EmptyElement object it should not be able to receive child nodes.

That's a very good point. Could elementToElement() support that, @scofalik?

scofalik

At the moment, neither Conversion#elementToElement nor downcast-converters.downcastElementToElement support this. However changing that might not be difficult. We would have to add something like view.ElementDefinition#type which could be container|attribute|ui. Then it would be used in downcast-converters.downcastElementToElement.

scofalik · 2018-05-09T11:14:10Z

src/softbreak.js

+ */
+
+/**
+ * @module enter/enter


Shouldn't it be enter/softbreak?

pomek · 2018-05-15T13:25:51Z

@alexeckermann, because your PR was created before the @Reinmar's comment (https://github.com/ckeditor/ckeditor5-enter/issues/2#issuecomment-386607029), I introduced some commits into your PR that makes the whole PR is ready to review and can be merged.

I found some bug with the soft enter but I guess it's not related to our work but some mechanism in the engine fails. I will report that in the engine.

pomek · 2018-05-16T09:59:05Z

I found some bug with the soft enter but I guess it's not related to our work but some mechanism in the engine fails. I will report that in the engine.

Reported in ckeditor/ckeditor5#1024.

…that's where we should add the ShiftEnter plugin.

Reinmar · 2018-06-04T09:45:35Z

src/shiftentercommand.js

+// @param {module:engine/model/schema~Schema} schema
+// @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
+function isEnabled( schema, selection ) {
+	if ( selection.rangeCount > 1 ) {


First of all – there are no tests of ShiftEnterCommand#isEnabled. There should be. Those tests should explain the logic in this function.

The other things is – why is this command disabled on multiple ranges? I guess you did that on purpose to simplify it and it makes sense to do that, but that requires a comment in the code (because, in the future, we'll need to change that).

Reinmar · 2018-06-04T10:14:12Z

src/shiftentercommand.js

+	const endElement = range.end.parent;
+
+	// If the selection contains at least two elements and one of them is the limit element, the soft enter shouldn't be enabled.
+	if ( ( schema.isLimit( startElement ) || schema.isLimit( endElement ) ) && startElement !== endElement ) {


This logic is incomplete. Selection can be deeply nested inside limit elements. E.g.:

<limit> <paragraph>X[Y</paragrap> </limit> <paragraph>Z]A</paragraph>

Besides, I'm not sure whether this command must be disabled in this case. It can be because cross-limit selections are (most likely) to be banned at all. But in any case, it's good to explain such decisions.

Reinmar · 2018-06-04T10:26:01Z

@pomek, I left you a couple of comments. Other than that, this PR is ready.

Reinmar · 2018-06-07T20:22:41Z

Please add one more test that will verify that the ShiftEnter plugin adds the enter observer. You can make it more a unit test by checking directly if the enter observer is available. Or integration test which checks this by firing a keydown event and checking if shiftEnter was executed.

This test is important because it's not that clear that ShiftEnter should be independent from the Enter plugin. If only the latter would register the observer, ShiftEnter would not work when loaded alone.

Reinmar · 2018-06-07T21:29:36Z

Let's finish this PR tomorro, @pomek because it blocks ckeditor/ckeditor5-engine#1430 too.

pomek · 2018-06-08T12:56:37Z

@Reinmar, I added missing tests. Could you check it once again? I also fixed an issue related to limit elements as you mentioned.

Reinmar · 2018-06-11T09:45:16Z

tests/shiftentercommand.js

@@ -215,4 +215,94 @@ describe( 'ShiftEnterCommand', () => {
 			} );
 		}
 	} );
+
+	describe( '#isEnabled', () => {
+		test( 'should be disabled if $text cannot be inserted into element',


Please don't create such test() functions when not really necessary. And in this case it doesn't change much – from 5LOC to 4LOC ;)

Reinmar · 2018-06-11T09:46:51Z

tests/shiftenter.js

@@ -26,6 +27,12 @@ describe( 'ShiftEnter feature', () => {
 		expect( editor.commands.get( 'shiftEnter' ) ).to.be.instanceof( ShiftEnterCommand );
 	} );

+	it( 'registers the EnterObserver', () => {


To make this test secure and show what it really tests, we should also check that the Enter plugin is not loaded. Then, it'll be completely clear that ShiftEnter should do it on purpose.

Reinmar · 2018-06-11T12:00:46Z

Done! Thanks guys!

alexeckermann · 2018-07-04T12:16:20Z

Thanks to everyone that made this happen! 🍻

alexeckermann added 2 commits April 24, 2018 12:42

Implementation of soft breaks into enter plugin. Usage is optional.

3ac04cc

README update. eslint changes not included in last commit.

148c5d5

alexeckermann added 3 commits April 26, 2018 09:49

Merge branch 'master' into soft-breaks

b328298

Fix: EnterObserver shouldnt respond to soft break key events.

3b32206

Fix: Soft break schema must be defined as a block

6a77cc2

Reinmar requested review from oleq and pomek May 4, 2018 12:25

scofalik assigned scofalik and unassigned scofalik May 9, 2018

scofalik reviewed May 15, 2018

View reviewed changes

src/softbreak.js Outdated

*/

/**

* @module enter/enter

Copy link

Contributor

scofalik May 9, 2018

Choose a reason for hiding this comment

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

Shouldn't it be enter/softbreak?

Kamil Piechaczek added 4 commits May 15, 2018 15:07

EnterObserver passes "isSoft" option in the event object.

fb1d320

Introduced the ShiftEnter plugin.

a21afcc

Introduced the Essentials plugin.

deb0ba2

Updated the MT.

33b6af3

Kamil Piechaczek added 3 commits May 15, 2018 15:26

Code style.

f899536

Added missing devDependencies in package.json.

9b2ad57

Improved the docs. [skip ci]

2782422

pomek pushed a commit that referenced this pull request May 16, 2018

Copy files from #54 to new branch in order to reproduce the issue.

be52eb0

pomek pushed a commit that referenced this pull request May 16, 2018

Copy files from #54 to new branch in order to reproduce the issue.

017eed9

pomek mentioned this pull request May 16, 2018

Cannot type after inserting break line element in a paragraph that ends with a space ckeditor/ckeditor5#1024

Closed

pomek and others added 3 commits May 18, 2018 09:05

Fixed invalid plugin name.

5f3ef34

Docs and comment improvements.

75f2d7b

The Essentials plugin is defined in ckeditor5-essentials package and …

9c42c9b

…that's where we should add the ShiftEnter plugin.

Use more meaningful <softBreak> instand of <break>.

645a128

Reinmar reviewed Jun 4, 2018

View reviewed changes

Minor API docs polishing.

8d1e951

Reinmar added the status:review- label Jun 4, 2018

Merge branch 'master' into soft-breaks

74cc5b2

Reinmar mentioned this pull request Jun 7, 2018

Fixed view <-> DOM conversion of whitespaces around elements ckeditor/ckeditor5-engine#1430

Merged

Reinmar mentioned this pull request Jun 7, 2018

Plain text to HTML conversion removes linebreaks ckeditor/ckeditor5#766

Closed

Added missing tests.

7c2afe9

pomek removed the status:review- label Jun 8, 2018

Code style.

2eb8708

Reinmar reviewed Jun 11, 2018

View reviewed changes

Reinmar added 4 commits June 11, 2018 12:33

Removed an unnecessary helper function.

f49dc6f

Wording.

bc0d221

Explained one decision and clarified the code.

089fc14

Docs: Fixed link.

cec771c

Reinmar merged commit 0181bbf into ckeditor:master Jun 11, 2018

Reinmar mentioned this pull request Oct 9, 2019

Add ShiftEnter plugin once its stabilised ckeditor/ckeditor5#2326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Soft Break support #54

Soft Break support #54

alexeckermann commented Apr 24, 2018 •

edited

Loading

coveralls commented Apr 24, 2018 •

edited

Loading

alexeckermann commented Apr 24, 2018

Reinmar commented May 4, 2018

Reinmar commented May 4, 2018

Reinmar commented May 4, 2018

scofalik left a comment

scofalik May 9, 2018

pomek commented May 15, 2018

pomek commented May 16, 2018

Reinmar Jun 4, 2018

Reinmar Jun 4, 2018

Reinmar commented Jun 4, 2018

Reinmar commented Jun 7, 2018

Reinmar commented Jun 7, 2018

pomek commented Jun 8, 2018

Reinmar Jun 11, 2018

Reinmar Jun 11, 2018 •

edited

Loading

Reinmar commented Jun 11, 2018

alexeckermann commented Jul 4, 2018 •

edited

Loading

Soft Break support #54

Soft Break support #54

Conversation

alexeckermann commented Apr 24, 2018 • edited Loading

Usage

Additional information

coveralls commented Apr 24, 2018 • edited Loading

alexeckermann commented Apr 24, 2018

Reinmar commented May 4, 2018

Reinmar commented May 4, 2018

Reinmar commented May 4, 2018

scofalik left a comment

Choose a reason for hiding this comment

scofalik May 9, 2018

Choose a reason for hiding this comment

pomek commented May 15, 2018

pomek commented May 16, 2018

Reinmar Jun 4, 2018

Choose a reason for hiding this comment

Reinmar Jun 4, 2018

Choose a reason for hiding this comment

Reinmar commented Jun 4, 2018

Reinmar commented Jun 7, 2018

Reinmar commented Jun 7, 2018

pomek commented Jun 8, 2018

Reinmar Jun 11, 2018

Choose a reason for hiding this comment

Reinmar Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Reinmar commented Jun 11, 2018

alexeckermann commented Jul 4, 2018 • edited Loading

alexeckermann commented Apr 24, 2018 •

edited

Loading

coveralls commented Apr 24, 2018 •

edited

Loading

Reinmar Jun 11, 2018 •

edited

Loading

alexeckermann commented Jul 4, 2018 •

edited

Loading