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

[$250] Attachment -The image can be dragged by moving the cursor outside the image #12777

Closed
kbecciv opened this issue Nov 16, 2022 · 50 comments
Closed
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Nov 16, 2022

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


Issue found when executing PR #12226

Action Performed:

  1. Open the App
  2. Login with any account
  3. Go to any chat
  4. Send some image file (verify that the resolution is large enough to be draggable in zoomed state)
  5. Click on the image
  6. Now click to zoom and drag
  7. Release the cursor when that is outside the image view
  8. Try to move the cursor

Expected Result:

The image should NOT be dragged

Actual Result:

The image can be dragged by moving the cursor outside the image

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.2.28.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5823089_Record_2022-11-16-07-03-20_4f9154176b47c00da84e32064abf1c48__1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 16, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 16, 2022

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

@stephanieelliott
Copy link
Contributor

Yeah, this feels like weird and buggy behavior.

Posted to Upwork: https://www.upwork.com/jobs/~017a548ef0784db6bb

@stephanieelliott stephanieelliott added the External Added to denote the issue can be worked on by a contributor label Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

Job added to Upwork: https://www.upwork.com/jobs/~01d038f00534da37c6

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@melvin-bot melvin-bot bot changed the title Attachment -The image can be dragged by moving the cursor outside the image [$250] Attachment -The image can be dragged by moving the cursor outside the image Nov 17, 2022
@aimane-chnaif
Copy link
Contributor

@stephanieelliott I think you don't need to create upwork job manually. There's already upwork job link in issue description. It seems @thienlnam integrated upwork automation successfully.
Also Melvin commented this: #12777 (comment)

@RaresShoemaker
Copy link

This can be simply done by implementing a custom hook with two zones of draggable and droppable using https://developer.mozilla.org/en-US/docs/Web/API/HTML_Drag_and_Drop_API no other React libraries that will only make the app slower. I can provide a full explanation of this later if you are intersted.

@Emz27
Copy link

Emz27 commented Nov 17, 2022

Proposal

  1. measure image size and get proportions to container. calculate min max panning distance using those values
  2. make ImageView component from scratch using react-native-gesture-handler + react-native-reanimated to imitate native like performance and expectations like image panning decay (would take me a few days)

@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

@PauloGasparSv, @stephanieelliott, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2022
@aimane-chnaif
Copy link
Contributor

@Emz27 Thanks for your proposal but can you please explain more details with code changes you will apply?

@melvin-bot melvin-bot bot removed the Overdue label Nov 21, 2022
@aimane-chnaif
Copy link
Contributor

@RaresShoemaker If you haven't check already, please go through our contributing guidelines here. The proposal should include the root cause if applicable and technical explanation of code changes you will make.

@tienifr
Copy link
Contributor

tienifr commented Nov 21, 2022

Proposal

Problem

In

