Expensify
[MEDIUM] [Image][$1000] Improve NewDot image uploading experience, add large file support
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
- Upload an image in App
Expected Result:
The image should be grayed (opacity 0.6 i.e., chatItemUnsentMessage) until its uploaded and once its uploaded it should not show the white thumbnail with loader
and instead continue showing the local thumbnail or show the final image immediately.
Actual Result:
- The local image is being uploaded and we see the local thumbnail,
- The local image has finished uploading and now we attempt to render it which first shows up a white thumbnail with loader
- The render completes and we see the uploaded image
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platform:
- Web
- iOS
- Android
- Desktop App
- Mobile Web
Version Number: 1.1.76 Reproducible in staging?: Y Reproducible in production?: Y Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1654262165566879
cc @michaelhaxhiu, @cead22, @kidroca.
Issue Owner
Current Issue Owner: @deetergp
Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)
Triggered auto assignment to @deetergp (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Proposal
Opacity can be added to the preview image, but loading later. It would be better to keep it, because it is the loading that all the images have when a url is received and it is being loaded.
This spinner while loading the image is better to keep it.
https://user-images.githubusercontent.com/16966456/174337160-dadac70e-49d2-4e4f-9fdc-3ad56f4756b0.mp4
Add a style: https://github.com/Expensify/App/blob/72ed0e50b9197b8cefad017ac14db9eb16b34433/src/styles/styles.js#L326-L328
https://github.com/Expensify/App/blob/72ed0e50b9197b8cefad017ac14db9eb16b34433/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js#L52-L58
Result
https://user-images.githubusercontent.com/16966456/174337239-e38b03e0-7e8b-46a6-b6c1-78d0eac1c424.mp4
@JosueEchandiaAsto Thanks for the proposal. But as you are a new contributor and have already been assigned two 2 tasks, you will have to wait before applying to new tasks.
Contribution policy: https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#payment-for-contributions
New contributors are limited to working on one job at a time, however experienced contributors may work on numerous jobs simultaneously.
@parasharrajat Both tasks have already been completed and have been merged. Do I need to complete any additional steps in those tasks?
@JosueEchandiaAsto Your proposal does not meet the requirements.
It would be better to keep it, because it is the loading that all the images have when a url is received and it is being loaded.This spinner while loading the image is better to keep it.
Why should we keep the loader?
Idea is to show the same thumbnail when the image is loading even after the server URL is received. When the image is fully loaded from the URL show the final image.
@parasharrajat
This is the process:
- You are requested to upload an image.
- A thumbnail with opacity 0.6 is displayed.
- Confirmation of successful upload is received and the list of chat messages is reloaded.
- If any message is of type image, with the url we load the image.
- A loading is displayed.
- Loading the image and hides the loading.
In step 3, when reloading the list of chat messages, the thumbnail is lost and what comes from the service is loaded. In this case the white load is shown. while loading the url from the service. Therefore, I thought about leaving the current post-load experience. and just add the opacity before uploading the image. Now, I can have it save the thumbnail, then validate which message it corresponds to, and instead of showing the load, show the saved thumbnail. Is that what you want to do?
Now, I can have it save the thumbnail, then validate which message it corresponds to, and instead of showing the load, show the saved thumbnail. Is that what you want to do?
It would be best to achieve the expected results from this issue. let's see your proposal and we can analyze if it fits well.
@parasharrajat Of course yes, I implement it and send an update of the proposal. Thank you very much.
I think the ideal experience should be
- User uploads image
- The user sees the thumbnail of the local right away
- Note if the user is online, the thumbnail should show with full opacity. The transparency should only appear if the user isn't online imo (cc @chiragsalian)
- The image never changes, we don't show a loading image/spinner, to the user uploading the image it should look as if it worked instantly
@JosueEchandiaAsto Am I right in understanding you are considering a proposal for this change?
@MitchExpensify Hello, I have been preparing another proposal but it is complicated to recover the local image when the image post-upload service is called again. At the moment, I am looking at other Issues.
Proposal
The uploaded image goes through two phases. First, it is shown from the local URL, then, once it’s uploaded, from the server URL. This causes flickering and the loading icon. Flickering is also caused by resizing the image from the default size when re-rendering.
ImageWithSizeCalculation
First, let’s add a new localUrl property so we can distinguish in which phase we are at the image component level. It takes precedence over the url prop.
+ const uri = this.props.localUrl || this.props.url;
Remove contain mode, which causes resizing and does not change the appearance of the image because it is fully contained in the component given that its size is calculated from the image itself. Additionally, we set the defaultSource to the same value on the web, otherwise, the RN component flickers. There is an issue related to Avatar flickering where the contributor describes this bug extensively.
- source={{uri: this.props.url}}
- resizeMode="contain"
+ source={{uri}}
+ defaultSource={Platform.select({web: {uri}, default: undefined})}
Only show the upload icon if we are not uploading the image
- {this.state.isLoading && (
+ {this.state.isLoading && !this.props.localUrl && (
Remove the default values which cause flickering. This change does not affect the image in any way because the size is calculated.
ThumbnailImage
- imageWidth: 200,
- imageHeight: 200,
https://github.com/Expensify/App/blob/4b4db2365cd444d5c1f75ab7489f34b4bf60bcc5/src/components/ThumbnailImage.js#L57-L59
- if (!width || !height) {
- return {};
- }
+ if (this.props.localUrl && (!width || !height)) {
+ return {};
+ }
+ if (!width || !height) {
+ // This should improve the stability across the board because if the image is not yet fully loaded,
+ // a call from updateImageSize could overwrite the default values.
+ width = 200;
+ height = 200;
+ }
Pass the prop through.
+ localUrl={this.props.localUrl}
Now we have to make sure that once the image is rendered in Phase 2, it is shown from the local URL instead of the remote URL, which results in no image to show until it’s downloaded. Any subsequent re-render will be fetched automatically from the remote URL.
We do that by temporarily holding the original filename through the phases, then we delete the reference.
The image tag in Phase 2 is generated from HTML received from the server. The uploading process is asynchronous and the original file name is the only way how to correlate both phases.
ImageRenderer
We add a simple object that refreshes when the app loads.
+ const currentUploads = {};
Here, imagePreviewModalDisabled indicates that we are in Phase 1. If that is the case, normalize the image name because the server does it too. Set localUrl to the current URL. If that is not the case, we are in Phase 2, so fetch the cached local URL, assign it to localUrl , and delete it from the cache. https://github.com/Expensify/App/blob/3439796754d31accfdd4b116afe0eba598278f1b/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js#L50
+ const normFileName = originalFileName.replaceAll(/[\s-._]/g, '');
+ let localUrl = null;
+ if (imagePreviewModalDisabled) {
+ currentUploads[normFileName] = previewSource;
+ localUrl = previewSource;
+ }
+ else if (normFileName in currentUploads) {
+ localUrl = currentUploads[normFileName];
+ // delete currentUploads[normFileName] // Potentially delete
+ }
Assign the value to the new prop.
+ localUrl={localUrl}
Pass it through.
+ localUrl={localUrl}
In Phase 2, the server attaches the original filename to the data-name attribute. In Phase 1, we render the tag ourselves. We have to attach it the same way in order to be resolved to the originalFileName variable used above.
ReportActionItemFragment
- <RenderHTML html={`<comment><img src="${props.attachmentInfo.source}" data-expensify-preview-modal-disabled="true"/></comment>`} />
+ <RenderHTML html={`<comment><img src="${props.attachmentInfo.source}" data-name="${props.attachmentInfo.name}" data-expensify-preview-modal-disabled="true"/></comment>`} />
Remove the default values which cause flickering. This change does not affect the image in any way because the size is calculated.
Would that work for remotely served images?
If the size is calculated from the image it has to first be fetched, what will happen in the meantime (e.g. slow connection) It might work instantly for local images but will have delay for served content
@deetergp, @parasharrajat, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
We can set it conditionally instead. If the image is rendered when uploading (including Phase 2), keep it undefined, otherwise set the original default values.
Proposal:
When ImageWithSizeCalculation has to render a local image preview (when it is in process of uploading to the server), we save that local image preview url and name. After that, if ImageWithSizeCalculation renders an image from server and we have a local preview with the same name, we display it instead of loading indicator
- In ReportActionItemFragment.js we add filename to local previews
- <RenderHTML html={`<comment><img src="${props.attachmentInfo.source}" data-expensify-preview-modal-disabled="true"/></comment>`} />
+ <RenderHTML html={`<comment><img
+ src="${props.attachmentInfo.source}"
+ ${CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE}="${props.attachmentInfo.name.replace(/\s/g, '')}"
+ data-expensify-preview-modal-disabled="true"/></comment>`}
+ />
- In ImageRenderer.js pass file name and flag whether to save a local preview
return imagePreviewModalDisabled ? (
<ThumbnailImage
previewSourceURL={previewSource}
style={styles.webViewStyles.tagStyles.img}
isAuthTokenRequired={isAttachment}
imageWidth={imageWidth}
imageHeight={imageHeight}
+ originalFileName={originalFileName}
+ saveLocalPreview
/>
) : (
<AttachmentModal
allowDownload
sourceURL={source}
isAuthTokenRequired={isAttachment}
originalFileName={originalFileName}
>
{({show}) => (
<PressableWithoutFocus
style={styles.noOutline}
onPress={show}
>
<ThumbnailImage
previewSourceURL={previewSource}
style={styles.webViewStyles.tagStyles.img}
isAuthTokenRequired={isAttachment}
imageWidth={imageWidth}
imageHeight={imageHeight}
+ originalFileName={originalFileName}
/>
</PressableWithoutFocus>
)}
</AttachmentModal>
);
- In ThumbnailImage.js pass these props to
ImageWithSizeCalculation
<ImageWithSizeCalculation
url={url}
onMeasure={this.updateImageSize}
+ saveLocalPreview={this.props.saveLocalPreview}
+ originalFileName={this.props.originalFileName}
/>
- And finally at the top of the ImageWithSizeCalculation.js add a variable to store all the local previews
const localPreviews = {};and implement a couple of methods for getting/setting/deleting local preview
saveLocalPreview({ url, width, height }) {
const localPreview = this.getLocalPreview();
if (url) {
localPreview.url = url;
}
if (width) {
localPreview.width = width;
}
if (height) {
localPreview.height = height;
}
localPreviews[this.props.originalFileName] = localPreview;
}
getLocalPreview() {
return localPreviews[this.props.originalFileName] || {};
}
deleteLocalPreview() {
delete localPreviews[this.props.originalFileName];
}
- After that, we need to save local preview in
imageLoadingStartmethod
imageLoadingStart() {
this.setState({isLoading: true});
+ if (this.props.saveLocalPreview) {
+ this.saveLocalPreview({ url: this.props.url });
+ }
}
- Delete it in
imageLoadingEnd
imageLoadingEnd() {
this.setState({isLoading: false});
+ if (!this.props.saveLocalPreview) {
+ this.deleteLocalPreview();
+ }
}
- We also need to save width and height from
onMeasure
calculateImageSize() {
if (!this.props.url) {
return;
}
+ if (!this.props.saveLocalPreview) {
+ const localPreview = this.getLocalPreview();
+ if (localPreview.width && localPreview.height) {
+ this.props.onMeasure({width: localPreview.width, height: localPreview.height});
+ return;
+ }
+ }
this.getImageSizePromise = makeCancellablePromise(this.getImageSize(this.props.url));
this.getImageSizePromise.promise
.then(({width, height}) => {
if (!width || !height) {
// Image didn't load properly
return;
}
this.props.onMeasure({width, height});
+ if (this.props.saveLocalPreview) {
+ this.saveLocalPreview({width, height});
+ }
})
.catch((error) => {
Log.hmmm('Unable to fetch image to calculate size', {error, url: this.props.url});
});
}
- After that we can render our preview, implementing
this.renderPlaceholder()method
renderPlaceholder() {
if (!this.state.isLoading) {
return null;
}
const localPreview = this.getLocalPreview();
if (localPreview.url) {
return (
<Image
style={[
styles.w100,
styles.h100,
styles.pAbsolute,
]}
source={{uri: localPreview.url}}
resizeMode="contain"
/>
)
}
return <FullscreenLoadingIndicator style={[styles.opacity1, styles.bgTransparent]} />
}
-{this.state.isLoading && (
- <FullscreenLoadingIndicator
- style={[styles.opacity1, styles.bgTransparent]}
- />
-)}
+ {this.renderPlaceholder()}
Results: web
https://user-images.githubusercontent.com/9059945/177592113-3131dc20-f410-4f50-99a7-6010caaa8673.mp4
native
https://user-images.githubusercontent.com/9059945/177592662-b77515ab-8585-44b1-9d0e-4cd14286dc57.mp4
You can see the image flickering for a split second, but I'm guessing that's a delay from loading image from local storage
@marktoman Your proposal seems pretty well reasoned but I'd love to see it in action, especially with a slow/throttled internet connection. Do you think you could post a screen cap video or two?
@deetergp Sure, I have updated my proposal and added back the default values to address @kidroca's concern. Here it is in action:
https://user-images.githubusercontent.com/47689923/178010505-346f2c8d-7bb3-4ab9-bbb8-acc07de35815.mp4
https://user-images.githubusercontent.com/47689923/178010669-22890caa-cafb-4ac6-8e50-dcdca5f5184e.mp4
Thanks for the screen captures @marktoman. I like your proposal. If @parasharrajat agrees, then let's go with it!