Introduced Title plugin #130

oskarwrobel · 2019-08-28T13:20:12Z

Suggested merge commit message (convention)

Feature: Introduced Title plugin. Closes ckeditor/ckeditor5#2003. Closes ckeditor/ckeditor5#2005.

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

oskarwrobel · 2019-08-28T13:30:27Z

I'm wondering if we should disable heading command when selection is in the title element?

The document title is always represented as a <title /> element in the model. It's not possible to change it into the other block element (post-fixer will always fix such a change).

Is it possible to set some shema rules for that? Tell schema that title element cannot be renamed?

ckeditor/ckeditor5#2005

pjasiun · 2019-08-29T15:36:38Z

Make sure that title integrates well with the placeholder support (https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/editor-placeholder.html) I think that placeholder if defined should be the placeholder for the body element. If not defined you should use "Body" as a default option.

Mgsy · 2019-09-02T10:25:31Z

I'm wondering if we should disable heading command when selection is in the title element?

I think it should be disabled. During testing, I noticed that heading dropdown is active and I tried to convert the title into a heading/paragraph, so for me it indicates that if it's active, I can do something with it.

oskarwrobel · 2019-09-02T10:30:16Z

I think it should be disabled. During testing, I noticed that heading dropdown is active and I tried to convert the title into a heading/paragraph, so for me it indicates that if it's active, I can do something with it.

Yep, I'm working on it.

…oll it by schema.

oskarwrobel · 2019-09-02T11:44:50Z

Known issue ckeditor/ckeditor5#2010

Mgsy · 2019-09-02T14:14:05Z

Steps to reproduce

Copy two block elements.
Paste them at the beginning of the title.
Undo.

Current result

The editor crashes.

Error

ckeditorerror.js:66 Uncaught CKEditorError: model-createPositionAt-offset-required: Model#createPositionAt() requires the offset when the first parameter is a model item. Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-model-createPositionAt-offset-required

    at Function._createAt (http://localhost:8125/ckeditor5-core/tests/manual/article.js:59326:11)
    at new Range (http://localhost:8125/ckeditor5-core/tests/manual/article.js:59526:66)
    at LiveRange._getTransformedByMove (http://localhost:8125/ckeditor5-core/tests/manual/article.js:60259:17)
    at LiveRange._getTransformedByMoveOperation (http://localhost:8125/ckeditor5-core/tests/manual/article.js:60041:15)
    at LiveRange.getTransformedByOperation (http://localhost:8125/ckeditor5-core/tests/manual/article.js:59935:17)
    at LiveRange.transform (http://localhost:8125/ckeditor5-core/tests/manual/article.js:51404:22)
    at Model.listenTo.priority (http://localhost:8125/ckeditor5-core/tests/manual/article.js:51392:14)
    at Model.fire (http://localhost:8125/ckeditor5-core/tests/manual/article.js:114391:29)
    at Model.<computed> [as applyOperation] (http://localhost:8125/ckeditor5-core/tests/manual/article.js:116520:16)
    at UndoCommand._undo (http://localhost:8125/ckeditor5-core/tests/manual/article.js:109329:11)

GIF

jodator · 2019-09-02T16:30:59Z

@oskarwrobel - from my manual tests

should The Alignment feature be enabled in the title? (I do not have preference in that matter - looks like it's OK to have it).
the mention plugin gets invoked inside title (might be bug of a mention plugin)

jodator

I found some things to iron out - to not block you I'll R- now and will check the tests later.

jodator · 2019-09-02T16:33:08Z

src/title.js

+} from '@ckeditor/ckeditor5-engine/src/view/placeholder';
+
+// List of model elements that are allowed to be renamed to title element.
+const allowedToBeTitle = new Set( [ 'heading1', 'heading2', 'heading3', 'paragraph' ] );


Missing heading4...6.

jodator · 2019-09-02T16:38:09Z

docs/features/title.md

+
+## Keyboard navigation
+
+Title plugin lets you navigate between title and body elements using <kbd>Tab</kbd> key and back, using <kbd>Shift</kbd> + <kbd>Tab</kbd>, providing form-like experience. You can also use <kbd>Enter</kbd> and <kbd>Backspace</kbd> keys to move caret between title and body.


Doesn't work like that if the body has content and selection is not at the start:

Docs should be more precise. Shift+Tab works only when selection is at the beginning of the "body" section.

Please add this explanation to the docs.

docs/features/title.md

jodator · 2019-09-02T16:49:32Z

src/title.js

+
+		model.schema.addChildCheck( ( context, childDefinition ) => {
+			// Allow `title` element only directly inside a root.
+			if ( childDefinition.name === 'title' && !context.endsWith( '$root' ) ) {


Shouldn't this be handled by: allowIn: '$root' in schema.register()?It bloats this check - it wis for two different items and the if below is already "big".

I was afraid it won't disallow title element inside a blockQuote (because of inheritAllFrom: '$block'). Works fine after the change 👌

jodator · 2019-09-02T16:51:29Z

src/title.js

+			// Allow only `title-content` inside `title`.
+			if (
+				context.endsWith( 'title' ) && childDefinition.name !== 'title-content' ||
+				childDefinition.name === 'title-content' && !context.endsWith( 'title' )


Isn't this second check covered by schema definition allowIn?

I've checked the tests with:

model.schema.addChildCheck( ( context, childDefinition ) => { // Allow only `title-content` inside `title`. if ( context.endsWith( 'title' ) && childDefinition.name !== 'title-content' ) { return false; } } );

and only schema rules test fails so either we can simplify this or add more tests.

ps.: I didn't read the tests yet ;)

Ugh.. I was wrong because of inheritAllFrom there. So Now I'm not sure if we shouldn't make this more specific in the first place? Either by inheriting only those required options or by manually defining the title and title-content with full schema.

It looks like more straight-forward then allowing all there and then adding callback to check only those allowed scenarios.

You are right. I'll define title and title-content manually.

jodator · 2019-09-02T17:00:09Z

src/title.js

+		const view = editor.editing.view;
+		const viewRoot = view.document.getRoot();
+
+		const bodyPlaceholder = editor.config.get( 'placeholder' ) || 'Body';


I'll check this but I feel that we should make those strings ('Title' & 'Body') translate-able.

No doubts. I'll change it.

jodator · 2019-09-02T17:05:20Z

src/title.js

+	_getTitleElement() {
+		const root = this.editor.model.document.getRoot();
+
+		return Array.from( root.getChildren() ).find( element => element.is( 'title' ) );


This will return undefined not null if element is not found in the array.

Good catch.

jodator · 2019-09-02T17:07:03Z

src/title.js

+	 * @returns {String} Title of the document.
+	 */
+	getTitle() {
+		const title = this.editor.model.document.getRoot().getChild( 0 ).getChild( 0 );


I know that we have a post-fixer but I would feel safer with using getTitleElement(), WDYT?

Or OTOH - maybe add a comment about that post-fixer takes care of this.

hm..... I was thinking about using getTitleElement() but finally decided to do it as it is. getTitleElement() it's an additional call of Array.from and Array.find. As you wrote, post-fixer takes care of this structure.

I changed this.

jodator · 2019-09-02T17:13:11Z

src/title.js

+		const titleChildren = Array.from( title.getChildren() );
+		let hasChanged = false;
+
+		titleChildren.shift();


Maybe a comment about preserving of the first child? (I wonted to write firstborn 😆)

jodator · 2019-09-02T17:15:11Z

tests/manual/title.md

@@ -0,0 +1,28 @@
+## Title feature
+
+- you should see `Title` and `Body` placeholders when there is no text in any of sections.


Suggested change

- you should see `Title` and `Body` placeholders when there is no text in any of sections.

- you should see `Title` and `Body` placeholders when there is no text in any of the sections.

…ist to a static class property.

oskarwrobel · 2019-09-02T21:02:15Z

the mention plugin gets invoked inside title (might be bug of a mention plugin)

I think mention balloon should be not displayed in the text where mention attribute is disallowed.

oskarwrobel · 2019-09-03T06:51:22Z

should The Alignment feature be enabled in the title? (I do not have preference in that matter - looks like it's OK to have it).

It's enabled now.

oskarwrobel · 2019-09-03T07:54:10Z

Steps to reproduce

Copy two block elements.

Paste them at the beginning of the title.

Undo.

Current result

The editor crashes.

I fixed this scenario however this error is still reproducible.

Copy 2 blocks (paragraph and list item).
Paste them at the beginning of the title.
Undo.

I hope I'm wrong but it can be a bug in OT.

[Edit]

I was debugging this for a while and I found the way to prevent this exception but this is something for the exorcist @scofalik ckeditor/ckeditor5#2022.

jodator

I've found that one case wasn't tested (defined). Als I would take a different approach in the post fixer as it si a quite big.

src/title.js

jodator · 2019-09-04T11:36:48Z

docs/features/title.md

+
+## Keyboard navigation
+
+Title plugin lets you navigate between title and body elements using <kbd>Tab</kbd> key and back, using <kbd>Shift</kbd> + <kbd>Tab</kbd>, providing form-like experience. You can also use <kbd>Enter</kbd> and <kbd>Backspace</kbd> keys to move caret between title and body.


Please add this explanation to the docs.

src/title.js

jodator · 2019-09-04T12:46:59Z

src/title.js

+				const selection = model.document.selection;
+				const selectedElements = Array.from( selection.getSelectedBlocks() );
+
+				if ( selectedElements.length === 1 && selectedElements[ 0 ].name === 'title-content' ) {


selectedElements[ 0 ].is( 'title-content' )

src/title.js

oskarwrobel added 2 commits August 28, 2019 15:09

Introduced Title plugin.

82fd357

Added missing dev deps.

c04659f

oskarwrobel added 3 commits August 28, 2019 17:01

Added missing dev deps.

b3bf397

Fixed invalid import path.

ee9467e

Disallow attributes in title.

8e2639f

Piotr Jasiun added 3 commits August 30, 2019 16:12

Docs: title feature guide and snippet.

4a19242

Remove broken links.

4520804

Small fixed in docs.

5cc7883

jodator self-assigned this Sep 2, 2019

oskarwrobel added the status:review- label Sep 2, 2019

Changed model structure of title element to make it possible to contr…

6e5ce6b

…oll it by schema.

oskarwrobel removed the status:review- label Sep 2, 2019

oskarwrobel added 4 commits September 2, 2019 13:47

Added missing dev dependency.

4a85217

Added integration with EditorConfig#placeholder.

21d1ec4

Removed Enter from Title requienments.

8099525

Minor improvements.

bf6a252

jodator suggested changes Sep 2, 2019

View reviewed changes

oskarwrobel added 6 commits September 2, 2019 20:32

Added missing headings to the list title-like elements. Changed the l…

9d70d35

…ist to a static class property.

Simplified schema rules.

475c8f6

Minor improvements.

d03c323

Used translation service for placeholders.

93f629a

Docs.

65c2ffd

Improved code readability.

3865496

oskarwrobel added 2 commits September 2, 2019 21:16

Added comment.

32b7540

Minor manual test improvements.

14fee6d

oskarwrobel requested a review from jodator September 2, 2019 19:18

oskarwrobel mentioned this pull request Sep 2, 2019

Configurable placeholder for Title section in title plugin ckeditor/ckeditor5#2020

Closed

Enabled alignment feature.

5b0fec7

oskarwrobel added 2 commits September 3, 2019 08:51

Add missing dev dependency.

c03ba99

Handle multiple non title-like elements pasted before the title.

b015f2b

oskarwrobel and others added 5 commits September 3, 2019 15:32

Filter out disallowed attributes from title-content element.

00176c3

Tests improvements.

0514a88

Title.titleLikeElements docs.

3d61f01

Add explanatory comments.

25d2e3b

Added test case for existing title with title-like elements in the data.

8ab1e9e

jodator reviewed Sep 4, 2019

View reviewed changes

jodator added the status:review- label Sep 4, 2019

Simplified post-fixers.

d587b39

oskarwrobel force-pushed the t/ckeditor5/2003 branch from 296820c to 454325d Compare September 4, 2019 22:19

oskarwrobel and others added 4 commits September 5, 2019 00:20

Used first() util to get first selected block.

454325d

Minor giude improvement.

0df59cd

Fix post-fixer that fixes extra paragraphs name.

d12b0cd

Docs wording fixup.

fbba254

jodator approved these changes Sep 5, 2019

View reviewed changes

jodator added status:review+ and removed status:review- labels Sep 5, 2019

jodator merged commit a65c0ac into master Sep 5, 2019

jodator deleted the t/ckeditor5/2003 branch September 5, 2019 08:35

jodator mentioned this pull request Oct 9, 2019

Mention view UI opens when user selection is in position where mention is disallowed. ckeditor/ckeditor5#4670

Closed


		## Keyboard navigation

		Title plugin lets you navigate between title and body elements using <kbd>Tab</kbd> key and back, using <kbd>Shift</kbd> + <kbd>Tab</kbd>, providing form-like experience. You can also use <kbd>Enter</kbd> and <kbd>Backspace</kbd> keys to move caret between title and body.

		@@ -0,0 +1,28 @@
		## Title feature

		- you should see `Title` and `Body` placeholders when there is no text in any of sections.

Introduced Title plugin #130

Introduced Title plugin #130

Conversation

oskarwrobel commented Aug 28, 2019 • edited Loading

Suggested merge commit message (convention)

Additional information

oskarwrobel commented Aug 28, 2019 • edited Loading

pjasiun commented Aug 29, 2019

Mgsy commented Sep 2, 2019

oskarwrobel commented Sep 2, 2019

oskarwrobel commented Sep 2, 2019

Mgsy commented Sep 2, 2019 • edited Loading

Steps to reproduce

Current result

Error

GIF

jodator commented Sep 2, 2019

jodator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel commented Sep 2, 2019 • edited Loading

oskarwrobel commented Sep 3, 2019

oskarwrobel commented Sep 3, 2019 • edited Loading

Steps to reproduce

Current result

jodator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel commented Aug 28, 2019 •

edited

Loading

oskarwrobel commented Aug 28, 2019 •

edited

Loading

Mgsy commented Sep 2, 2019 •

edited

Loading

oskarwrobel commented Sep 2, 2019 •

edited

Loading

oskarwrobel commented Sep 3, 2019 •

edited

Loading