-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[$2000] Large image doesn't zoom well in Android and gets blurry #12893
Comments
Triggered auto assignment to @tjferriss ( |
I remember this same images was causing this a long time ago: #5193 I wonder if it ever got fixed and if this is a regression. |
@aldo-expensify I mentioned this in the slack too, it is a regression. I had tested this earlier and it did work at some point when working on another issue. |
@tjferriss Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
I don't have an Android device. How would you suggest reproducing the bug? |
@tjferriss do you have an Android emulator or say Browserstack login? You can then reproduce this. This occurs only on Android. Second video on the issue body is from my Android device. |
Maybe this will get fixed once the PR #12648 gets re-done and we use fast-image.. maybe |
Well I tested this when #12648 was worked upon and it wasn't resolved. In fact I found this issue, while testing the same PR. We can still put this on hold, and come back again if you think that's the best option. |
No, given that you already saw that this doesn't get fixed, ignore my previous comment :P |
Thank you @aldo-expensify. This looks ready to go so I'm going to add the External tag. |
Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new. |
Job added to Upwork: https://www.upwork.com/jobs/~0100609c253c2769f4 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Triggered auto assignment to @marcaaron ( |
It seems that this is a common issue on android only here is a few links of people complaining about it. https://stackoverflow.com/questions/53402308/react-native-0-57-x-image-large-images-low-quality You can consider it a Fresco problem or a react native problem, but regardless the solution is not feasable for a ticket of this size. I spent the last 5 hours debugging this |
Thanks @markarmanus I was also having a look, what you said matches with explanation on what was happening when it was fixed in the past: #5193 (comment) Here is the PR that was done to fix it: #6442 Haven't looked further into why now it is not working, but I'm leaving the links here in case they are of use to someone. |
Proposal Problem: The issue is happening on android, when you render an image that the original resolution is massive but you render it with a smaller resolution Example: // original image is 14500x3000
<Image style={{width: 300, height: 300, }} resize="contain" /> On IOS this image is going to stay at the original resolution when zooming in. On Android the image gets downscaled to the resolution sepefied to the component so that results in a blury picture on zoom in. As i stated above this is a recurring issue in the react native community. There is currently no solution for the issue. I was able to create a solution. its somewhat hacky but it works. Here is how to achieve this. const imageTransformScale = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);
this.setState({imageHeight, imageWidth, imageTransformScale});
// calculate a imageTransformScale We also git rid of this code which is attempting to make the resolution the size of the screen. const aspectRatio = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);
if (imageHeight > imageWidth) {
imageHeight *= aspectRatio;
} else {
imageWidth *= aspectRatio;
} Then Contain the image in a view that uses that scale <View style={{transform: [{scale: this.state.imageTransformScale}]}}>
<ImageZoom
...
</ImageZoom>
</View> We also need to extend ImageZoom with these props cropWidth={this.props.windowWidth / this.state.imageTransformScale}
cropHeight={this.state.containerHeight / this.state.imageTransformScale} Here is the result in screen shots. Behaves exactly the same on ios The final solution includes a few other line changes but this is mostly it. |
@marcaaron and @mananjadhav what do we think of @markarmanus proposal? |
I am not 100% convinced with @markarmanus's proposal.
Also I didn't understand this statement. @markarmanus if you check this earlier, it was solved and it worked fine. I think with multiple refactors to the image component, we've lost/remove the change. |
Not overdue, @ArekChr is going to open an App PR soon. |
We have our fork ready, we now have to add it to the App. @ArekChr is working on it. We're facing some challenges when building the App with the fork. We'll ask for the team's help if we don't figure it out soon. |
I should be free by tomorrow or day after. Do let me know if I can help in anyway for building the App with the fork. |
We just got past that thing. PR is going to be ready soon, @ArekChr is taking screen recordings now. |
There is a blocker to finish QA checklist tests for mWeb safari and chrome #2499 |
This issue has not been updated in over 15 days. @mananjadhav, @tjferriss, @cristipaval, @ArekChr eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
Closing this one, as the fix hit production a few weeks ago. |
@cristipaval @tjferriss I did the C+ review and testing on this one. Payout is pending for that . |
Oh sorry, I missed that! @tjferriss , should we re-open this one and add |
@mananjadhav I've sent you a request via Upworks here: https://www.upwork.com/jobs/~01ae27c2191acb5934. I also included the reporting bonus in the amount. |
Accepted @tjferriss |
The payment has been sent. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Image should zoom to show a clear image instead of a blurry one
Actual Result:
Image content is not visible, and it is blurry.
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.29-6
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
az_recorder_20221121_114833.1.mp4
android-image-blurry.mov
Expensify/Expensify Issue URL:
Issue reported by: @mananjadhav
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1668928921788569
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: