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

Allow defining target in attributeToAttrbibute conversion helpers #2061

Closed
Reinmar opened this issue Sep 19, 2019 · 7 comments
Closed

Allow defining target in attributeToAttrbibute conversion helpers #2061

Reinmar opened this issue Sep 19, 2019 · 7 comments
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. status:stale type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@Reinmar
Copy link
Member

Reinmar commented Sep 19, 2019

Typical problem – model and view structures don't match one another. Example:

model:
<image src="foo"></image>

view:
<figure class="image">
    <img src="foo">
</figure>

If you want to write a converter for let's say an id attribute that should be added on <img> a simple use of attributeToAttribute() will not work as expected, because the attribute will end up in the <figure>. That's because <image> is mapped to <figure>.

It's possible to work around this by using lower-level, event-based mechanism. You can find the solution here: https://github.com/ckeditor/ckeditor5-core/compare/poc/customfigureattributes

However, I think we can easily resolve this by adding viewTarget property:

editor.conversion.for( 'downcast' ).attributeToAttribute( {
	model: 'id',
	view: 'id',
	viewTarget: viewElement => {
		const imgViewElement = findImgElement( viewElement );

		return imgViewElement;
	}
} );

I'm not sure if upcast conversion requires a similar mechanism, but it's worth checking.

@Reinmar Reinmar added package:engine status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). labels Sep 19, 2019
@Reinmar Reinmar added this to the next milestone Sep 19, 2019
@Reinmar
Copy link
Member Author

Reinmar commented Sep 19, 2019

cc @jodator @scofalik

@Reinmar
Copy link
Member Author

Reinmar commented Sep 19, 2019

BTW, another addition to the conversion API that people are missing is #1589.

@jodator
Copy link
Contributor

jodator commented Sep 19, 2019

I wonder how this API could look like and if we should somehow restrict it.

Let's assume someone would go crazy and convert this id to id on some random node in the tree above current element just because it's doable (not a real problem TBH - but read below).

Now if we assume that the intended way of using this API is to point to a know inner child we might simplify it by:

  1. providing a Matcher compatible object - to be used internally to find this element
  2. providing callback that should return true/false
  3. providing a callback that return the element

The idea behind 1/2 is such:

editor.conversion.for( 'downcast' ).attributeToAttribute( {
	model: 'id',
	view: 'id',
// 1.
	viewTarget: 'img'
// or 1.
	viewTarget: {
		name: 'img',
		classes: 'foo bar' // or whatever the API is...
	}
// or 2.
	viewTarget: viewChild => viewChild.is( 'img' )
} );

I think that such API might be a bit simpler and in line with other options that conversion API provides for defining view elements. Internally this could be a treewalker that iterates over the viewElement children (whole tree) (the mapped view element from the model element that holds this attribute).

The only problem I see in the above is how to restrict the depth of a search? Immediate children solves only figure > img problem but CKEditor allows theoretically to create deeper structrues. But, OTOH, I don't recall what would be available as view children - only a view structure from elementToElement() convertions or does the children will be converted also? If it's the first thing (so withouth model's children converted) the this would play nice.


Bottom line it looks like simple addition and I don't see potential risks but @scofalik may have additonal points to make here.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 25, 2020

This issue affects #6325.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 25, 2020

I wonder how this API could look like and if we should somehow restrict it.

Callback which gets the parent element and lets you return whatever you want. No more magic and no more options.

All other options, such as flat or deep search with mapper or a callback should be available in the view element API. This way these methods will be reusable in other places.

@pomek pomek modified the milestones: next, nice-to-have Nov 10, 2020
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 5, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. status:stale type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

5 participants