search

ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.56k stars 3.7k forks source link

Millisecond glitch on image upload #7548

Closed fredck closed 12 months ago

fredck commented 4 years ago

📝 Provide detailed reproduction steps (if any)

  1. In Firefox, go to the base64 upload doc demo.
  2. Upload an image (easer to see with a small one).

✔️ Expected result

Image "uploaded" with no glitches.

❌ Actual result

A perceivable glitch happens.

📃 Other details

I wasn't able to reproduce the problem in Chrome, but it has been reported in GitHub Writer that a similar issue happens in Chrome as well: ckeditor/github-writer#203.

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

Reinmar commented 4 years ago

@fredck, could you test whether this issue occurs when WidgetTypeAround is disabled (you'd need to remove it from the Widget plugin deps)

fredck commented 4 years ago

@fredck, could you test whether this issue occurs when WidgetTypeAround is disabled (you'd need to remove it from the Widget plugin deps)

I've just tried it with a local build of GitHub Writer as it suffers with this issue (ckeditor/github-writer#203). Removing that plugin didn't change anything and the glitch is still there. I confirmed that the plugin is disabled because there was no fancy arrows and horizontal carets anymore around the image.

I wasn't able to test it with the base64 example, as described in this issue, but I believe you guys should be able to easily do so. Thanks!

Reinmar commented 4 years ago

Seems to be related to #7633.

Reinmar commented 4 years ago

7633 makes it quite severe.

oleq commented 4 years ago

FYI: #7633 is a different issue and there's a PR addressing it.

Reinmar commented 4 years ago

Thanks for debugging this, @oleq

What worries me, though, is that two similar issues appeared shortly one after another. It's either Chrome's issue or we changed something and it'd be good to know what was that.

Also, besides image upload I can see that normal image paste (image copied from the editor) is affected too. The resize handles are repositioned with a long delay, even 300ms-500ms. This definitely didn't happen before. And as Fred checked, this is not related to WidgetTypeAround. You can even observe the glitch on this gif:

It'd be good to know WTH is happening there. Bisecting it might actually help a lot.

oleq commented 4 years ago

@fredck Can you post some GIFs? I'm having trouble reproducing your issue. It's either too subtle or there are some external factors involved. Thanks!

Reinmar commented 4 years ago

I also can't reproduce the issue anymore. I saw it previously.

panr commented 4 years ago

There's a 200ms throttle on redrawing the handles after you click on the image - https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-widget/src/widgetresize.js#L90

I had a pretty straightforward fix for this https://github.com/ckeditor/ckeditor5/pull/7654/commits/6c2a01cf6d52b3691cf781ac358140a12ca540a5 but after Olek's changes I decided to remove it (also because it was related to the config.image.disableResizeHandles which we decided to get rid of the scope of the ticket).

Reinmar commented 4 years ago

There should be no delay when the user focuses the image. Throttling makes sense when something happens very frequently. I remember that we added it for performance reasons but that was related to the performance during resizing. Changing how it behaves on image focus must've been coincidental.

Anyway, I'm for changing this – on focus the redraw should be immediate.

Reinmar commented 4 years ago

@oleq pointed out that we can also think about reconfiguring throttle to execute on the leading edge. That'd shorten the initial reaction.

Reinmar commented 4 years ago

So, we have two possible solutions:

fredck commented 4 years ago

Can you post some GIFs? I'm having trouble reproducing your issue. It's either too subtle or there are some external factors involved. Thanks!

Was finally able to do so... Chrome:

Firefox:

This was tested with GitHub Writer after upgrading it to CKEditor 5 v21.0.0.

niegowski commented 4 years ago

This glitch is also happening on the linked images - when the user is removing a link from the image.

panr commented 4 years ago

Unfortunately, I couldn’t reproduce exactly the same glitch on Chrome / Firefox. But I tried to follow the whole process step by step with devtools and debugger and I presume that the issue may be related to the reading status of the file upload. During this stage, an image placeholder is created, but its styling depends on those styles:

image

This means that the placeholder will expand to the whole width of the editor, no matter how wide / big the uploading image is. When we upload a smaller (and "lighter") image — like 200px or so — the placeholder flashes for a moment (with that blue outline and height equals 0 — BUT the placeholder contains .ck-upload-placeholder-loader which is this empty upload placeholder svg file — https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-image/theme/icons/image_placeholder.svg) and disappears.

image

Then the uploaded image appears instead.

image

Both, placeholder and actual image have different widths and heights and both have outlines, so it looks like the image stretches and shrinks within a few milliseconds.

EDIT:

Splitting the gifs above, we can see that it's not a progress bar, but the outline itself (don't know why the placeholder has no height here):

Firefox: image

Chrome (I don't understand what happened here): image

cc @oleq

oleq commented 4 years ago

Thanks, @panr. 

My two cents:

Blame

I searched git blame and I found out this width: 100% was introduced along with this PR ckeditor/ckeditor5-theme-lark#188 and addressed this issue #5150. Unfortunately, this does not help much.

Code

I also briefly checked the code and it looks like the part responsible for the "glitch" in FF is somewhere in here https://github.com/ckeditor/ckeditor5/blob/bcf3af6bee1edbd3a6d0c6874e0ad0518f73f518/packages/ckeditor5-image/src/imageupload/imageuploadprogress.js#L85-L104

I started commenting out some of the helpers _hidePlaceholder(), _showProgressBar(), _displayLocalImage() and without them, it looks smooth. So I suppose The Glitch could be caused by many things happening at once because the image is very small and Firefox is quick to read it and upload it (but not as quick as Chrome). 

Recordings

Finally, I decided to capture The Glitch myself. First, I recorded the GIF below in the easy-image manual test on Firefox in 50fps and slowed down to 25%:

And here's the same scenario in the Github Writer Firefox addon (also in slow-mo):

Note there's a difference between vanilla CKEditor 5 and GHWriter. In the former, the image never stretches to 100% at any point. In the latter, there's a clear moment when the outline takes 100% width of the editing root (it has height, though, unlike the screenshot from the @panr comment above). Which brings a couple of questions:

panr commented 4 years ago

in the vanilla CKEditor 5, the image never stretches horizontally at any point

Hmm... are you sure? I caught this yesterday in imageuploadviaurl.html manual test 🤔 The image stretches (specifically image placeholder) in the screen below: https://user-images.githubusercontent.com/1303365/92606924-cc5df980-f2b3-11ea-8e7d-d8789bdcee83.png

And today I made these frames to show how I see the process in my editor (Chrome):

Paused in debugger Pasted Graphic 5 Pasted Graphic 6

I also briefly checked the code and it looks like the part responsible for the "glitch" in FF is somewhere in here

I bet that the glitch is somewhere in reading state ;-D

Maybe we should wait a little bit longer and get the image data (width and height) and then show a placeholder that mimics the actual (fixed) image size?

mlewand commented 4 years ago

@oleq , please double check the width aspect. @panr is right in saying that it also occurs in vanilla CKE5.

Disclaimer, poor screenshot resolution because… chrome devtools work on such potato screens (make sense from perf perspective).

tl;dr;

Based on imageuploadviaurl manual test

Image upload (chrome)

4 frames are produced (2 reflows + 2 repaints)

  • Empty widget container is inserted (no spinner visible)
  • Container seems to be much larger than it should be
  • balloon is mispositioned
  • Balloon panel position is fixed
  • Spinner is visible
  • Caption fades in (I believe it's available from the beginning, it's just the animation that makes it visible here)
  • Image is loaded and visible
  • Ballon panel is mispositioned again
  • Balloon is being moved to the final position

Image insert by URL

Following frames are produced:

  • The text in image wrapper is "Enter image caption"
  • Balloon panel position is fixed

Fredck case

Here are key frames from fredck screencast:

First, the editable gets much bigger, to shrink later on. I assume it's, again, caused by widget wrapper.

Notes regarding previous comments

I started commenting out some of the helpers _hidePlaceholder(), _showProgressBar(), _displayLocalImage() and without them

https://github.com/ckeditor/ckeditor5/blob/bcf3af6bee1edbd3a6d0c6874e0ad0518f73f518/packages/ckeditor5-image/src/imageupload/imageuploadprogress.js#L85-L104

I wonder if that was really the case, as the only thing that I was able to comment out without breaking the feature was _showProgressBar and with it commented the issue persists. Skipping _displayLocalImage() keeps us with a placeholder spinner instead of an image, while skipping _hidePlaceholder() leaves the spinner on top of an image.

CKEditorBot commented 1 year ago

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 still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

CKEditorBot commented 12 months ago

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).