From 9bc5552c11f6aca08c8c873a0561882b3e099350 Mon Sep 17 00:00:00 2001 From: Tyrone Yeh Date: Thu, 27 Jun 2024 17:31:49 +0800 Subject: [PATCH] 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 Co-authored-by: wxiaoguang --- .../js/features/comp/ComboMarkdownEditor.js | 10 +- web_src/js/features/comp/EditorMarkdown.js | 4 +- .../comp/{Paste.js => EditorUpload.js} | 124 ++++++++++-------- web_src/js/features/comp/EditorUpload.test.js | 14 ++ web_src/js/features/dropzone.js | 62 ++++++--- web_src/js/utils.js | 14 +- web_src/js/utils.test.js | 18 ++- web_src/js/utils/dom.js | 12 -- web_src/js/utils/image.js | 7 +- web_src/js/utils/image.test.js | 1 + 10 files changed, 169 insertions(+), 97 deletions(-) rename web_src/js/features/comp/{Paste.js => EditorUpload.js} (52%) create mode 100644 web_src/js/features/comp/EditorUpload.test.js diff --git a/web_src/js/features/comp/ComboMarkdownEditor.js b/web_src/js/features/comp/ComboMarkdownEditor.js index bd11c8383c..21779e32a8 100644 --- a/web_src/js/features/comp/ComboMarkdownEditor.js +++ b/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() { diff --git a/web_src/js/features/comp/EditorMarkdown.js b/web_src/js/features/comp/EditorMarkdown.js index cf412e3807..9ec71aba74 100644 --- a/web_src/js/features/comp/EditorMarkdown.js +++ b/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; diff --git a/web_src/js/features/comp/Paste.js b/web_src/js/features/comp/EditorUpload.js similarity index 52% rename from web_src/js/features/comp/Paste.js rename to web_src/js/features/comp/EditorUpload.js index c72434c4cc..8861abfe03 100644 --- a/web_src/js/features/comp/Paste.js +++ b/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) { - const formData = new FormData(); - formData.append('file', file, file.name); +let uploadIdCounter = 0; - const res = await POST(uploadUrl, {data: formData}); - return await res.json(); -} - -export function triggerEditorContentChanged(target) { - target.dispatchEvent(new CustomEvent('ce-editor-content-changed', {bubbles: true})); +function uploadFile(dropzoneEl, file) { + return new Promise((resolve) => { + const curUploadId = uploadIdCounter++; + file._giteaUploadId = curUploadId; + const dropzoneInst = dropzoneEl.dropzone; + 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) { - const uploadUrl = dropzone.getAttribute('data-upload-url'); - const filesContainer = dropzone.querySelector('.files'); - - if (!dropzone || !uploadUrl || !filesContainer || !images.length) return; - +async function handleUploadFiles(editor, dropzoneEl, files, e) { 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); - - const {uuid} = await uploadFile(img, uploadUrl); - const {width, dppx} = await imageInfo(img); - - let text; - if (width > 0 && dppx > 1) { - // Scale down images from HiDPI monitors. This uses the 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 = `${htmlEscape(name)}`; - } 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); + await uploadFile(dropzoneEl, file); // the "file" will get its "uuid" during the upload + editor.replacePlaceholder(placeholder, generateMarkdownLinkForAttachment(file, {width, dppx})); } } +export function removeAttachmentLinksFromMarkdown(text, fileUuid) { + text = text.replace(new RegExp(`!?\\[([^\\]]+)\\]\\(/?attachments/${fileUuid}\\)`, 'g'), ''); + text = text.replace(new RegExp(`]+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) { - handleClipboardImages(new CodeMirrorEditor(easyMDE.codemirror), dropzone, images, e); - } + if (!images.length) return; + 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; + }); } diff --git a/web_src/js/features/comp/EditorUpload.test.js b/web_src/js/features/comp/EditorUpload.test.js new file mode 100644 index 0000000000..caecfb91ea --- /dev/null +++ b/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 b', 'foo')).toBe('a b'); + expect(removeAttachmentLinksFromMarkdown('a b', 'foo')).toBe('a b'); + expect(removeAttachmentLinksFromMarkdown('a b', 'foo')).toBe('a b'); + expect(removeAttachmentLinksFromMarkdown('a b', 'foo')).toBe('a b'); +}); diff --git a/web_src/js/features/dropzone.js b/web_src/js/features/dropzone.js index 8d70fc774b..f25a613718 100644 --- a/web_src/js/features/dropzone.js +++ 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 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 = `${htmlEscape(file.name)}`; + } 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 = ``; + } + return fileMarkdown; +} + function addCopyLink(file) { // Create a "Copy Link" element, to conveniently copy the image or file link as Markdown to the clipboard // The "" element has a hardcoded cursor: pointer because the default is overridden by .dropzone @@ -25,13 +51,7 @@ function addCopyLink(file) { `); copyLinkEl.addEventListener('click', async (e) => { e.preventDefault(); - let fileMarkdown = `[${file.name}](/attachments/${file.uuid})`; - if (file.type?.startsWith('image/')) { - fileMarkdown = `!${fileMarkdown}`; - } else if (file.type?.startsWith('video/')) { - fileMarkdown = ``; - } - const success = await clippie(fileMarkdown); + const success = await clippie(generateMarkdownLinkForAttachment(file)); 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 '' const dzInst = await createDropzone(dropzoneEl, opts); - dzInst.on('success', (file, data) => { - file.uuid = data.uuid; + dzInst.on('success', (file, resp) => { + 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}`; - dzInst.emit('addedfile', attachment); - dzInst.emit('thumbnail', attachment, imgSrc); - dzInst.emit('complete', attachment); - addCopyLink(attachment); - fileUuidDict[attachment.uuid] = {submitted: true}; - const input = createElementFromAttrs('input', {name: 'files', type: 'hidden', id: `dropzone-file-${attachment.uuid}`, value: attachment.uuid}); + const file = {name: attachment.name, uuid: attachment.uuid, size: attachment.size}; + const imgSrc = `${attachmentBaseLinkUrl}/${file.uuid}`; + dzInst.emit('addedfile', file); + dzInst.emit('thumbnail', file, imgSrc); + dzInst.emit('complete', file); + addCopyLink(file); // it is from server response, so no "type" + 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; } diff --git a/web_src/js/utils.js b/web_src/js/utils.js index ce0fb66343..2d40fa20a8 100644 --- a/web_src/js/utils.js +++ b/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/'); +} diff --git a/web_src/js/utils.test.js b/web_src/js/utils.test.js index 2754e41c43..1ec3d3630b 100644 --- a/web_src/js/utils.test.js +++ b/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(); + } +}); diff --git a/web_src/js/utils/dom.js b/web_src/js/utils/dom.js index 57c7c8796a..6a38968899 100644 --- a/web_src/js/utils/dom.js +++ b/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); diff --git a/web_src/js/utils/image.js b/web_src/js/utils/image.js index ed5d98e35a..c71d715941 100644 --- a/web_src/js/utils/image.js +++ b/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 dppx = 1; // 1 dot per pixel for non-HiDPI screens + let width = 0, dppx = 1; // dppx: 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}; diff --git a/web_src/js/utils/image.test.js b/web_src/js/utils/image.test.js index ba4758250c..af56ed2331 100644 --- a/web_src/js/utils/image.test.js +++ b/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({}); });