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

Why is h1 element not supported? #2443

Closed
Reinmar opened this issue Apr 28, 2017 · 11 comments · Fixed by ckeditor/ckeditor5-heading#92
Closed

Why is h1 element not supported? #2443

Reinmar opened this issue Apr 28, 2017 · 11 comments · Fixed by ckeditor/ckeditor5-heading#92
Labels
package:heading status:discussion type:question This issue asks a question (how to...).

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 28, 2017

Currently, when you try to load <h1> into the editor (e.g. by setData(), but also by pasting), then it will be converted to a paragraph.

This happens because the heading feature is by default configured to handle h2-h4 elements only ("Heading 1" means <h2>, "Heading 2" means <h3> and so on). You can find the default config here: https://github.com/ckeditor/ckeditor5-heading/blob/74559bb09f97c9272ba3c19ee2599fbd56b0b5e9/src/headingengine.js#L34-L41. And you can, of course, pass a different configuration to Editor.create() – one which will also handle <h1> elements:

ClassicEditor.create( element, {
	heading: {
		options: [
			{ modelElement: 'paragraph', title: 'Paragraph', class: 'ck-heading_paragraph' },
			{ modelElement: 'heading1', viewElement: 'h1', title: 'Heading 1', class: 'ck-heading_heading1' },
			{ modelElement: 'heading2', viewElement: 'h2', title: 'Heading 2', class: 'ck-heading_heading2' },
			{ modelElement: 'heading3', viewElement: 'h3', title: 'Heading 3', class: 'ck-heading_heading3' }
		]
	}
} )
.then( ( editor ) => {

} )
.catch( ( error ) => {

} );

Why doesn't the heading feature handle <h1>? Why does "Heading 1" mean <h2>?

You can find the answer in Editor recommendations:

In the vast majority of use cases, the content created using the editor would be a part of a page, therefore usage of <h1> element is highly unrecommended. However if the content of the editor would be presented in an independent form, <h1> element MAY be used. Implementer MAY think of naming this level of heading in a more distinguishable way e.g. “Title”.

That's why the default setting is to start with <h2> which means that <h1> features are not handled by the heading feature so they get converted to <p>.

Further considerations

When pasting content into the editor you will, currently, lose all <h1> elements. I wonder whether it wouldn't be better if they were converted to <h2> automatically. That may be still confusing for developers (that's why I decided to create this ticket), but at least it will be more useful from the user perspective.

WDYT?

@deanvaessen
Copy link

deanvaessen commented Apr 29, 2017

I'm trying to implement it but getting this error in console:

documenteditor.js:929 CKEditorError: model-schema-item-exists: Item with specified name already exists in schema.

My whole code:

try {
	ClassicEditor.create(document.querySelector('#ckeditor'), {
		heading : {
			options: [
				{ modelElement : 'paragraph', title: 'Paragraph', class : 'ck-heading_paragraph' },
				{ modelElement : 'heading1', viewElement : 'h1', title : 'Heading 1', class : 'ck-heading_heading1' },
				{ modelElement : 'heading2', viewElement : 'h2', title : 'Heading 2', class : 'ck-heading_heading2' },
				{ modelElement : 'heading3', viewElement : 'h3', title : 'Heading 3', class : 'ck-heading_heading3' }
			]
		 },
		plugins : [
				//Autoformat,
				//ArticlePreset,
				Enter,
				Headings,
				Typing,
				Paragraph,
				Undo,
				Bold,
				Italic,
				Image,
				Link,
				List,
				Submit,
				ToMarkdown,
				Clipboard
			],
			toolbar : [
				'headings',
				'bold',
				'italic',
				'link',
				'unlink',
				'bulletedList',
				'numberedList',
				'undo',
				'redo',
				'submit',
				'toMarkdown',
			]
	}).then((editor) => {
		// Make it available to the rest of the app
		window.swarmagent.editor.engine = editor;
		documentEditor = editor;

		// Set a welcome text
		documentEditor.setData('<h2>What would you like to share?</h2>');
	})
	.catch((err) => {
		console.error(err.stack);
	});
} catch (err) {
	showCompatibilityMessage();
}

@Reinmar
Copy link
Member Author

Reinmar commented Apr 29, 2017

Mmm... We have a test for this, but I can see the test is poor. It does use a different schema name (model name) than any of the default ones, so there's no collision.

OTOH, I can see that the code looks good... I need to check it deeper.

@Reinmar
Copy link
Member Author

Reinmar commented Apr 29, 2017

This worked for me just fine:

import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classic';
import Enter from '@ckeditor/ckeditor5-enter/src/enter';
import Typing from '@ckeditor/ckeditor5-typing/src/typing';
import Heading from '../../src/heading';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Undo from '@ckeditor/ckeditor5-undo/src/undo';

ClassicEditor.create( document.querySelector( '#editor' ), {
	plugins: [ Enter, Typing, Undo, Heading, Paragraph ],
	toolbar: [ 'headings', 'undo', 'redo' ],
	heading: {
		options: [
			{ modelElement : 'paragraph', title: 'Paragraph', class : 'ck-heading_paragraph' },
			{ modelElement : 'heading1', viewElement : 'h1', title : 'Heading 1', class : 'ck-heading_heading1' },
			{ modelElement : 'heading2', viewElement : 'h2', title : 'Heading 2', class : 'ck-heading_heading2' },
			{ modelElement : 'heading3', viewElement : 'h3', title : 'Heading 3', class : 'ck-heading_heading3' }
		]
	},
} )
.then( editor => {
	window.editor = editor;
} )
.catch( err => {
	console.error( err.stack );
} );

Maybe you need to update your dependencies? Heading package was refactored in 0.9.0 so if you're on 0.8.0 it may not work. Check out the changelog: https://github.com/ckeditor/ckeditor5-heading/blob/master/CHANGELOG.md

@ssougnez
Copy link

Hi,

I think that converting h1 to h2 on copy paste would be more interesting than converting them into paragraph. Whatever the solution you choose, the user will be confused at first:

  • If h1 titles are converted in paragraphs, the user will wonder why his h1 titles are not kept while h2 are.
  • If h1 titles are converted in h2 and so forth, this will also confuse the user but probably a bit less.

Personally, I just experienced the second case, then I ended up quite quickly on this post and understood what happened.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 27, 2017

If h1 titles are converted in h2 and so forth, this will also confuse the user but probably a bit less.

This seems to be a lot better option. Most of the users won't know whether the headings in the original content they copied were h1 or h2. I wouldn't know myself (unless I checked :D).

@ssougnez
Copy link

It has a small impact on styling.

On my page, I have a h1 then I copy/pasted content that contained other h1 and I was surprised that their sizes where smaller than the one of the title :-) I think that's almost the only case noticable by the user. And to be honest, I was pleased by this behaviour, so, for me, it's the best solution ^^

@fredck
Copy link
Contributor

fredck commented Aug 9, 2017

I agree that converting them automatically is a must... but let's make it more complicate, ofc...

Should we sniff the pasted content and, if it has any h1 we must "downgrade" all headings by one (h1->h2, h2->h3, etc...)? I think yes.

Then, if there is no h1, we don't downgrade anything? I think yes, again.

@ssougnez
Copy link

ssougnez commented Aug 9, 2017

For my part, I think it's indeed a good idea.

@wwalc
Copy link
Member

wwalc commented Aug 10, 2017

I agree that converting them automatically is a must... but let's make it more complicate, ofc...

Should we sniff the pasted content and, if it has any h1 we must "downgrade" all headings by one (h1->h2, h2->h3, etc...)? I think yes.

Then, if there is no h1, we don't downgrade anything? I think yes, again.

Sounds reasonable but in some situations might be confusing as well. Suppose you are copying content from the same source. First you copy the beginning of the article, including H1, then you start copying other parts from the same article (e.g. because you want to place it in different order or just want to copy selected parts of the article, because its too long, contains internal notes or sth). As a result one ends up with H2 used in place of "H1" and then H2 used in the middle of content again.

In any case, this is nothing that cannot be fixed manually later, just wanted to point out some downsides.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 27, 2017

In #638 (comment) I proposed a cofig format which would allow everyone to easily configure which elements should be handled by which heading option.

Reinmar referenced this issue in ckeditor/ckeditor5-heading Jan 16, 2018
Enhancement: Added support for `ConverterDefinition` in the heading options configuration. Closes #72. Closes ckeditor/ckeditor5#651.

BREAKING CHANGE: `config.heading.options` format has changed. The valid `HeadingOption` syntax is now compatible with `ConverterDefinition`: `{ model: 'heading1', view: 'h2', title: 'Heading 1' }`.
@Reinmar
Copy link
Member Author

Reinmar commented Jan 16, 2018

Hm... I mistakenly closed this ticket with ckeditor/ckeditor5-heading#92. I reported this ticket to explain why "heading 1" option uses <h2> and how to change the editor configuration to have a different mapping. But, since this is already covered with the docs we can actually close this ticket, so I'll leave it this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:heading status:discussion type:question This issue asks a question (how to...).
Projects
None yet
6 participants