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

Partial mentions should not be downcasted (not upcasted, like in the docs) #4633

Closed
Reinmar opened this issue Apr 2, 2019 · 6 comments · Fixed by ckeditor/ckeditor5-mention#76
Assignees
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 2, 2019

Part of the documentation:

function MentionCustomization( editor ) {
	// The upcast converter will convert <a class="mention"> elements to the model 'mention' attribute.
	editor.conversion.for( 'upcast' ).elementToAttribute( {
		view: {
			name: 'a',
			key: 'data-mention',
			classes: 'mention',
			attributes: {
				href: true,
				'data-user-id': true
			}
		},
		model: {
			key: 'mention',
			value: viewItem => {
				// Optional: Do not convert partial mentions.
				if ( !isFullMention( viewItem ) ) {
					return;
				}

				// The mention feature expects that mention attribute value in the model is a plain object:
				const mentionValue = {
					// The name attribute is required by mention editing.
					name: viewItem.getAttribute( 'data-mention' ),
					// Add any other properties as required.
					link: viewItem.getAttribute( 'href' ),
					id: viewItem.getAttribute( 'data-user-id' )
				};

				return mentionValue;
			}
		},
		converterPriority: 'high'
	} );

	function isFullMention( viewElement ) {
		const textNode = viewElement.getChild( 0 );
		const dataMention = viewElement.getAttribute( 'data-mention' );

		// Do not parse empty mentions.
		if ( !textNode || !textNode.is( 'text' ) ) {
			return false;
		}

		const mentionString = textNode.data;

		// Assume that mention is set as marker + mention name.
		const name = mentionString.slice( 1 );

		// Do not upcast partial mentions - might come from copy-paste of partially selected mention.
		return name == dataMention;
	}

IMO, partial mentions should never end up in the data (any sort of data – also the data clipboard). No one should worry about that except us in the mention feature's code.

@Reinmar
Copy link
Member Author

Reinmar commented Apr 2, 2019

I'm removing this whole bit from the docs.

@jodator
Copy link
Contributor

jodator commented Apr 2, 2019

This might be obsolete after ckeditor/ckeditor5-mention#17. To be checked.

@jodator jodator self-assigned this May 29, 2019
@jodator
Copy link
Contributor

jodator commented May 29, 2019

So the Mention allows to downcast part of mention:
This is copied to clipboard:

<meta http-equiv="content-type" content="text/html; charset=utf-8">
Hello <span class="mention" data-mention="@Ted">@Te</span>

@Reinmar
Copy link
Member Author

Reinmar commented May 29, 2019

What do you mean? Do you describe an expected or actual behaviour before or after that PR? :D

@jodator
Copy link
Contributor

jodator commented May 29, 2019

It is current behavior (copy partial mention) and is fixed on PR (will not copy partial mention).

I've added this comment because in my previous one I thought that ckeditor/ckeditor5-mention#17 might fix that issue. I've checked this and it is still present (you can copy partial mention on current master).

@jodator
Copy link
Contributor

jodator commented May 29, 2019

One note in some earlier works we concluded that partial mention from data will be converter as-is. So it might be a case for a partial mention. We did this because we're not able to verify if the mention is partial when we upcast it to the model.

We could do this asynchronously using feed callback and verify each mention if it is rendered as a whole mention. I don't recall who was proposing this (maybe it was @scofalik or @pjasiun) but we could do conversion asynchronously (feed might be asynchronous).

Just to be clear :)

oleq referenced this issue in ckeditor/ckeditor5-mention Jun 26, 2019
Fix: Partial mentions should not be downcasted (e.g. not copied to clipboard). Closes #24.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-mention Oct 9, 2019
@mlewand mlewand added this to the iteration 25 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature. package:mention labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants