T/884 Mapper: default position mapping algorithms may crash editor #885
Conversation
…stom callbacks did not provide a result.
…ns that do not exist in model.
src/conversion/mapper.js
Outdated
| mapper: this | ||
| }; | ||
|
|
||
| this.fire( 'viewToModelPosition', data ); | ||
|
|
||
| return data.modelPosition; | ||
| return data.modelPosition ? data.modelPosition : this._defaultToModelPosition( viewPosition ); |
There was a problem hiding this comment.
Can't we put this._defaultToModelPosition( viewPosition ) as an viewToModelPosition event listener? Someone may need a mapped position to calculate his version.
There was a problem hiding this comment.
In fact I was curious how such custom mapping may look like, how to create custom mapping without the result of the default mapping, and I realised there is no single test for this change.
There was a problem hiding this comment.
Can't we put this._defaultToModelPosition( viewPosition ) as an viewToModelPosition event listener?
We can, it was like this for some time some time ago.
In fact I was curious how such custom mapping may look like, how to create custom mapping without the result of the default mapping, and I realised there is no single test for this change.
I don't understand what exactly you mean. There are test for adding a custom callback and there are tests for default mapping. What exactly do you need?
There was a problem hiding this comment.
We can, it was like this for some time some time ago.
I wanted to say in the ticket that I remember that it was already possible to add the custom listener before the default mapping and I don't know why it's not possible anymore, but I thought it was a dream ;)
There was a problem hiding this comment.
There are test for adding a custom callback and there are tests for default mapping. What exactly do you need?
Could you link where are these tests, because I may miss something.
There was a problem hiding this comment.
I wanted to say in the ticket that I remember that it was already possible to add the custom listener before the default mapping and I don't know why it's not possible anymore, but I thought it was a dream ;)
Yes, it was like this for a while but then it got changed. I don't remember the exact reasons, though. I can change it so the default mapping callback is fired with low priority. Not a big deal.
Could you link where are these tests, because I may miss something.
I meant these:
https://github.com/ckeditor/ckeditor5-engine/blob/master/tests/conversion/mapper.js#L209-L223
https://github.com/ckeditor/ckeditor5-engine/blob/master/tests/conversion/mapper.js#L282-L296
I haven't added new tests because, from API point of view, in fact nothing changed, other than the fact that you don't get "default" position in the callback.
There was a problem hiding this comment.
I see that there were tests. As I said I was curious how such custom converter may look like if I do not have access to the default mapping. Tests is the place where I look for the sample use-case and I did not find any example how I could use this event.
There was a problem hiding this comment.
So, you mean something that resembles real case scenario. Well, the problem with such tests is that you need to think of some non-dumb case/feature and then code it. So you either go easy way (like the linked tests) or start re-writing other feature (i.e. List). Maybe it would be better to drop some reference in the event description (to List feature?).
There was a problem hiding this comment.
BTW. I just noticed that there's example in event's description.
There was a problem hiding this comment.
BTW. I just noticed that there's example in event's description.
I see. That's fine then.
| const viewPosition = conversionApi.mapper.toViewPosition( data.sourcePosition ); | ||
| const viewRange = data.item.is( 'element' ) ? | ||
| ViewRange.createOn( viewPosition.nodeAfter ) : | ||
| _expandViewPositionOnText( viewPosition, data.item.offsetSize ); |
|
Fixed. |
Suggested merge commit message (convention)
Other: Default
conversion.Mapperposition mapping algorithms are now added as callbacks withlowpriority and are fired only if earlier callbacks did not provide a result. Closes ckeditor/ckeditor5#4025.BREAKING CHANGES: Since default position mapping algorithms are attached with
lowpriority, custom position mapping callbacks added with higher priority won't receive position calculated by default algorithms indata. To execute default position mapping algorithms and use their value, hook custom callback with lower priority.