Retry to download an image if a download error occured after an upload#3803
Retry to download an image if a download error occured after an upload#3803
Conversation
…rror after an upload
|
Note: it's a pure |
| // Even on an error, we swap the image for the time being. This is because private | ||
| // blogs are currently failing to download images due to access privilege issues. | ||
| // | ||
| imageNode.attr('src', image.src); |
There was a problem hiding this comment.
This was needed to make sure the image inside the container (the 'real' image, not the one we load into memory using the image object) is set to the remote URL. With this line removed, if the image fails to load, the local image isn't swapped out, resulting in a 'loaded'-looking image with a local (Android filesystem) src, which can be uploaded.
We should keep this, so that even if the image fails and never loads for some reason, the post is still uploaded with a remote URL. The comment doesn't make its use clear though, I think we can add some clarification.
There was a problem hiding this comment.
Follow-up note: this is a little trickier than I initially realized, since we'll be showing the user a dead image icon for however long it takes to load the image from the server. This might only be for 500ms, but it's still not ideal. I still think that's better than risking uploading the post with the (wrong) local image url.
A permanent solution might be to keep the line removed as you had it, but make sure the local src is swapped for the remote one if the user attempts to upload before the image has been loaded from the server from onerror retries.
There was a problem hiding this comment.
This might only be for 500ms, but it's still not ideal. I still think that's better than risking uploading the post with the (wrong) local image url.
Yes, it's a bit weird, but I think it's better to not change the HTML source to fix a graphic glitch - we could end up breaking the post.
Here a demo with a 5 secs delay:
https://cloudup.com/cErFyGdP1GX
A permanent solution might be to keep the line removed as you had it, but make sure the local src is swapped for the remote one if the user attempts to upload before the image has been loaded from the server from onerror retries.
What would be the best solution to mark it "unswapped yet", add a class to the img?
There was a problem hiding this comment.
What would be the best solution to mark it "unswapped yet", add a class to the img?
I think a good solution would be adding a remoteUrl attribute, set to the remote URL once we have it. Then we add an extra step to ZSSEditor.removeVisualFormatting() to find any image tags with that attribute and swap out the src for remoteUrl. That should cover the case where the image never loads before publishing.
|
From testing with a .org site (adding a character in the code when setting the image src originally to force a 404, then renaming the image server-side while the editor attempts to reload), the retries after the initial attempt load from the cache instead of attempting a fresh network call, so that the image is never loaded even when it's available on the server. One fix would be to append a |
Never happened during my tests, seems weird, the 404 error call shouldn't be cached. |
|
Cache-Control on 404 errors make sense in some cases, so we want to use the |
b14a939 to
7fba5d6
Compare
|
Added a |
|
Closing this and deleting the branch - followup in #3896 |
Retry to download an image if a download error occured after an upload - workaround to fix wordpress-mobile/WordPress-Editor-Android#300
Previous
onErrorimplementation seemed weird (imageNode.attr('src', image.src);), not sure if it was working. I tried to upload images on private sites, and everything works as expected.