Improve attachment upload methods (#30513)

* Use dropzone to handle file uploading for all cases, including pasting
and dragging
* Merge duplicate code, use consistent behavior for link generating

Close #20130

---------

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

web_src/js/features/comp/ComboMarkdownEditor.js

@@ -3,7 +3,7 @@ import '@github/text-expander-element';
 import $ from 'jquery';
 import {attachTribute} from '../tribute.js';
 import {hideElem, showElem, autosize, isElemVisible} from '../../utils/dom.js';
-import {initEasyMDEPaste, initTextareaPaste} from './Paste.js';
+import {initEasyMDEPaste, initTextareaUpload} from './EditorUpload.js';
 import {handleGlobalEnterQuickSubmit} from './QuickSubmit.js';
 import {renderPreviewPanelContent} from '../repo-editor.js';
 import {easyMDEToolbarActions} from './EasyMDEToolbarActions.js';
@@ -11,7 +11,7 @@ import {initTextExpander} from './TextExpander.js';
 import {showErrorToast} from '../../modules/toast.js';
 import {POST} from '../../modules/fetch.js';
 import {initTextareaMarkdown} from './EditorMarkdown.js';
-import {initDropzone} from '../dropzone.js';
+import {DropzoneCustomEventReloadFiles, initDropzone} from '../dropzone.js';
 
 let elementIdCounter = 0;
 
@@ -111,7 +111,7 @@ class ComboMarkdownEditor {
 
     initTextareaMarkdown(this.textarea);
     if (this.dropzone) {
-      initTextareaPaste(this.textarea, this.dropzone);
+      initTextareaUpload(this.textarea, this.dropzone);
     }
   }
 
@@ -130,13 +130,13 @@ class ComboMarkdownEditor {
 
   dropzoneReloadFiles() {
     if (!this.dropzone) return;
-    this.attachedDropzoneInst.emit('reload');
+    this.attachedDropzoneInst.emit(DropzoneCustomEventReloadFiles);
   }
 
   dropzoneSubmitReload() {
     if (!this.dropzone) return;
     this.attachedDropzoneInst.emit('submit');
-    this.attachedDropzoneInst.emit('reload');
+    this.attachedDropzoneInst.emit(DropzoneCustomEventReloadFiles);
   }
 
   setupTab() {

web_src/js/features/comp/EditorMarkdown.js

@@ -1,4 +1,6 @@
-import {triggerEditorContentChanged} from './Paste.js';
+export function triggerEditorContentChanged(target) {
+  target.dispatchEvent(new CustomEvent('ce-editor-content-changed', {bubbles: true}));
+}
 
 function handleIndentSelection(textarea, e) {
   const selStart = textarea.selectionStart;

web_src/js/features/comp/EditorUpload.js

@@ -1,19 +1,29 @@
-import {htmlEscape} from 'escape-goat';
-import {POST} from '../../modules/fetch.js';
 import {imageInfo} from '../../utils/image.js';
-import {getPastedContent, replaceTextareaSelection} from '../../utils/dom.js';
+import {replaceTextareaSelection} from '../../utils/dom.js';
 import {isUrl} from '../../utils/url.js';
+import {triggerEditorContentChanged} from './EditorMarkdown.js';
+import {
+  DropzoneCustomEventRemovedFile,
+  DropzoneCustomEventUploadDone,
+  generateMarkdownLinkForAttachment,
+} from '../dropzone.js';
 
-async function uploadFile(file, uploadUrl) {
+let uploadIdCounter = 0;
-  const formData = new FormData();
-  formData.append('file', file, file.name);
 
-  const res = await POST(uploadUrl, {data: formData});
+function uploadFile(dropzoneEl, file) {
-  return await res.json();
+  return new Promise((resolve) => {
-}
+    const curUploadId = uploadIdCounter++;
-
+    file._giteaUploadId = curUploadId;
-export function triggerEditorContentChanged(target) {
+    const dropzoneInst = dropzoneEl.dropzone;
-  target.dispatchEvent(new CustomEvent('ce-editor-content-changed', {bubbles: true}));
+    const onUploadDone = ({file}) => {
+      if (file._giteaUploadId === curUploadId) {
+        dropzoneInst.off(DropzoneCustomEventUploadDone, onUploadDone);
+        resolve();
+      }
+    };
+    dropzoneInst.on(DropzoneCustomEventUploadDone, onUploadDone);
+    dropzoneInst.handleFiles([file]);
+  });
 }
 
 class TextareaEditor {
@@ -82,48 +92,25 @@ class CodeMirrorEditor {
   }
 }
 
-async function handleClipboardImages(editor, dropzone, images, e) {
+async function handleUploadFiles(editor, dropzoneEl, files, e) {
-  const uploadUrl = dropzone.getAttribute('data-upload-url');
-  const filesContainer = dropzone.querySelector('.files');
-
-  if (!dropzone || !uploadUrl || !filesContainer || !images.length) return;
-
   e.preventDefault();
-  e.stopPropagation();
+  for (const file of files) {
+    const name = file.name.slice(0, file.name.lastIndexOf('.'));
+    const {width, dppx} = await imageInfo(file);
+    const placeholder = `[${name}](uploading ...)`;
 
-  for (const img of images) {
-    const name = img.name.slice(0, img.name.lastIndexOf('.'));
-
-    const placeholder = `![${name}](uploading ...)`;
     editor.insertPlaceholder(placeholder);
-
+    await uploadFile(dropzoneEl, file); // the "file" will get its "uuid" during the upload
-    const {uuid} = await uploadFile(img, uploadUrl);
+    editor.replacePlaceholder(placeholder, generateMarkdownLinkForAttachment(file, {width, dppx}));
-    const {width, dppx} = await imageInfo(img);
-
-    let text;
-    if (width > 0 && dppx > 1) {
-      // Scale down images from HiDPI monitors. This uses the <img> tag because it's the only
-      // method to change image size in Markdown that is supported by all implementations.
-      // Make the image link relative to the repo path, then the final URL is "/sub-path/owner/repo/attachments/{uuid}"
-      const url = `attachments/${uuid}`;
-      text = `<img width="${Math.round(width / dppx)}" alt="${htmlEscape(name)}" src="${htmlEscape(url)}">`;
-    } else {
-      // Markdown always renders the image with a relative path, so the final URL is "/sub-path/owner/repo/attachments/{uuid}"
-      // TODO: it should also use relative path for consistency, because absolute is ambiguous for "/sub-path/attachments" or "/attachments"
-      const url = `/attachments/${uuid}`;
-      text = `![${name}](${url})`;
-    }
-    editor.replacePlaceholder(placeholder, text);
-
-    const input = document.createElement('input');
-    input.setAttribute('name', 'files');
-    input.setAttribute('type', 'hidden');
-    input.setAttribute('id', uuid);
-    input.value = uuid;
-    filesContainer.append(input);
   }
 }
 
+export function removeAttachmentLinksFromMarkdown(text, fileUuid) {
+  text = text.replace(new RegExp(`!?\\[([^\\]]+)\\]\\(/?attachments/${fileUuid}\\)`, 'g'), '');
+  text = text.replace(new RegExp(`<img[^>]+src="/?attachments/${fileUuid}"[^>]*>`, 'g'), '');
+  return text;
+}
+
 function handleClipboardText(textarea, e, {text, isShiftDown}) {
   // pasting with "shift" means "paste as original content" in most applications
   if (isShiftDown) return; // let the browser handle it
@@ -139,16 +126,37 @@ function handleClipboardText(textarea, e, {text, isShiftDown}) {
   // else, let the browser handle it
 }
 
-export function initEasyMDEPaste(easyMDE, dropzone) {
+// extract text and images from "paste" event
+function getPastedContent(e) {
+  const images = [];
+  for (const item of e.clipboardData?.items ?? []) {
+    if (item.type?.startsWith('image/')) {
+      images.push(item.getAsFile());
+    }
+  }
+  const text = e.clipboardData?.getData?.('text') ?? '';
+  return {text, images};
+}
+
+export function initEasyMDEPaste(easyMDE, dropzoneEl) {
+  const editor = new CodeMirrorEditor(easyMDE.codemirror);
   easyMDE.codemirror.on('paste', (_, e) => {
     const {images} = getPastedContent(e);
-    if (images.length) {
+    if (!images.length) return;
-      handleClipboardImages(new CodeMirrorEditor(easyMDE.codemirror), dropzone, images, e);
+    handleUploadFiles(editor, dropzoneEl, images, e);
-    }
+  });
+  easyMDE.codemirror.on('drop', (_, e) => {
+    if (!e.dataTransfer.files.length) return;
+    handleUploadFiles(editor, dropzoneEl, e.dataTransfer.files, e);
+  });
+  dropzoneEl.dropzone.on(DropzoneCustomEventRemovedFile, ({fileUuid}) => {
+    const oldText = easyMDE.codemirror.getValue();
+    const newText = removeAttachmentLinksFromMarkdown(oldText, fileUuid);
+    if (oldText !== newText) easyMDE.codemirror.setValue(newText);
   });
 }
 
-export function initTextareaPaste(textarea, dropzone) {
+export function initTextareaUpload(textarea, dropzoneEl) {
   let isShiftDown = false;
   textarea.addEventListener('keydown', (e) => {
     if (e.shiftKey) isShiftDown = true;
@@ -159,9 +167,17 @@ export function initTextareaPaste(textarea, dropzone) {
   textarea.addEventListener('paste', (e) => {
     const {images, text} = getPastedContent(e);
     if (images.length) {
-      handleClipboardImages(new TextareaEditor(textarea), dropzone, images, e);
+      handleUploadFiles(new TextareaEditor(textarea), dropzoneEl, images, e);
     } else if (text) {
       handleClipboardText(textarea, e, {text, isShiftDown});
     }
   });
+  textarea.addEventListener('drop', (e) => {
+    if (!e.dataTransfer.files.length) return;
+    handleUploadFiles(new TextareaEditor(textarea), dropzoneEl, e.dataTransfer.files, e);
+  });
+  dropzoneEl.dropzone.on(DropzoneCustomEventRemovedFile, ({fileUuid}) => {
+    const newText = removeAttachmentLinksFromMarkdown(textarea.value, fileUuid);
+    if (textarea.value !== newText) textarea.value = newText;
+  });
 }

web_src/js/features/comp/EditorUpload.test.js

@@ -0,0 +1,14 @@
+import {removeAttachmentLinksFromMarkdown} from './EditorUpload.js';
+
+test('removeAttachmentLinksFromMarkdown', () => {
+  expect(removeAttachmentLinksFromMarkdown('a foo b', 'foo')).toBe('a foo b');
+  expect(removeAttachmentLinksFromMarkdown('a [x](attachments/foo) b', 'foo')).toBe('a  b');
+  expect(removeAttachmentLinksFromMarkdown('a ![x](attachments/foo) b', 'foo')).toBe('a  b');
+  expect(removeAttachmentLinksFromMarkdown('a [x](/attachments/foo) b', 'foo')).toBe('a  b');
+  expect(removeAttachmentLinksFromMarkdown('a ![x](/attachments/foo) b', 'foo')).toBe('a  b');
+
+  expect(removeAttachmentLinksFromMarkdown('a <img src="attachments/foo"> b', 'foo')).toBe('a  b');
+  expect(removeAttachmentLinksFromMarkdown('a <img width="100" src="attachments/foo"> b', 'foo')).toBe('a  b');
+  expect(removeAttachmentLinksFromMarkdown('a <img src="/attachments/foo"> b', 'foo')).toBe('a  b');
+  expect(removeAttachmentLinksFromMarkdown('a <img src="/attachments/foo" width="100"/> b', 'foo')).toBe('a  b');
+});

web_src/js/features/dropzone.js

@@ -5,9 +5,15 @@ import {showTemporaryTooltip} from '../modules/tippy.js';
 import {GET, POST} from '../modules/fetch.js';
 import {showErrorToast} from '../modules/toast.js';
 import {createElementFromHTML, createElementFromAttrs} from '../utils/dom.js';
+import {isImageFile, isVideoFile} from '../utils.js';
 
 const {csrfToken, i18n} = window.config;
 
+// dropzone has its owner event dispatcher (emitter)
+export const DropzoneCustomEventReloadFiles = 'dropzone-custom-reload-files';
+export const DropzoneCustomEventRemovedFile = 'dropzone-custom-removed-file';
+export const DropzoneCustomEventUploadDone = 'dropzone-custom-upload-done';
+
 async function createDropzone(el, opts) {
   const [{Dropzone}] = await Promise.all([
     import(/* webpackChunkName: "dropzone" */'dropzone'),
@@ -16,6 +22,26 @@ async function createDropzone(el, opts) {
   return new Dropzone(el, opts);
 }
 
+export function generateMarkdownLinkForAttachment(file, {width, dppx} = {}) {
+  let fileMarkdown = `[${file.name}](/attachments/${file.uuid})`;
+  if (isImageFile(file)) {
+    fileMarkdown = `!${fileMarkdown}`;
+    if (width > 0 && dppx > 1) {
+      // Scale down images from HiDPI monitors. This uses the <img> tag because it's the only
+      // method to change image size in Markdown that is supported by all implementations.
+      // Make the image link relative to the repo path, then the final URL is "/sub-path/owner/repo/attachments/{uuid}"
+      fileMarkdown = `<img width="${Math.round(width / dppx)}" alt="${htmlEscape(file.name)}" src="attachments/${htmlEscape(file.uuid)}">`;
+    } else {
+      // Markdown always renders the image with a relative path, so the final URL is "/sub-path/owner/repo/attachments/{uuid}"
+      // TODO: it should also use relative path for consistency, because absolute is ambiguous for "/sub-path/attachments" or "/attachments"
+      fileMarkdown = `![${file.name}](/attachments/${file.uuid})`;
+    }
+  } else if (isVideoFile(file)) {
+    fileMarkdown = `<video src="attachments/${htmlEscape(file.uuid)}" title="${htmlEscape(file.name)}" controls></video>`;
+  }
+  return fileMarkdown;
+}
+
 function addCopyLink(file) {
   // Create a "Copy Link" element, to conveniently copy the image or file link as Markdown to the clipboard
   // The "<a>" element has a hardcoded cursor: pointer because the default is overridden by .dropzone
@@ -25,13 +51,7 @@ function addCopyLink(file) {
 </div>`);
   copyLinkEl.addEventListener('click', async (e) => {
     e.preventDefault();
-    let fileMarkdown = `[${file.name}](/attachments/${file.uuid})`;
+    const success = await clippie(generateMarkdownLinkForAttachment(file));
-    if (file.type?.startsWith('image/')) {
-      fileMarkdown = `!${fileMarkdown}`;
-    } else if (file.type?.startsWith('video/')) {
-      fileMarkdown = `<video src="/attachments/${htmlEscape(file.uuid)}" title="${htmlEscape(file.name)}" controls></video>`;
-    }
-    const success = await clippie(fileMarkdown);
     showTemporaryTooltip(e.target, success ? i18n.copy_success : i18n.copy_error);
   });
   file.previewTemplate.append(copyLinkEl);
@@ -68,16 +88,19 @@ export async function initDropzone(dropzoneEl) {
   // "http://localhost:3000/owner/repo/issues/[object%20Event]"
   // the reason is that the preview "callback(dataURL)" is assign to "img.onerror" then "thumbnail" uses the error object as the dataURL and generates '<img src="[object Event]">'
   const dzInst = await createDropzone(dropzoneEl, opts);
-  dzInst.on('success', (file, data) => {
+  dzInst.on('success', (file, resp) => {
-    file.uuid = data.uuid;
+    file.uuid = resp.uuid;
     fileUuidDict[file.uuid] = {submitted: false};
-    const input = createElementFromAttrs('input', {name: 'files', type: 'hidden', id: `dropzone-file-${data.uuid}`, value: data.uuid});
+    const input = createElementFromAttrs('input', {name: 'files', type: 'hidden', id: `dropzone-file-${resp.uuid}`, value: resp.uuid});
     dropzoneEl.querySelector('.files').append(input);
     addCopyLink(file);
+    dzInst.emit(DropzoneCustomEventUploadDone, {file});
   });
 
   dzInst.on('removedfile', async (file) => {
     if (disableRemovedfileEvent) return;
+
+    dzInst.emit(DropzoneCustomEventRemovedFile, {fileUuid: file.uuid});
     document.querySelector(`#dropzone-file-${file.uuid}`)?.remove();
     // when the uploaded file number reaches the limit, there is no uuid in the dict, and it doesn't need to be removed from server
     if (removeAttachmentUrl && fileUuidDict[file.uuid] && !fileUuidDict[file.uuid].submitted) {
@@ -91,7 +114,7 @@ export async function initDropzone(dropzoneEl) {
     }
   });
 
-  dzInst.on('reload', async () => {
+  dzInst.on(DropzoneCustomEventReloadFiles, async () => {
     try {
       const resp = await GET(listAttachmentsUrl);
       const respData = await resp.json();
@@ -104,13 +127,14 @@ export async function initDropzone(dropzoneEl) {
       for (const el of dropzoneEl.querySelectorAll('.dz-preview')) el.remove();
       fileUuidDict = {};
       for (const attachment of respData) {
-        const imgSrc = `${attachmentBaseLinkUrl}/${attachment.uuid}`;
+        const file = {name: attachment.name, uuid: attachment.uuid, size: attachment.size};
-        dzInst.emit('addedfile', attachment);
+        const imgSrc = `${attachmentBaseLinkUrl}/${file.uuid}`;
-        dzInst.emit('thumbnail', attachment, imgSrc);
+        dzInst.emit('addedfile', file);
-        dzInst.emit('complete', attachment);
+        dzInst.emit('thumbnail', file, imgSrc);
-        addCopyLink(attachment);
+        dzInst.emit('complete', file);
-        fileUuidDict[attachment.uuid] = {submitted: true};
+        addCopyLink(file); // it is from server response, so no "type"
-        const input = createElementFromAttrs('input', {name: 'files', type: 'hidden', id: `dropzone-file-${attachment.uuid}`, value: attachment.uuid});
+        fileUuidDict[file.uuid] = {submitted: true};
+        const input = createElementFromAttrs('input', {name: 'files', type: 'hidden', id: `dropzone-file-${file.uuid}`, value: file.uuid});
         dropzoneEl.querySelector('.files').append(input);
       }
       if (!dropzoneEl.querySelector('.dz-preview')) {
@@ -129,6 +153,6 @@ export async function initDropzone(dropzoneEl) {
     dzInst.removeFile(file);
   });
 
-  if (listAttachmentsUrl) dzInst.emit('reload');
+  if (listAttachmentsUrl) dzInst.emit(DropzoneCustomEventReloadFiles);
   return dzInst;
 }

web_src/js/utils.js

@@ -1,14 +1,16 @@
 import {encode, decode} from 'uint8-to-base64';
 
 // transform /path/to/file.ext to file.ext
-export function basename(path = '') {
+export function basename(path) {
   const lastSlashIndex = path.lastIndexOf('/');
   return lastSlashIndex < 0 ? path : path.substring(lastSlashIndex + 1);
 }
 
 // transform /path/to/file.ext to .ext
-export function extname(path = '') {
+export function extname(path) {
+  const lastSlashIndex = path.lastIndexOf('/');
   const lastPointIndex = path.lastIndexOf('.');
+  if (lastSlashIndex > lastPointIndex) return '';
   return lastPointIndex < 0 ? '' : path.substring(lastPointIndex);
 }
 
@@ -142,3 +144,11 @@ export function serializeXml(node) {
 }
 
 export const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
+
+export function isImageFile({name, type}) {
+  return /\.(jpe?g|png|gif|webp|svg|heic)$/i.test(name || '') || type?.startsWith('image/');
+}
+
+export function isVideoFile({name, type}) {
+  return /\.(mpe?g|mp4|mkv|webm)$/i.test(name || '') || type?.startsWith('video/');
+}

web_src/js/utils.test.js

@@ -1,7 +1,7 @@
 import {
   basename, extname, isObject, stripTags, parseIssueHref,
   parseUrl, translateMonth, translateDay, blobToDataURI,
-  toAbsoluteUrl, encodeURLEncodedBase64, decodeURLEncodedBase64,
+  toAbsoluteUrl, encodeURLEncodedBase64, decodeURLEncodedBase64, isImageFile, isVideoFile,
 } from './utils.js';
 
 test('basename', () => {
@@ -15,6 +15,7 @@ test('extname', () => {
   expect(extname('/path/')).toEqual('');
   expect(extname('/path')).toEqual('');
   expect(extname('file.js')).toEqual('.js');
+  expect(extname('/my.path/file')).toEqual('');
 });
 
 test('isObject', () => {
@@ -112,3 +113,18 @@ test('encodeURLEncodedBase64, decodeURLEncodedBase64', () => {
   expect(Array.from(decodeURLEncodedBase64('YQ'))).toEqual(Array.from(uint8array('a')));
   expect(Array.from(decodeURLEncodedBase64('YQ=='))).toEqual(Array.from(uint8array('a')));
 });
+
+test('file detection', () => {
+  for (const name of ['a.jpg', '/a.jpeg', '.file.png', '.webp', 'file.svg']) {
+    expect(isImageFile({name})).toBeTruthy();
+  }
+  for (const name of ['', 'a.jpg.x', '/path.png/x', 'webp']) {
+    expect(isImageFile({name})).toBeFalsy();
+  }
+  for (const name of ['a.mpg', '/a.mpeg', '.file.mp4', '.webm', 'file.mkv']) {
+    expect(isVideoFile({name})).toBeTruthy();
+  }
+  for (const name of ['', 'a.mpg.x', '/path.mp4/x', 'webm']) {
+    expect(isVideoFile({name})).toBeFalsy();
+  }
+});

web_src/js/utils/dom.js

@@ -262,18 +262,6 @@ export function isElemVisible(element) {
   return Boolean(element.offsetWidth || element.offsetHeight || element.getClientRects().length);
 }
 
-// extract text and images from "paste" event
-export function getPastedContent(e) {
-  const images = [];
-  for (const item of e.clipboardData?.items ?? []) {
-    if (item.type?.startsWith('image/')) {
-      images.push(item.getAsFile());
-    }
-  }
-  const text = e.clipboardData?.getData?.('text') ?? '';
-  return {text, images};
-}
-
 // replace selected text in a textarea while preserving editor history, e.g. CTRL-Z works after this
 export function replaceTextareaSelection(textarea, text) {
   const before = textarea.value.slice(0, textarea.selectionStart ?? undefined);

web_src/js/utils/image.js

@@ -19,11 +19,10 @@ export async function pngChunks(blob) {
   return chunks;
 }
 
-// decode a image and try to obtain width and dppx. If will never throw but instead
+// decode a image and try to obtain width and dppx. It will never throw but instead
 // return default values.
 export async function imageInfo(blob) {
-  let width = 0; // 0 means no width could be determined
+  let width = 0, dppx = 1; // dppx: 1 dot per pixel for non-HiDPI screens
-  let dppx = 1; // 1 dot per pixel for non-HiDPI screens
 
   if (blob.type === 'image/png') { // only png is supported currently
     try {
@@ -41,6 +40,8 @@ export async function imageInfo(blob) {
         }
       }
     } catch {}
+  } else {
+    return {}; // no image info for non-image files
   }
 
   return {width, dppx};

web_src/js/utils/image.test.js

@@ -26,4 +26,5 @@ test('imageInfo', async () => {
   expect(await imageInfo(await dataUriToBlob(pngNoPhys))).toEqual({width: 1, dppx: 1});
   expect(await imageInfo(await dataUriToBlob(pngPhys))).toEqual({width: 2, dppx: 2});
   expect(await imageInfo(await dataUriToBlob(pngEmpty))).toEqual({width: 0, dppx: 1});
+  expect(await imageInfo(await dataUriToBlob(`data:image/gif;base64,`))).toEqual({});
 });