search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[HOLD for payment 2024-03-29] [$500] Android – Image is reduced for a moment when quickly slide forward/back images using arrows #35615

Closed lanitochka17 closed 5 months ago

lanitochka17 commented 7 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.35-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4265112 Email or phone of affected tester (no customers): applausetester+jp_e_category@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Launch app
  2. Log in
  3. Navigate to a conversation that has 5 or more image attachments in it's history
  4. Click on one of the attachments to open the preview
  5. Click on the forward or back arrow to navigate through the conversation attachments quickly

Expected Result:

The image size is normal when quickly slide forward/back images using the arrows

Actual Result:

The image is reduced for a moment when quickly slide forward/back images using the arrows

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/a99e901f-5fa3-4580-8113-aa288435f338

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019fd6243b644a1821
  • Upwork Job ID: 1753212539309678592
  • Last Price Increase: 2024-02-23
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 0
    • tienifr | Contributor | 0
melvin-bot[bot] commented 7 months ago

Job added to Upwork: https://www.upwork.com/jobs/~019fd6243b644a1821

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

tienifr commented 7 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The image is reduced for a moment when quickly slide forward/back images using the arrows

What is the root cause of that problem?

In here and here, we're showing the image even though the size was not determined, we use the DEFAULT_IMAGE_DIMENSION as the default in that case, which leads to the image being small for a short time while it loads, then after the image dimensions are loaded, it will show in full size.

What changes do you think we should make in order to solve the problem?

In here and here, make sure the image is not visible while its size is being loaded, we can use opacity: 0, visibilityHidden or similar. While the image is still loading the dimensions, the LoadingIndicator will show here so the UX is smooth.

So in here

style={[contentSize ?? DEFAULT_IMAGE_DIMENSION, !contentSize && {opacity: 0}]}

here

style={[fallbackSize, !contentSize && {opacity: 0}]}

