Changes image diff comments to use percents

This changes the positioning of the image diff comments
from absolute x & y positions to instead use percents.
These percents then mean that the comment bubble can
correctly adjust depending on the screen size.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/225961

app/assets/javascripts/diffs/components/diff_content.vue

@@ -173,12 +173,16 @@ export default {
         :a-mode="diffFile.a_mode"
         :b-mode="diffFile.b_mode"
       >
-        <image-diff-overlay
-          slot="image-overlay"
-          :discussions="imageDiscussions"
-          :file-hash="diffFileHash"
-          :can-comment="getNoteableData.current_user.can_create_note && !diffFile.brokenSymlink"
-        />
+        <template #image-overlay="{ renderedWidth, renderedHeight }">
+          <image-diff-overlay
+            v-if="renderedWidth"
+            :rendered-width="renderedWidth"
+            :rendered-height="renderedHeight"
+            :discussions="imageDiscussions"
+            :file-hash="diffFileHash"
+            :can-comment="getNoteableData.current_user.can_create_note && !diffFile.brokenSymlink"
+          />
+        </template>
         <div v-if="showNotesContainer" class="note-container">
           <user-avatar-link
             v-if="diffFileCommentForm && author"

app/assets/javascripts/diffs/components/image_diff_overlay.vue

@@ -4,6 +4,10 @@ import { isArray } from 'lodash';
 import imageDiffMixin from 'ee_else_ce/diffs/mixins/image_diff';
 import { GlIcon } from '@gitlab/ui';
 
+function calcPercent(pos, size, renderedSize) {
+  return (((pos / size) * 100) / ((renderedSize / size) * 100)) * 100;
+}
+
 export default {
   name: 'ImageDiffOverlay',
   components: {
@@ -39,6 +43,14 @@ export default {
       required: false,
       default: true,
     },
+    renderedWidth: {
+      type: Number,
+      required: true,
+    },
+    renderedHeight: {
+      type: Number,
+      required: true,
+    },
   },
   computed: {
     ...mapGetters('diffs', ['getDiffFileByHash', 'getCommentFormForDiffFile']),
@@ -59,33 +71,33 @@ export default {
     },
     getPositionForObject(meta) {
       const { x, y, width, height } = meta;
-      const imageWidth = this.getImageDimensions().width;
-      const imageHeight = this.getImageDimensions().height;
-      const widthRatio = imageWidth / width;
-      const heightRatio = imageHeight / height;
 
       return {
-        x: Math.round(x * widthRatio),
-        y: Math.round(y * heightRatio),
+        x: (x / width) * 100,
+        y: (y / height) * 100,
       };
     },
     getPosition(discussion) {
       const { x, y } = this.getPositionForObject(discussion.position);
 
       return {
-        left: `${x}px`,
-        top: `${y}px`,
+        left: `${x}%`,
+        top: `${y}%`,
       };
     },
     clickedImage(x, y) {
       const { width, height } = this.getImageDimensions();
+      const xPercent = calcPercent(x, width, this.renderedWidth);
+      const yPercent = calcPercent(y, height, this.renderedHeight);
 
       this.openDiffFileCommentForm({
         fileHash: this.fileHash,
         width,
         height,
-        x,
-        y,
+        x: width * (xPercent / 100),
+        y: height * (yPercent / 100),
+        xPercent,
+        yPercent,
       });
     },
   },
@@ -112,22 +124,19 @@ export default {
       type="button"
       @click="clickedToggle(discussion)"
     >
-      <gl-icon v-if="showCommentIcon" name="image-comment-dark" />
+      <gl-icon v-if="showCommentIcon" name="image-comment-dark" :size="24" />
       <template v-else>
         {{ toggleText(discussion, index) }}
       </template>
     </button>
     <button
-      v-if="currentCommentForm"
-      :style="{
-        left: `${currentCommentForm.x}px`,
-        top: `${currentCommentForm.y}px`,
-      }"
+      v-if="canComment && currentCommentForm"
+      :style="{ left: `${currentCommentForm.xPercent}%`, top: `${currentCommentForm.yPercent}%` }"
       :aria-label="__('Comment form position')"
-      class="btn-transparent comment-indicator"
+      class="btn-transparent comment-indicator position-absolute"
       type="button"
     >
-      <gl-icon name="image-comment-dark" />
+      <gl-icon name="image-comment-dark" :size="24" />
     </button>
   </div>
 </template>

app/assets/javascripts/notes/components/diff_with_note.vue

@@ -131,14 +131,18 @@ export default {
         :file-hash="discussion.diff_file.file_hash"
         :project-path="projectPath"
       >
-        <image-diff-overlay
-          slot="image-overlay"
-          :discussions="discussion"
-          :file-hash="discussion.diff_file.file_hash"
-          :show-comment-icon="true"
-          :should-toggle-discussion="false"
-          badge-class="image-comment-badge"
-        />
+        <template #image-overlay="{ renderedWidth, renderedHeight }">
+          <image-diff-overlay
+            v-if="renderedWidth"
+            :rendered-width="renderedWidth"
+            :rendered-height="renderedHeight"
+            :discussions="discussion"
+            :file-hash="discussion.diff_file.file_hash"
+            :show-comment-icon="true"
+            :should-toggle-discussion="false"
+            badge-class="image-comment-badge gl-text-gray-500"
+          />
+        </template>
       </diff-viewer>
       <slot></slot>
     </div>

app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue

@@ -28,6 +28,8 @@ export default {
     return {
       width: 0,
       height: 0,
+      renderedWidth: 0,
+      renderedHeight: 0,
     };
   },
   computed: {
@@ -63,11 +65,14 @@ export default {
         this.height = contentImg.naturalHeight;
 
         this.$nextTick(() => {
+          this.renderedWidth = contentImg.clientWidth;
+          this.renderedHeight = contentImg.clientHeight;
+
           this.$emit('imgLoaded', {
             width: this.width,
             height: this.height,
-            renderedWidth: contentImg.clientWidth,
-            renderedHeight: contentImg.clientHeight,
+            renderedWidth: this.renderedWidth,
+            renderedHeight: this.renderedHeight,
           });
         });
       }
@@ -79,7 +84,12 @@ export default {
 <template>
   <div data-testid="image-viewer">
     <div :class="innerCssClasses" class="position-relative">
-      <img ref="contentImg" :src="path" @load="onImgLoad" /> <slot name="image-overlay"></slot>
+      <img ref="contentImg" :src="path" @load="onImgLoad" />
+      <slot
+        name="image-overlay"
+        :rendered-width="renderedWidth"
+        :rendered-height="renderedHeight"
+      ></slot>
     </div>
     <p v-if="renderInfo" class="image-info">
       <template v-if="hasFileSize">

app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue

@@ -106,7 +106,13 @@ export default {
       :a-mode="aMode"
       :b-mode="bMode"
     >
-      <slot slot="image-overlay" name="image-overlay"></slot>
+      <template #image-overlay="{ renderedWidth, renderedHeight }">
+        <slot
+          :rendered-width="renderedWidth"
+          :rendered-height="renderedHeight"
+          name="image-overlay"
+        ></slot>
+      </template>
     </component>
     <slot></slot>
   </div>

app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue

@@ -141,7 +141,13 @@ export default {
           :path="newPath"
           @imgLoaded="onionNewImgLoaded"
         >
-          <slot slot="image-overlay" name="image-overlay"> </slot>
+          <template #image-overlay="{ renderedWidth, renderedHeight }">
+            <slot
+              :rendered-width="renderedWidth"
+              :rendered-height="renderedHeight"
+              name="image-overlay"
+            ></slot>
+          </template>
         </image-viewer>
       </div>
       <div class="controls">

app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue

@@ -143,7 +143,13 @@ export default {
           class="frame added"
           @imgLoaded="swipeNewImgLoaded"
         >
-          <slot slot="image-overlay" name="image-overlay"> </slot>
+          <template #image-overlay="{ renderedWidth, renderedHeight }">
+            <slot
+              :rendered-width="renderedWidth"
+              :rendered-height="renderedHeight"
+              name="image-overlay"
+            ></slot>
+          </template>
         </image-viewer>
       </div>
       <span

app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/two_up_viewer.vue

@@ -44,7 +44,13 @@ export default {
       :inner-css-classes="['frame', 'added']"
       class="wrap w-50"
     >
-      <slot slot="image-overlay" name="image-overlay"> </slot>
+      <template #image-overlay="{ renderedWidth, renderedHeight }">
+        <slot
+          :rendered-width="renderedWidth"
+          :rendered-height="renderedHeight"
+          name="image-overlay"
+        ></slot>
+      </template>
     </image-viewer>
   </div>
 </template>

app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue

@@ -76,7 +76,13 @@ export default {
     <div v-if="diffMode === $options.diffModes.replaced" class="diff-viewer">
       <div class="image js-replaced-image">
         <component :is="imageViewComponent" v-bind="$props">
-          <slot slot="image-overlay" name="image-overlay"> </slot>
+          <template #image-overlay="{ renderedWidth, renderedHeight }">
+            <slot
+              :rendered-width="renderedWidth"
+              :rendered-height="renderedHeight"
+              name="image-overlay"
+            ></slot>
+          </template>
         </component>
       </div>
       <div class="view-modes">
@@ -121,7 +127,13 @@ export default {
             },
           ]"
         >
-          <slot v-if="isNew || isRenamed" slot="image-overlay" name="image-overlay"> </slot>
+          <template v-if="isNew || isRenamed" #image-overlay="{ renderedWidth, renderedHeight }">
+            <slot
+              :rendered-width="renderedWidth"
+              :rendered-height="renderedHeight"
+              name="image-overlay"
+            ></slot>
+          </template>
         </image-viewer>
       </div>
     </div>

changelogs/unreleased/ph-225961-changeImageDiffBadgesPositionToPercent.yml

+---
+title: Fixed image diff comments positioning
+merge_request:
+author:
+type: fixed

spec/frontend/diffs/components/diff_content_spec.js

@@ -6,7 +6,6 @@ import InlineDiffView from '~/diffs/components/inline_diff_view.vue';
 import NotDiffableViewer from '~/vue_shared/components/diff_viewer/viewers/not_diffable.vue';
 import NoPreviewViewer from '~/vue_shared/components/diff_viewer/viewers/no_preview.vue';
 import ParallelDiffView from '~/diffs/components/parallel_diff_view.vue';
-import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue';
 import NoteForm from '~/notes/components/note_form.vue';
 import DiffDiscussions from '~/diffs/components/diff_discussions.vue';
 import { IMAGE_DIFF_POSITION_TYPE } from '~/diffs/constants';
@@ -167,14 +166,6 @@ describe('DiffContent', () => {
   describe('with image files', () => {
     const imageDiffFile = { ...defaultProps.diffFile, viewer: { name: diffViewerModes.image } };
 
-    it('should have image diff view in place', () => {
-      getCommentFormForDiffFileGetterMock.mockReturnValue(() => true);
-      createComponent({ props: { diffFile: imageDiffFile } });
-
-      expect(wrapper.find(InlineDiffView).exists()).toBe(false);
-      expect(wrapper.find(ImageDiffOverlay).exists()).toBe(true);
-    });
-
     it('renders diff file discussions', () => {
       getCommentFormForDiffFileGetterMock.mockReturnValue(() => true);
       createComponent({

spec/frontend/diffs/components/image_diff_overlay_spec.js

@@ -24,6 +24,8 @@ describe('Diffs image diff overlay component', () => {
       propsData: {
         discussions: [...imageDiffDiscussions],
         fileHash: 'ABC',
+        renderedWidth: 200,
+        renderedHeight: 200,
         ...props,
       },
       methods: {
@@ -71,8 +73,8 @@ describe('Diffs image diff overlay component', () => {
     createComponent();
     const imageBadges = getAllImageBadges();
 
-    expect(imageBadges.at(0).attributes('style')).toBe('left: 10px; top: 10px;');
-    expect(imageBadges.at(1).attributes('style')).toBe('left: 5px; top: 5px;');
+    expect(imageBadges.at(0).attributes('style')).toBe('left: 10%; top: 5%;');
+    expect(imageBadges.at(1).attributes('style')).toBe('left: 5%; top: 2.5%;');
   });
 
   it('renders single badge for discussion object', () => {
@@ -95,6 +97,8 @@ describe('Diffs image diff overlay component', () => {
       y: 0,
       width: 100,
       height: 200,
+      xPercent: 0,
+      yPercent: 0,
     });
   });
 
@@ -120,11 +124,13 @@ describe('Diffs image diff overlay component', () => {
   describe('comment form', () => {
     const getCommentIndicator = () => wrapper.find('.comment-indicator');
     beforeEach(() => {
-      createComponent({}, store => {
+      createComponent({ canComment: true }, store => {
         store.state.diffs.commentForms.push({
           fileHash: 'ABC',
           x: 20,
           y: 10,
+          xPercent: 10,
+          yPercent: 10,
         });
       });
     });
@@ -134,7 +140,7 @@ describe('Diffs image diff overlay component', () => {
     });
 
     it('sets comment form badge position', () => {
-      expect(getCommentIndicator().attributes('style')).toBe('left: 20px; top: 10px;');
+      expect(getCommentIndicator().attributes('style')).toBe('left: 10%; top: 10%;');
     });
   });
 });