Make sure existing picture element is handled correctly on 'sources' downcast #17193
Conversation
DawidKossowski
changed the title
| if ( prevPictureElement ) { | ||
| // Insert a new picture, move img into it, remove previous picture element (ckeditor5#17192). | ||
| viewWriter.insert( viewWriter.createPositionBefore( prevPictureElement ), pictureElement ); | ||
| viewWriter.move( viewWriter.createRangeOn( imgElement ), viewWriter.createPositionAt( pictureElement, 'end' ) ); | ||
| viewWriter.remove( prevPictureElement ); |
There was a problem hiding this comment.
What would happen if other converters downcast other attributes to the picture element? Would those be lost? Maybe in such a case, we should remove source elements from the picture element and reinsert them into the already existing picture element.
There was a problem hiding this comment.
Good point 👍 🤔 So you basically mean to reuse existing picture element by emptying it and adding new elements inside it instead of recreating the whole structure AFAIU. I'll adjust the code then 👍
There was a problem hiding this comment.
Maybe not emptying but removing source elements and inserting new ones (img element should also be reused)
There was a problem hiding this comment.
Fixed in 893b1e3 and even the code go slightly simpler 👍 I had to add a custom attribute handling in test because we don't have anything for picture from what I see.
|
|
||
| viewWriter.remove( viewWriter.createRangeIn( pictureElement ) ); | ||
|
|
||
| // Make sure <picture> does not break attribute elements, for instance <a> in linked images. |
There was a problem hiding this comment.
This comment looks unrelated to the code. It's a more generic comment about collecting attribute elements and reapplying them.
There was a problem hiding this comment.
True, I moved it with the code but it doesn't seem to fit there. Fixed 👍
Suggested merge commit message (convention)
Fix (image): Handle existing picture element correctly on 'sources' downcast. Closes #17192.
Additional information
See #17192.