(There're a couple more conditions to determine the fallbackSize here, if we want to make sure to account for that condition, we can return undefined there, then in here if fallbackSize is undefined, fallback to DEFAULT_IMAGE_DIMENSION , and set {opacity: 0} just like we did above in this case)

If we're worried about the onLoad not being triggered if the image is not visible (although when I test on Android, iOS it works fine), we can use Image.getSize inside Image component for native, to determine the sizes rather than relying on Image's onLoad.

What alternative solutions did you explore? (Optional)

Do not render the image altogether if the size is not determined, meanwhile the Image.getSize can be used to populate the contentSize instead

abdulrahuman5196 commented 7 months ago

~Will work on review today~

johncschuster commented 7 months ago

Bumping for Melvin

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abdulrahuman5196 commented 7 months ago

Sorry for the delay. Working on review now

abdulrahuman5196 commented 7 months ago

@tienifr From the proposal here https://github.com/Expensify/App/issues/35615#issuecomment-1922853744, To summarize, we show the image before the image size is determined by the system, causing it to show in a fallback size. The suggestion is to not show the image and show the loading indicator.

@johncschuster This expectation seems fine IMO, But I would like to confirm the expectation.

abdulrahuman5196 commented 7 months ago

Followup question to @tienifr Is there a way to re-use the image sizes which we got previously?

tienifr commented 7 months ago

Followup question to @tienifr Is there a way to re-use the image sizes which we got previously?

@abdulrahuman5196 Yes, we can re-use it by not setting the contentSize (and subsequently the cachedImageDimensions cache value) to undefined here when the current lightbox becomes invisible. But this won't fix the issue fully because the issue will still happen when the image is rendered the first time. I agree though that we can surely consider it as a further optimization.

johncschuster commented 7 months ago

@abdulrahuman5196 / @tienifr To make sure I understand:

The suggestion is to not show the image and show the loading indicator.

Are you suggesting that we shouldn't show an image at all when a user is in image preview mode and swiping through images, and instead show a loading indicator?

tienifr commented 7 months ago

@johncschuster no, let me clarify

When swiping to the next image, currently it will show a small image in the center of the screen, with a loading indicator, then after the image is fully loaded, it will show with correct size.

The suggestion is to not show the small image initially. Instead, it will only show loading indicator while the image is loading (which is brief), then after the image is loaded and the size is known, the full image will be shown with the correct size from the start (rather than showing image with incorrect size first then change to correct size as currently)

abdulrahuman5196 commented 7 months ago

Are you suggesting that we shouldn't show an image at all when a user is in image preview mode and swiping through images, and instead show a loading indicator?

Not a suggestion. I would put it as a question instead.

@tienifr proposal here https://github.com/Expensify/App/issues/35615#issuecomment-1922853744 targets to do, also explained in detail here https://github.com/Expensify/App/issues/35615#issuecomment-1946404643. My question would be, Is it good enough from UX perspective? or should we try out for more optimized fix(which would be reach out for other proposals)?

melvin-bot[bot] commented 7 months ago

@johncschuster @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 7 months ago

@johncschuster, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

johncschuster commented 7 months ago

@tienifr

@johncschuster no, let me clarify

When swiping to the next image, currently it will show a small image in the center of the screen, with a loading indicator, then after the image is fully loaded, it will show with correct size.

The suggestion is to not show the small image initially. Instead, it will only show loading indicator while the image is loading (which is brief), then after the image is loaded and the size is known, the full image will be shown with the correct size from the start (rather than showing image with incorrect size first then change to correct size as currently)

Got it. That makes sense to me! Thank you for clarifying that!

johncschuster commented 7 months ago

@abdulrahuman5196

@tienifr proposal here #35615 (comment) targets to do, also explained in detail here #35615 (comment). My question would be, Is it good enough from UX perspective? or should we try out for more optimized fix(which would be reach out for other proposals)?

Fair question. I think I'd like to get some ideas from our @Expensify/design on the UX of this proposal.

@Expensify/design, we're trying to figure out the best way to address how images load when a user scrolls through a carousel of images in NewDot. There are a few suggestions (starting here) on how we can address this, but these might not consider the overall user experience. I'd love your thoughts!

dubielzyk-expensify commented 7 months ago

I probably prefer us showing a loading indicator until we can properly display the images. I don't feel very strongly though, but the less "jumpy" we can make the UI, the better.

Unless we can somehow load all the image sizes as a request on the very first image, instead of as you scroll through the carousel?

Keen for rest of @Expensify/design opinions.

shawnborton commented 7 months ago

Yup, that makes sense to me. I basically agree with this comment too:

The suggestion is to not show the small image initially. Instead, it will only show loading indicator while the image is loading (which is brief), then after the image is loaded and the size is known, the full image will be shown with the correct size from the start (rather than showing image with incorrect size first then change to correct size as currently)

dannymcclain commented 7 months ago

Yeah that sounds good to me too. Agree with Jon that the less jumpy we can make the UI, the better.

johncschuster commented 7 months ago

Awesome. Thanks for your feedback, team!

melvin-bot[bot] commented 7 months ago

@johncschuster @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 7 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

johncschuster commented 7 months ago

@abdulrahuman5196 can you take a look at the comments above? Based on the design team's feedback, it sounds like we would go with @tienifr's proposal. Do you agree?

abdulrahuman5196 commented 6 months ago

Hi, Reviewing now.

abdulrahuman5196 commented 6 months ago

@tienifr I noticed that we are getting the contentSize from cachedImageDimensions which is basically caching the image dimensions. Then how this issue happening even if we swipe for the second time without exiting the attachment corousel?

https://github.com/Expensify/App/blob/7120be24bbcce3c32ff3e257a2690aa27af00eaa/src/components/Lightbox/index.tsx#L103

tienifr commented 6 months ago

Then how this issue happening even if we swipe for the second time without exiting the attachment carousel?

@abdulrahuman5196 It's because the cached dimensions will be cleared as soon as the lightbox becomes invisible here. Only a limited number of the light boxes will be rendered at a time due to performance reasons.

Of course we can make it so that the Lightbox dimensions will keep being cached even though the lightbox has became invisible, but it will only fix it for second-time-rendering case.

mvtglobally commented 6 months ago

Issue not reproducible during KI retests. (First week)

johncschuster commented 6 months ago

@mvtglobally can you share a video of what you saw instead? Was the image not reduced? Did you see the loading pattern?

abdulrahuman5196 commented 6 months ago

Reviewing now again

abdulrahuman5196 commented 6 months ago

Nope I still see the issue

abdulrahuman5196 commented 6 months ago

@tienifr Thanks for the explanation here https://github.com/Expensify/App/issues/35615#issuecomment-1963996981. I think it fine to leave the number of lighboxes to be as it is for now to avoid any performance regressions.

abdulrahuman5196 commented 6 months ago

Given the design discussions we had. @tienifr 's proposal here https://github.com/Expensify/App/issues/35615#issuecomment-1922853744 to show loading indicator instead of fallbackSize looks good and works well.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

iwiznia commented 6 months ago

Hmmmm but is the proposal from @tienifr making the LoadingIndicator show? I see you said:

the LoadingIndicator will show here so the UX is smooth.

So, maybe how this works is like this?

melvin-bot[bot] commented 6 months ago

@iwiznia @johncschuster @abdulrahuman5196 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 6 months ago

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 6 months ago

Current assignee @abdulrahuman5196 is eligible for the External assigner, not assigning anyone new.

iwiznia commented 6 months ago

@tienifr bump

abdulrahuman5196 commented 6 months ago

Hmmmm but is the proposal from @tienifr making the LoadingIndicator show? I see you said:

the LoadingIndicator will show here so the UX is smooth.

So, maybe how this works is like this?

* Image is loading (loading indicator shows)

* Image loaded, but dimensions not determined yet (loading indicator is not there and image is neither as its opacity is 0)

* Image loaded, dimensions computed (image shows properly)

@iwiznia I don't exactly understand the question here. But as far as the usecase goes, we will show the image only after the image is loaded and the dimensions are computed. Until then loader will be shown. Hope it clarifies.

tienifr commented 6 months ago

So, maybe how this works is like this?

@iwiznia There're only 2 phases instead of 3:

  1. Image is not fully loaded, dimensions not determined yet (loading indicator shows)
  2. Image loaded, dimensions computed (image shows properly)

The only difference compared to the current flow is that in step 1:

iwiznia commented 6 months ago

Currently: loading indicator shows and the image with the wrong size is shown briefly

Ahhh got it!

melvin-bot[bot] commented 6 months ago

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 6 months ago

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

tienifr commented 6 months ago

PR ready for review https://github.com/Expensify/App/pull/37739.

melvin-bot[bot] commented 6 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 6 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.55-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-03-29. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 6 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: