Merge pull request #808 from Expensify/michal-img-attributes

Added img attribute caching to ExpensiMark
Expensify · Oct 7, 2024 · fb19129 · fb19129
2 parents 1373f28 + e0f9c4a
commit fb19129
Show file tree

Hide file tree

Showing 4 changed files with 230 additions and 15 deletions.
diff --git a/__tests__/ExpensiMark-HTML-test.js b/__tests__/ExpensiMark-HTML-test.js
@@ -2100,7 +2100,7 @@ describe('Video markdown conversion to html tag', () => {
         expect(parser.replace(testString, {shouldKeepRawInput: true})).toBe(resultString);
     })
 
-    test('Single video with extra cached attribues', () => {
+    test('Single video with extra cached attributes with videoAttributeCache', () => {
         const testString = '![test](https://example.com/video.mp4)';
         const resultString = '<video data-expensify-source="https://example.com/video.mp4" data-expensify-height="100" data-expensify-width="100">test</video>';
         expect(parser.replace(testString, {
@@ -2112,6 +2112,18 @@ describe('Video markdown conversion to html tag', () => {
         })).toBe(resultString);
     })
 
+    test('Single video with extra cached attributes with mediaAttributeCache', () => {
+        const testString = '![test](https://example.com/video.mp4)';
+        const resultString = '<video data-expensify-source="https://example.com/video.mp4" data-expensify-height="100" data-expensify-width="100">test</video>';
+        expect(parser.replace(testString, {
+            extras: {
+                mediaAttributeCache: {
+                    'https://example.com/video.mp4': 'data-expensify-height="100" data-expensify-width="100"'
+                }
+            }
+        })).toBe(resultString);
+    })
+
     test('Text containing image and video', () => {
         const testString = 'An image of a banana: ![banana](https://example.com/banana.png) and a video of a banana: ![banana](https://example.com/banana.mp4)';
         const resultString = 'An image of a banana: <img src="https://example.com/banana.png" alt="banana" /> and a video of a banana: <video data-expensify-source="https://example.com/banana.mp4" >banana</video>';
@@ -2223,6 +2235,31 @@ describe('Image markdown conversion to html tag', () => {
             '<img src="https://example.com/image.png" alt="&#x60;&#x60;&#x60;code&#x60;&#x60;&#x60;" data-raw-href="https://example.com/image.png" data-link-variant="labeled" />';
         expect(parser.replace(testString, {shouldKeepRawInput: true})).toBe(resultString);
     });
+
+    test('Single image with extra cached attributes using mediaAttributeCache', () => {
+        const testString = '![test](https://example.com/image.jpg)';
+        const resultString = '<img src="https://example.com/image.jpg" alt="test" data-expensify-height="100" data-expensify-width="100"/>';
+        expect(parser.replace(testString, {
+            extras: {
+                mediaAttributeCache: {
+                    'https://example.com/image.jpg': 'data-expensify-height="100" data-expensify-width="100"'
+                }
+            }
+        })).toBe(resultString);
+    })
+
+    test('Single image with extra cached attributes using videAttributeCache', () => {
+        const testString = '![test](https://example.com/image.jpg)';
+        const resultString = '<img src="https://example.com/image.jpg" alt="test" data-expensify-height="100" data-expensify-width="100"/>';
+        expect(parser.replace(testString, {
+            extras: {
+                videoAttributeCache: {
+                    'https://example.com/image.jpg': 'data-expensify-height="100" data-expensify-width="100"'
+                }
+            }
+        })).toBe(resultString);
+    })
+
 });
 
 describe('room mentions', () => {

diff --git a/__tests__/ExpensiMark-Markdown-test.js b/__tests__/ExpensiMark-Markdown-test.js
@@ -867,6 +867,50 @@ describe('Image tag conversion to markdown', () => {
         const resultString = '![```code```](https://example.com/image.png)';
         expect(parser.htmlToMarkdown(testString)).toBe(resultString);
     });
+
+    test('Cache extra attributes for img with alt with mediaAttributeCachingFn', () => {
+        const mediaAttributeCachingFn = jest.fn();
+        const testString = '<img src="https://example.com/image.png" alt="altText" data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source" />';
+        const resultString = '![altText](https://example.com/image.png)';
+        const extras = {
+            mediaAttributeCachingFn,
+        };
+        expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString);
+        expect(mediaAttributeCachingFn).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"')
+    });
+
+    test('Cache extra attributes for img with alt with cacheVideoAttributes', () => {
+        const cacheVideoAttributes = jest.fn();
+        const testString = '<img src="https://example.com/image.png" alt="altText" data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source" />';
+        const resultString = '![altText](https://example.com/image.png)';
+        const extras = {
+            cacheVideoAttributes,
+        };
+        expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString);
+        expect(cacheVideoAttributes).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"')
+    });
+
+    test('Cache extra attributes for img without alt with mediaAttributeCachingFn', () => {
+        const mediaAttributeCachingFn = jest.fn();
+        const testString = '<img src="https://example.com/image.png" data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source" />';
+        const resultString = '!(https://example.com/image.png)';
+        const extras = {
+            mediaAttributeCachingFn,
+        };
+        expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString);
+        expect(mediaAttributeCachingFn).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"')
+    });
+
+    test('Cache extra attributes for img without alt with cacheVideoAttributes', () => {
+        const cacheVideoAttributes = jest.fn();
+        const testString = '<img src="https://example.com/image.png" data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source" />';
+        const resultString = '!(https://example.com/image.png)';
+        const extras = {
+            cacheVideoAttributes,
+        };
+        expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString);
+        expect(cacheVideoAttributes).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"')
+    });
 });
 
 describe('Video tag conversion to markdown', () => {
@@ -882,7 +926,7 @@ describe('Video tag conversion to markdown', () => {
         expect(parser.htmlToMarkdown(testString)).toBe(resultString);
     })
 
-    test('While convert video, cache some extra attributes from the video tag', () => {
+    test('Video with extra attributes to be cached with cacheVideoAttributes', () => {
         const cacheVideoAttributes = jest.fn();
         const testString = '<video data-expensify-source="https://example.com/video.mp4" data-expensify-width="100" data-expensify-height="500" data-expensify-thumbnail-url="https://image.com/img.jpg">video</video>';
         const resultString = '![video](https://example.com/video.mp4)';
@@ -892,6 +936,17 @@ describe('Video tag conversion to markdown', () => {
         expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString);
         expect(cacheVideoAttributes).toHaveBeenCalledWith("https://example.com/video.mp4", ' data-expensify-width="100" data-expensify-height="500" data-expensify-thumbnail-url="https://image.com/img.jpg"')
     })
+
+    test('Video with extra attributes to be cached with mediaAttributeCachingFn', () => {
+        const mediaAttributeCachingFn = jest.fn();
+        const testString = '<video data-expensify-source="https://example.com/video.mp4" data-expensify-width="100" data-expensify-height="500" data-expensify-thumbnail-url="https://image.com/img.jpg">video</video>';
+        const resultString = '![video](https://example.com/video.mp4)';
+        const extras = {
+            mediaAttributeCachingFn,
+        };
+        expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString);
+        expect(mediaAttributeCachingFn).toHaveBeenCalledWith("https://example.com/video.mp4", ' data-expensify-width="100" data-expensify-height="500" data-expensify-thumbnail-url="https://image.com/img.jpg"')
+    })
 })
 
 describe('Tag names starting with common charaters', () => {

diff --git a/__tests__/ExpensiMark-test.js b/__tests__/ExpensiMark-test.js
@@ -1,6 +1,7 @@
 /* eslint-disable max-len */
 import ExpensiMark from '../lib/ExpensiMark';
 import * as Utils from '../lib/utils';
+import {any, string} from "prop-types";
 
 const parser = new ExpensiMark();
 
@@ -48,3 +49,55 @@ test('Test extract link from Markdown link syntax', () => {
     const links = ['https://new.expensify.com/'];
     expect(parser.extractLinksInMarkdownComment(comment)).toStrictEqual(links);
 });
+
+describe('Test ExpensiMark getAttributeCache', () => {
+    const expensiMark = new ExpensiMark();
+
+    describe('For attrCachingFn', () => {
+        test('If mediaAttributeCachingFn is provided, returns it', () => {
+            const extras = {
+                mediaAttributeCachingFn: jest.fn(),
+            }
+            expect(expensiMark.getAttributeCache(extras).attrCachingFn).toBe(extras.mediaAttributeCachingFn);
+        })
+
+        test('If mediaAttributeCachingFn is not provided, returns cacheVideoAttributes', () => {
+            const extras = {
+                cacheVideoAttributes: jest.fn(),
+            }
+            expect(expensiMark.getAttributeCache(extras).attrCachingFn).toBe(extras.cacheVideoAttributes);
+        })
+
+        test('If mediaAttributeCachingFn and cacheVideoAttributes are not provided, returns undefined', () => {
+            const extras = {}
+            expect(expensiMark.getAttributeCache(extras).attrCachingFn).toBe(undefined);
+        })
+    });
+
+    describe('For attrCache', () => {
+        test('If mediaAttributeCache is provided, returns it', () => {
+            const extras = {
+                mediaAttributeCache: jest.fn(),
+            }
+            expect(expensiMark.getAttributeCache(extras).attrCache).toBe(extras.mediaAttributeCache);
+        })
+
+        test('If mediaAttributeCache is not provided, returns videoAttributeCache', () => {
+            const extras = {
+                videoAttributeCache: jest.fn(),
+            }
+            expect(expensiMark.getAttributeCache(extras).attrCache).toBe(extras.videoAttributeCache);
+        })
+
+        test('If mediaAttributeCache and videoAttributeCache are not provided, returns undefined', () => {
+            const extras = {}
+            expect(expensiMark.getAttributeCache(extras).attrCache).toBe(undefined);
+        })
+    });
+
+    test('If no extras are undefined, returns undefined for both attrCachingFn and attrCache', () => {
+        const {attrCachingFn, attrCache} = expensiMark.getAttributeCache(undefined);
+        expect(attrCachingFn).toBe(undefined);
+        expect(attrCache).toBe(undefined);
+    })
+});
diff --git a/lib/ExpensiMark.ts b/lib/ExpensiMark.ts
@@ -9,9 +9,31 @@ import * as Utils from './utils';
 type Extras = {
     reportIDToName?: Record<string, string>;
     accountIDToName?: Record<string, string>;
+
+    /**
+     * @deprecated Replaced with mediaAttributeCachingFn
+     */
     cacheVideoAttributes?: (vidSource: string, attrs: string) => void;
+
+    /**
+     * @deprecated Replaced with mediaAttributeCache
+     */
     videoAttributeCache?: Record<string, string>;
+
+    /**
+     * Function used to cache HTML tag attributes during conversion to and from Markdown
+     * @param mediaSource URI to media source
+     * @param attrs Additional attributes to be cached
+     */
+    mediaAttributeCachingFn?: (mediaSource: string, attrs: string) => void;
+
+    /**
+     * Key/value cache for HTML tag attributes, where the key is media source URI, value is cached attributes
+     */
+    mediaAttributeCache?: Record<string, string>;
 };
+export type {Extras};
+
 const EXTRAS_DEFAULT = {};
 
 type ReplacementFn = (extras: Extras, ...matches: string[]) => string;
@@ -62,6 +84,17 @@ const MARKDOWN_VIDEO_REGEX = new RegExp(
 const SLACK_SPAN_NEW_LINE_TAG = '<span class="c-mrkdwn__br" data-stringify-type="paragraph-break" style="box-sizing: inherit; display: block; height: unset;"></span>';
 
 export default class ExpensiMark {
+    getAttributeCache = (extras?: Extras) => {
+        if (!extras) {
+            return {attrCachingFn: undefined, attrCache: undefined};
+        }
+
+        return {
+            attrCachingFn: extras.mediaAttributeCachingFn ?? extras.cacheVideoAttributes,
+            attrCache: extras.mediaAttributeCache ?? extras.videoAttributeCache,
+        };
+    };
+
     static Log = new Logger({
         serverLoggingCallback: () => undefined,
         // eslint-disable-next-line no-console
@@ -171,11 +204,13 @@ export default class ExpensiMark {
                  * @return Returns the HTML video tag
                  */
                 replacement: (extras, _match, videoName, videoSource) => {
-                    const extraAttrs = extras && extras.videoAttributeCache && extras.videoAttributeCache[videoSource];
+                    const attrCache = this.getAttributeCache(extras).attrCache;
+                    const extraAttrs = attrCache && attrCache[videoSource];
                     return `<video data-expensify-source="${Str.sanitizeURL(videoSource)}" ${extraAttrs || ''}>${videoName ? `${videoName}` : ''}</video>`;
                 },
                 rawInputReplacement: (extras, _match, videoName, videoSource) => {
-                    const extraAttrs = extras && extras.videoAttributeCache && extras.videoAttributeCache[videoSource];
+                    const attrCache = this.getAttributeCache(extras).attrCache;
+                    const extraAttrs = attrCache && attrCache[videoSource];
                     return `<video data-expensify-source="${Str.sanitizeURL(videoSource)}" data-raw-href="${videoSource}" data-link-variant="${typeof videoName === 'string' ? 'labeled' : 'auto'}" ${extraAttrs || ''}>${videoName ? `${videoName}` : ''}</video>`;
                 },
             },
@@ -245,13 +280,21 @@ export default class ExpensiMark {
              * tag.
              * Additional sanitization is done to the alt attribute to prevent parsing it further to html by later
              * rules.
+             * Additional tags from cache are applied to the result HTML.
              */
             {
                 name: 'image',
                 regex: MARKDOWN_IMAGE_REGEX,
-                replacement: (_extras, _match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}"${g1 ? ` alt="${this.escapeAttributeContent(g1)}"` : ''} />`,
-                rawInputReplacement: (_extras, _match, g1, g2) =>
-                    `<img src="${Str.sanitizeURL(g2)}"${g1 ? ` alt="${this.escapeAttributeContent(g1)}"` : ''} data-raw-href="${g2}" data-link-variant="${typeof g1 === 'string' ? 'labeled' : 'auto'}" />`,
+                replacement: (extras, _match, imgAlt, imgSource) => {
+                    const attrCache = this.getAttributeCache(extras).attrCache;
+                    const extraAttrs = attrCache && attrCache[imgSource];
+                    return `<img src="${Str.sanitizeURL(imgSource)}"${imgAlt ? ` alt="${this.escapeAttributeContent(imgAlt)}"` : ''} ${extraAttrs || ''}/>`;
+                },
+                rawInputReplacement: (extras, _match, imgAlt, imgSource) => {
+                    const attrCache = this.getAttributeCache(extras).attrCache;
+                    const extraAttrs = attrCache && attrCache[imgSource];
+                    return `<img src="${Str.sanitizeURL(imgSource)}"${imgAlt ? ` alt="${this.escapeAttributeContent(imgAlt)}"` : ''} data-raw-href="${imgSource}" data-link-variant="${typeof imgAlt === 'string' ? 'labeled' : 'auto'}" ${extraAttrs || ''}/>`;
+                },
             },
 
             /**
@@ -658,13 +701,38 @@ export default class ExpensiMark {
 
             {
                 name: 'image',
-                regex: /<img[^><]*src\s*=\s*(['"])(.*?)\1(?:[^><]*alt\s*=\s*(['"])(.*?)\3)?[^><]*>*(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi,
-                replacement: (_extras, _match, _g1, g2, _g3, g4) => {
-                    if (g4) {
-                        return `![${g4}](${g2})`;
+                regex: /<img[^><]*src\s*=\s*(['"])(.*?)\1(.*?)>(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi,
+                /**
+                 * @param extras - The extras object
+                 * @param _match - The full match
+                 * @param _g1 - The first capture group (the quote)
+                 * @param imgSource - The second capture group - src attribute value
+                 * @param imgAttrs - The third capture group - any attributes after src
+                 * @returns The markdown image tag
+                 */
+                replacement: (extras, _match, _g1, imgSource, imgAttrs) => {
+                    // Extract alt attribute from imgAttrs
+                    let altText = '';
+                    const altRegex = /alt\s*=\s*(['"])(.*?)\1/i;
+                    const altMatch = imgAttrs.match(altRegex);
+                    const attrCachingFn = this.getAttributeCache(extras).attrCachingFn;
+                    let attributes = imgAttrs;
+                    if (altMatch) {
+                        altText = altMatch[2];
+                        // Remove the alt attribute from imgAttrs
+                        attributes = attributes.replace(altRegex, '');
                     }
-
-                    return `!(${g2})`;
+                    // Remove trailing slash and extra whitespace
+                    attributes = attributes.replace(/\s*\/\s*$/, '').trim();
+                    // Cache attributes without alt and trailing slash
+                    if (imgAttrs && attrCachingFn && typeof attrCachingFn === 'function') {
+                        attrCachingFn(imgSource, attributes);
+                    }
+                    // Return the markdown image tag
+                    if (altText) {
+                        return `![${altText}](${imgSource})`;
+                    }
+                    return `!(${imgSource})`;
                 },
             },
 
@@ -681,8 +749,10 @@ export default class ExpensiMark {
                  * @returns The markdown video tag
                  */
                 replacement: (extras, _match, _g1, videoSource, videoAttrs, videoName) => {
-                    if (videoAttrs && extras && extras.cacheVideoAttributes && typeof extras.cacheVideoAttributes === 'function') {
-                        extras.cacheVideoAttributes(videoSource, videoAttrs);
+                    const attrCachingFn = this.getAttributeCache(extras).attrCachingFn;
+
+                    if (videoAttrs && attrCachingFn && typeof attrCachingFn === 'function') {
+                        attrCachingFn(videoSource, videoAttrs);
                     }
                     if (videoName) {
                         return `![${videoName}](${videoSource})`;