ImageSize.getSize(this.props.url).then(({width, height}) => {
let imageWidth = width;
let imageHeight = height;
const containerWidth = Math.round(this.props.windowWidth);
const containerHeight = Math.round(this.state.containerHeight);
const aspectRatio = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);
if (imageHeight > imageWidth) {
imageHeight *= aspectRatio;
} else {
imageWidth *= aspectRatio;
}

The original width and height of the image is very high. For example in my case, these are 4032 and 3024
Screen Shot 2022-11-21 at 17 09 58

so we have to calculate the aspectRatio to calculate the dimension of the image when it's shown in the device. But we just re-calculate the width when imageWidth > imageHeight, so the imageHeight is still the original height (3024), vice versa. => we can drag image height as 3024.

Solution

We'll re-calculate both width and height

+            imageHeight *= aspectRatio;
+            imageWidth *= aspectRatio;

-            if (imageHeight > imageWidth) {
-                imageHeight *= aspectRatio;
-            } else {
-                imageWidth *= aspectRatio;
-            }

Result:

Screen.Recording.2022-11-21.at.17.22.51.mp4

@thesahindia
Copy link
Member

I think it's the same issue that I reported here

@aldo-expensify
Copy link
Contributor

Why is this a bug and not a feature? what is the downside of allowing the user to drag an image from the white part?

@melvin-bot melvin-bot bot added the Overdue label Nov 23, 2022
@aldo-expensify
Copy link
Contributor

cc @shawnborton can I get your opinion on this? 🙏

@shawnborton
Copy link
Contributor

Yeah, I don't understand why this is a bug - this seems to be behaving exactly like we'd expect. I think this is why it's important that our bug reports have corresponding Slack conversations so we can chat about these earlier.

@thesahindia
Copy link
Member

I think this is a better video (from the thread)

video_20220702_002457_edit.mp4

@tienifr
Copy link
Contributor

tienifr commented Nov 28, 2022

The PR of this issue is created. @PauloGasparSv Please help to review it. Thanks

@luacmartins
Copy link
Contributor

@PauloGasparSv @aimane-chnaif I'm gonna check this one off the deploy checklist since it didn't cause any regression, but can you please investigate this comment #13099 (comment). We are still waiting on a video from Applause.

@PauloGasparSv
Copy link
Contributor

Friendly bump @mvtglobally on the video for us to investigate

@aimane-chnaif
Copy link
Contributor

I wonder #13150 is what @mvtglobally concerns

@PauloGasparSv
Copy link
Contributor

That would make sense @aimane-chnaif! Let's wait for @mvtglobally to confirm.
If that's the case, it is a separate issue and I don't think is a bug.

@PauloGasparSv
Copy link
Contributor

That would make sense @aimane-chnaif! Let's wait for @mvtglobally to confirm.
If that's the case, it is a separate issue and I don't think is a bug.

Friendly bump on this : )

@mvtglobally
Copy link

Asking team to retest on the latest build with Video

@mvtglobally
Copy link

Here is what we are seeing on the latest build. Is there a need to log a new issue based on comments? Tread is quite big, not sure I follow the recent decision

Record_2022-12-07-19-04-47_4f9154176b47c00da84e32064abf1c48.mp4

@PauloGasparSv
Copy link
Contributor

@mvtglobally I think that's the desired behavior right? We wanted to make sure users wouldn't be able to drag and scroll the image forever until the image itself was outside of view.

This P.R. added those boundaries so you can't scroll outside of the image anymore : )

@mvtglobally
Copy link

It's for you to decide guys :). There are so many changes recently that we are not always sure whats the issue and what is not.

@aimane-chnaif
Copy link
Contributor

@mvtglobally I think correct guide here

@PauloGasparSv
Copy link
Contributor

I think we're good here so I'm closing this one off : )

@PauloGasparSv
Copy link
Contributor

PauloGasparSv commented Dec 7, 2022

I'm not sure how the payment process works but I think a bot will take care of it like this other issue. So I'll come back to this tomorrow and ask the team if nothing is updated here.

Edit: Just confirmed it here (found the doc hehe), the label should be added automatically 7 days after the P.R. hits prod.

@aimane-chnaif
Copy link
Contributor

PR was deployed to production on Nov 30 (#13077 (comment)).
So payment is due today.
Not sure why title was not updated.
cc: @stephanieelliott

@aimane-chnaif
Copy link
Contributor

@stephanieelliott bump ^

@stephanieelliott
Copy link
Contributor

Oops, sorry guys -- missed this because the issue is closed 🙃 (my fault)

@tienifr You've been paid!

@aimane-chnaif I just sent the offer your way on Upwork, just accept it when you get a chance -- I'll get an alert from Upwork and will issue the payment right away!

@aimane-chnaif
Copy link
Contributor

No problem. Thanks @stephanieelliott

@stephanieelliott
Copy link
Contributor

😊 Everyone is paid up!

@thesahindia
Copy link
Member

Sorry for the late comment, this is eligible for the reporting compensation @stephanieelliott. We decided to fix the bug that was reported here

@stephanieelliott
Copy link
Contributor

Oh, thanks for calling that out @thesahindia! Invited you to the job in Upwork, please accept when you get a chance! 😊

@thesahindia
Copy link
Member

Applied, thanks!

@thesahindia
Copy link
Member

Bump @stephanieelliott for the payment

@stephanieelliott
Copy link
Contributor

All paid up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests