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

Rethink image upload flow and image model & data while it is uploading #9238

Closed
scofalik opened this issue Mar 15, 2021 · 3 comments · Fixed by #10449
Closed

Rethink image upload flow and image model & data while it is uploading #9238

scofalik opened this issue Mar 15, 2021 · 3 comments · Fixed by #10449
Assignees
Labels
package:image squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

scofalik commented Mar 15, 2021

📝 Provide a description of the improvement

We have a problem with how uploading image is represented and handled in model and how it is handled in conversion. The problem is that an image which is uploading does not have src attribute. If such image is then downcasted, there is an image with no src attribute in the HTML. Then, when that's upcasted, the image is gone. It is not converted, because there is no src attribute.

This causes problems in some features that rely heavily on the editor state. One example is data compression, which may fail, because the model before and the model after compression may differ if the image was uploading when the compression got triggered.

I have two ideas:

  1. Simpler. If the image is uploading, it should be downcasted in a way that upcasting it is possible, it would still be an image but displaying some kind of "the image is broken because upload was cancelled" message.
  2. Complex. The image should be downcasted in such a way, that the upload would be continued after the image is upcasted. There are two problems here:
    • the image would have to be saved ad base64 in the data content (unless we save it in local storage but this has obvious limitations when it comes to re-uploading),
    • do not send images coded in base64 to other clients in RTC as the operation would be huge.

Not sure if the second idea is viable.

After we have some idea how we want to handle uploading and upload-resuming (if we do) we should consider this also for other asynchronously loaded features (not sure if auto-embed is any problematic in this regard, maybe some future features?). It would be good to have one system for all such cases.

Also related: #5159

@scofalik scofalik added type:improvement This issue reports a possible enhancement of an existing feature. package:image labels Mar 15, 2021
@Reinmar Reinmar modified the milestones: iteration 41, nice-to-have Mar 15, 2021
@Reinmar
Copy link
Member

Reinmar commented Mar 22, 2021

Image upload uses pending actions, so we can block other features while image upload is in progress. Hence, it's a low priority for now.

@Reinmar Reinmar modified the milestones: nice-to-have, backlog Mar 22, 2021
@Reinmar Reinmar removed the squad:dx label Mar 22, 2021
@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Aug 4, 2021
@Reinmar Reinmar modified the milestones: backlog, nice-to-have Aug 4, 2021
@Reinmar
Copy link
Member

Reinmar commented Aug 11, 2021

Scope for now:

  • Go with the simplest solution for now (upcast empty src)
    • Tests in ckeditor5-image
    • Tests for this case in RH (closes: cksource/collaboration-features#3859)
  • Try to figure out why this check was there in the first place
    • Needs going down the git blame far past the work on image block/inline
    • Also: verify whether in HTML it's correct to have only e.g. srcset+sizes, but no src
      • In that case, pasting content from ext websites might more often result in broken images
      • NOTE: According to MDN, src is required, so this should not be an issue
  • Open a followup to figure out whether we can provide a better UX
    • Research how other editor handle such a case (e.g. Google Docs)

@Reinmar
Copy link
Member

Reinmar commented Aug 11, 2021

  • Needs going down the git blame far past the work on image block/inline

Done already: Nothing found. Changes from 2017.

@Reinmar Reinmar modified the milestones: nice-to-have, iteration 46 Aug 16, 2021
@dawidurbanski dawidurbanski self-assigned this Aug 18, 2021
@AnnaTomanek AnnaTomanek modified the milestones: iteration 46, iteration 47 Sep 6, 2021
niegowski added a commit that referenced this issue Sep 7, 2021
Other (image): Upcast images without or with empty `src` attribute. Closes #9238.

Other (link): Upcast linked images without or with empty `src` attribute. See #9238.

Other (media): The `figure` element should not be consumed if the media embed is unknown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants