Merge branch 'fix-design-overlay-commenting' into 'master'

Fix incorrect repositioning of design comment pins when mouse leaves viewport

See merge request gitlab-org/gitlab!29464

ee/app/assets/javascripts/design_management/components/design_overlay.vue

@@ -219,7 +219,7 @@ export default {
       type="button"
       class="btn-transparent position-absolute image-diff-overlay-add-comment w-100 h-100 js-add-image-diff-note-button"
       data-qa-selector="design_image_button"
-      @click="setNewNoteCoordinates({ x: $event.offsetX, y: $event.offsetY })"
+      @mouseup="setNewNoteCoordinates({ x: $event.offsetX, y: $event.offsetY })"
     ></button>
     <design-note-pin
       v-for="(note, index) in notes"

ee/app/assets/javascripts/design_management/components/design_presentation.vue

@@ -46,8 +46,8 @@ export default {
         height: 0,
       },
       initialLoad: true,
-      startDragPosition: null,
       lastDragPosition: null,
+      isDraggingDesign: false,
     };
   },
   computed: {
@@ -62,9 +62,6 @@ export default {
         cursor: this.isDraggingDesign ? 'grabbing' : undefined,
       };
     },
-    isDraggingDesign() {
-      return Boolean(this.lastDragPosition);
-    },
   },
   beforeDestroy() {
     const { presentationViewport } = this.$refs;
@@ -219,15 +216,14 @@ export default {
     onPresentationMousedown({ clientX, clientY }) {
       if (!this.isDesignOverflowing()) return;
 
-      this.startDragPosition = {
+      this.lastDragPosition = {
         x: clientX,
         y: clientY,
       };
-
-      this.lastDragPosition = { ...this.startDragPosition };
     },
     onPresentationMousemove({ clientX, clientY }) {
       if (!this.lastDragPosition) return;
+      this.isDraggingDesign = true;
 
       const { presentationViewport } = this.$refs;
       if (!presentationViewport) return;
@@ -242,15 +238,9 @@ export default {
         y: clientY,
       };
     },
-    onPresentationMouseup({ offsetX, offsetY }) {
-      if (
-        this.startDragPosition?.x === this.lastDragPosition?.x &&
-        this.startDragPosition?.y === this.lastDragPosition?.y
-      ) {
-        this.openCommentForm({ x: offsetX, y: offsetY });
-      }
-
+    onPresentationMouseup() {
       this.lastDragPosition = null;
+      this.isDraggingDesign = false;
     },
     isDesignOverflowing() {
       const { presentationContainer } = this.$refs;

ee/changelogs/unreleased/fix-design-overlay-commenting.yml

+---
+title: Fix incorrect repositioning of design comment pins when mouse leaves viewport
+merge_request: 29464
+author:
+type: fixed

ee/spec/frontend/design_management/components/design_overlay_spec.js

@@ -61,7 +61,7 @@ describe('Design overlay component', () => {
 
     wrapper
       .find('.image-diff-overlay-add-comment')
-      .trigger('click', { offsetX: newCoordinates.x, offsetY: newCoordinates.y });
+      .trigger('mouseup', { offsetX: newCoordinates.x, offsetY: newCoordinates.y });
     return wrapper.vm.$nextTick().then(() => {
       expect(wrapper.emitted('openCommentForm')).toEqual([
         [{ x: newCoordinates.x, y: newCoordinates.y }],

ee/spec/frontend/design_management/components/design_presentation_spec.js

 import { shallowMount } from '@vue/test-utils';
 import DesignPresentation from 'ee/design_management/components/design_presentation.vue';
+import DesignOverlay from 'ee/design_management/components/design_overlay.vue';
 
 const mockOverlayData = {
   overlayDimensions: {
@@ -18,6 +19,7 @@ describe('Design management design presentation component', () => {
   function createComponent(
     { image, imageName, discussions = [], isAnnotating = false } = {},
     data = {},
+    stubs = {},
   ) {
     wrapper = shallowMount(DesignPresentation, {
       propsData: {
@@ -26,11 +28,15 @@ describe('Design management design presentation component', () => {
         discussions,
         isAnnotating,
       },
+      stubs,
     });
 
     wrapper.setData(data);
+    wrapper.element.scrollTo = jest.fn();
   }
 
+  const findOverlayCommentButton = () => wrapper.find('.image-diff-overlay-add-comment');
+
   /**
    * Spy on $refs and mock given values
    * @param {Object} viewportDimensions {width, height}
@@ -70,12 +76,17 @@ describe('Design management design presentation component', () => {
           mouseup: 'mouseup',
         };
 
-    wrapper.trigger(event.mousedown, {
+    const addCommentOverlay = findOverlayCommentButton();
+
+    // triggering mouse events on this element best simulates
+    // reality, as it is the lowest-level node that needs to
+    // respond to mouse events
+    addCommentOverlay.trigger(event.mousedown, {
       clientX: startCoords.clientX,
       clientY: startCoords.clientY,
     });
     return wrapper.vm.$nextTick().then(() => {
-      wrapper.trigger(event.mousemove, {
+      addCommentOverlay.trigger(event.mousemove, {
         clientX: endCoords.clientX,
         clientY: endCoords.clientY,
       });
@@ -425,52 +436,20 @@ describe('Design management design presentation component', () => {
     });
   });
 
-  describe('onPresentationMouseUp when design is overflowing', () => {
-    it('does not open a comment form if design was dragged', () => {
-      const startDragPosition = { x: 1, y: 1 };
-      const lastDragPosition = { x: 2, y: 2 };
-      createComponent({}, { startDragPosition, lastDragPosition });
-
-      wrapper.vm.onPresentationMouseup({ offsetX: 2, offsetY: 2 });
-
-      return wrapper.vm.$nextTick().then(() => {
-        expect(wrapper.emitted('openCommentForm')).toBeFalsy();
-      });
-    });
-
-    it('opens a comment form if design was not dragged', () => {
-      const startDragPosition = { x: 1, y: 1 };
-      const lastDragPosition = { x: 1, y: 1 };
-      createComponent(
-        {},
-        {
-          startDragPosition,
-          lastDragPosition,
-          ...mockOverlayData,
-        },
-      );
-
-      wrapper.vm.onPresentationMouseup({ offsetX: 2, offsetY: 2 });
-
-      return wrapper.vm.$nextTick().then(() => {
-        expect(wrapper.emitted('openCommentForm')).toBeDefined();
-      });
-    });
-  });
-
-  describe('when clicking and dragging', () => {
-    it.each`
-      description               | useTouchEvents
-      ${'with touch events'}    | ${true}
-      ${'without touch events'} | ${false}
-    `('calls scrollTo with correct arguments $description', ({ useTouchEvents }) => {
+  describe('when design is overflowing', () => {
+    beforeEach(() => {
       createComponent(
         {
           image: 'test.jpg',
           imageName: 'test',
         },
         mockOverlayData,
+        {
+          'design-overlay': DesignOverlay,
+        },
       );
+
+      // mock a design that overflows
       mockRefDimensions(
         wrapper.vm.$refs.presentationContainer,
         { width: 10, height: 10 },
@@ -478,15 +457,53 @@ describe('Design management design presentation component', () => {
         0,
         0,
       );
+    });
 
-      wrapper.element.scrollTo = jest.fn();
-      return clickDragExplore(
-        { clientX: 0, clientY: 0 },
-        { clientX: 10, clientY: 10 },
-        { useTouchEvents },
-      ).then(() => {
-        expect(wrapper.element.scrollTo).toHaveBeenCalledTimes(1);
-        expect(wrapper.element.scrollTo).toHaveBeenCalledWith(-10, -10);
+    it('opens a comment form if design was not dragged', () => {
+      const addCommentOverlay = findOverlayCommentButton();
+      const startCoords = {
+        clientX: 1,
+        clientY: 1,
+      };
+
+      addCommentOverlay.trigger('mousedown', {
+        clientX: startCoords.clientX,
+        clientY: startCoords.clientY,
+      });
+
+      return wrapper.vm
+        .$nextTick()
+        .then(() => {
+          addCommentOverlay.trigger('mouseup');
+          return wrapper.vm.$nextTick();
+        })
+        .then(() => {
+          expect(wrapper.emitted('openCommentForm')).toBeDefined();
+        });
+    });
+
+    describe('when clicking and dragging', () => {
+      it.each`
+        description               | useTouchEvents
+        ${'with touch events'}    | ${true}
+        ${'without touch events'} | ${false}
+      `('calls scrollTo with correct arguments $description', ({ useTouchEvents }) => {
+        return clickDragExplore(
+          { clientX: 0, clientY: 0 },
+          { clientX: 10, clientY: 10 },
+          { useTouchEvents },
+        ).then(() => {
+          expect(wrapper.element.scrollTo).toHaveBeenCalledTimes(1);
+          expect(wrapper.element.scrollTo).toHaveBeenCalledWith(-10, -10);
+        });
+      });
+
+      it('does not open a comment form', () => {
+        return clickDragExplore({ clientX: 0, clientY: 0 }, { clientX: 10, clientY: 10 }).then(
+          () => {
+            expect(wrapper.emitted('openCommentForm')).toBeFalsy();
+          },
+        );
       });
     });
   });