Chat - Image still moving when the cursor outside the image with resized window

[kbecciv]

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 #13077

Action Performed:

1. Go to staging.new.expensify.com
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. Resize the window browser
6. Click on the image
7. Now click to zoom and drag

Expected Result:

The image is not being dragged by moving the cursor outside the image with resized window

Actual Result:

Image still moving when the cursor outside the image with resized window

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Desktop

Version Number: 1.2.33.2

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

Screen.Recording.2022-11-29.at.11.28.52.AM.mp4

Screen.Recording.2022-11-29.at.1.19.38.PM.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[allroundexperts]

Proposal

In the ImageView modal, we should stop tracking the mouse movement if the cursor moves outside of the window.

Fix

diff --git a/src/components/ImageView/index.js b/src/components/ImageView/index.js
index 2356ffee8..5d26aa85e 100644
--- a/src/components/ImageView/index.js
+++ b/src/components/ImageView/index.js
@@ -191,7 +191,14 @@ class ImageView extends PureComponent {
     }
 
     trackMovement(e) {
-        if (!this.state.isZoomed) {
+        const cursorIsOutsideWindow = e.nativeEvent.clientY <= 0
+        || e.nativeEvent.clientX <= 0
+        || (
+            e.nativeEvent.clientX >= window.innerWidth
+            || e.nativeEvent.clientY >= window.innerHeight
+        );
+
+        if (!this.state.isZoomed || cursorIsOutsideWindow) {
             return;
         }
 

Result

screen-recording-2022-11-30-at-61329-am_lgs3r0l2.mp4

[maddylewis]

Managing a BUG issue

From  this  SO: https://stackoverflow.com/c/expensify/questions/14418

• Confirm the "bug" is  actually  a  bug
• Confirm the  bug is  not  a  duplicate  report of  an existing  GH (close  the  GH  and  add  any  relevant  details  to  the  original  GH ) 
• The  bug  is  reproducible

If you’re unable  to  reproduce  the  bug:

• Add  the Needs reproduction  label
• Comment  on the  issue  outlining the steps  you  took  to reproduce the bug, and your  results
• Tag the issue reporter and the Applause QA member who created the issue. Ask  them  to  clarify  the reproduction steps  and/or to  check  the  reproduction  steps  again.

If the bug is reproducible:

• The  GH  template is  filled  out  as  fully  as  possible -- this  means  the  GH  body  and  title  are  clear
• The  GH  was  created  by  an  Expensify employee  or  a  QA  tester
• If  the bug is an OldDot issue,  you  should  decide  whether  it  is  widespread  enough  to  justify fixing or  it  is  better  to  wait  for  reunification. If  it’s  better  to  wait,  close  the  GH and  provide details
• If there's  a  link  to  Slack, check the discussion and see why we decided to fix it or not
• Decide if  the  GH should be resolved by an External contributor or an Internal engineer and add the appropriate label

[Ollyws]

Proposal

The root cause of that the ImageView component tracks the position of the mouse, and updates the image accordingly regardless of where it is on the screen.

I'm slightly unsure of the expected behaviour here, personally I think it looks better if movement is restricted to when the mouse is over the image window, but I will post two options.

We can just determine if the mouse is outside the ImageView (or browser window) and do an early return in trackMovement, however the problem with this is it causes the image to jump when the mouse re-enters, which looks janky in my opinion:

JankyExample.mp4

To resolve this we can detect when the mouse has been dragged back into the window by setting and checking the previous value, and if it has then resetting the positions:

diff --git a/src/components/ImageView/index.js b/src/components/ImageView/index.js
index 2356ffee8..4a71764d9 100644
--- a/src/components/ImageView/index.js
+++ b/src/components/ImageView/index.js
@@ -195,7 +195,19 @@ class ImageView extends PureComponent {
             return;
         }
 
-        if (this.state.isDragging && this.state.isMouseDown) {
+        const isInsideImageView = this.scrollableRef.contains(e.nativeEvent.target);
+
+        if (!this.wasInsideImageView && isInsideImageView && this.state.isDragging) {
+            const {pageX, pageY} = e.nativeEvent;
+            this.setState({
+                initialX: pageX,
+                initialY: pageY,
+                initialScrollLeft: this.scrollableRef.scrollLeft,
+                initialScrollTop: this.scrollableRef.scrollTop,
+            });
+        }
+
+        if (this.state.isDragging && this.state.isMouseDown && isInsideImageView) {
             const x = e.nativeEvent.x;
             const y = e.nativeEvent.y;
             const moveX = this.state.initialX - x;
@@ -204,6 +216,7 @@ class ImageView extends PureComponent {
             this.scrollableRef.scrollTop = this.state.initialScrollTop + moveY;
         }
 
+        this.wasInsideImageView = isInsideImageView;
         this.setState(prevState => ({isDragging: prevState.isMouseDown}));
     }

Which looks alot smoother:

FixedExample.mp4

If we want to use the browser window instead of the ImageView window to determine whether to disable movement, we can just use this variable instead:

-const isInsideImageView = this.scrollableRef.contains(e.nativeEvent.target);
+const isInsideImageView = !(e.clientY <= 0 || e.clientX <= 0 || (e.clientX >= window.innerWidth || e.clientY >= window.innerHeight));

[marcaaron]

imo this is not really a bug nor worth prioritizing. There is no clear reason why mousing down and then dragging outside an area should prevent the image from being panned.

[maddylewis]

yep, after testing this on my end this doesn't seem like an issue that's worth prioritizing. i will confirm our thinking here - https://expensify.slack.com/archives/C049HHMV9SM/p1669914131812339

[PauloGasparSv]

Agree with @marcaaron on this.

I'm more inclined to think this is not a bug, the current behavior seems better for me and I'm pretty sure that's how Slack works too:

Screen.Recording.2022-12-01.at.15.54.05.mov

[maddylewis]

thanks for the input! since we confirmed this report is NOT actually a bug, I will go ahead and close this issue out.