The image uploading listener for handling base64/blob images no longer stops inputTransformation event
#264
Conversation
|
@f1ames I've forgot about this one after checking the images upload. It is still valid right? |
|
Still failing on Edge, due to some reason 🤔 I will look into it. |
|
|
||
| if ( !fetchableImages.length ) { | ||
| return; | ||
| } | ||
|
|
||
| evt.stop(); | ||
| const writer = new UpcastWriter(); |
There was a problem hiding this comment.
Hmm I'm worried about that construct. I think that we could expose those methods on the view if they're need. Tahen we could write it as:
const view = editor.editing.view; // ps.: also used for crateRangeIn()
/// the code as now
view.setAttribute( 'src', '', fetchableImage.imageElement );This unfortunately will require small changes in the engine.
Edit: I've just found this construct in the Clipboard docs. I'll report an issue there. So do not change this ATM.
There was a problem hiding this comment.
ps.: reporter here: https://github.com/ckeditor/ckeditor5-clipboard/issues/46#issuecomment-454025196
There was a problem hiding this comment.
Ok, I'll leave it as is for now. Btw. one may use change block here:
editor.editing.view.change( writer => {
// code...
} );to obtain the writer. However, it uses DowncastWriter not UpcastWriter (still their setAttribute() methods uses exactly same API).
There was a problem hiding this comment.
I was thinking abut it and while it looks safe we probably ought to fix it globally as in referenced ticket.
|
As the code is ready: @Mgsy could you check this one? |
Co-Authored-By: f1ames <dr.odpowiedz@gmail.com>
|
I'm not able to paste an image from Word to the editor in Edge (other browsers work fine and the sample has PFO plugin activated). Tested on VM.
|
That's probably caused by the lack of support for |
So should this be fixed here or in PFO plugin? |
|
☝️ Sorry, didn't make it clear in the previous comment. It should be fixed in this PR from what I can see and I'm working on the fix ATM. |
Mixed up a little bit, we cannot use blob here due to collaboration features (it may break it). So the image simply will not be pasted. However, |
|
Funny that there is no error in Edge in the case @Mgsy found. In Chrome (when file promise is rejected) there is - so I have added a fix there too - ckeditor/ckeditor5-upload#90. Please review and merge it together with this PR. |
After switching to this branch, indeed, the image isn't pasted in Edge, but unfortunately, the |
|
@Mgsy I have pushed a fix. In Edge image should not be pasted, no errors in the console and no alert displayed.
Sorry, it was part of the fix, the second part was pushed in the latest commit (f5a52f3). |
Suggested merge commit message (convention)
Other: The image uploading listener for handling
base64/blobimages no longer stopsinputTransformationevent. Closes ckeditor/ckeditor5#5177. Closes ckeditor/ckeditor5#2530.Additional information
This PR aligns the way
base64/blobimages uploading works to https://github.com/ckeditor/ckeditor5-upload/issues/87 issue in which theFileLoaderacceptsPromiseinstead ofFileinstance.It's nice to see how much
inputTransformationlistener got simplified. However, tests got a little more complex.This PR is based on #258 PR and will not work without ckeditor/ckeditor5-upload#88 so should be closed after closing this mentioned PRs. The CI will be red unless ckeditor/ckeditor5-upload#88 is merged.