🔧 ExpensiMark: Markdown image shorthand syntax #676

kidroca · 2024-04-02T18:30:34Z

This PR introduces refinements in the ExpensiMark library to enhance the handling of markdown syntax for images, including the introduction of shorthand syntax for images without alt text, and refines the cleanup of HTML attributes

Fixed Issues

$ Expensify/App#38952

Tests

Unit tests have been updated in __tests__/ExpensiMark-HTML-test.js and __tests__/ExpensiMark-Markdown-test.js to cover the new markdown shorthand for images without alt text, the handling of images with empty and special characters in alt text, and the refinement in HTML attribute cleanup. These tests validate the parsing of markdown to HTML and vice versa, ensuring that the changes behave as expected.
Manual tests were conducted to verify that the new markdown shorthand for images is correctly converted to HTML, that images with and without alt text are handled properly, and that no XSS vulnerabilities are introduced through the alt text or any other HTML attributes. The tests also confirmed that existing functionality for other markdown elements remains unaffected.

QA

QA needs to validate that images using the new shorthand markdown syntax are correctly rendered in the app. This includes testing with various image URLs, with and without alt text, and ensuring that images embedded within text blocks are displayed correctly.
It's crucial to test for regressions in the rendering of markdown content, particularly other markdown elements like links, bold/italic text, and code blocks. Ensuring that the sanitization of HTML attributes does not inadvertently affect the rendering of other elements or introduce any rendering issues is also important.

kidroca · 2024-04-02T18:32:51Z

lib/ExpensiMark.js

-
-    /**
-     * Replace MD characters with their HTML entity equivalent
-     * @param {String} text
-     * @return {String}
-     */
-    escapeMarkdownEntities(text) {
-        // A regex pattern matching special MD characters we'd like to escape
-        const pattern = /([*_{}[\]~])/g;
-
-        // A map of MD characters to their HTML entity equivalent
-        const entities = {
-            '*': '&ast;',
-            _: '&lowbar;',
-            '{': '&lbrace;',
-            '}': '&rbrace;',
-            '[': '&lbrack;',
-            ']': '&rbrack;',
-            '~': '&#126;',
-        };
-
-        return text.replace(pattern, char => entities[char] || char);
-    }


With the introduction of attributeCleanup rule - this logic is no longer needed

kidroca · 2024-04-02T18:34:57Z

lib/ExpensiMark.js

-const MARKDOWN_LINK = `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`;
-const MARKDOWN_LINK_REGEX = new RegExp(MARKDOWN_LINK, 'gi');
-const MARKDOWN_IMAGE_REGEX = new RegExp(`\\!${MARKDOWN_LINK}`, 'gi');
+const MARKDOWN_LINK_REGEX = new RegExp(`\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`, 'gi');
+const MARKDOWN_IMAGE_REGEX = new RegExp(`\\!(?:\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)])?\\(${MARKDOWN_URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`, 'gi');


MARKDOWN_LINK's string was directly moved into the MARDOWN_LINK_REGEX constructor as it's no longer reusable

MARKDOWN_IMAGE_REGEX was updated and although is similar to MARKDOWN_LINK the [alt text] part was made optional

chiragsalian

Code LGTM, i just have 2 questions for you. If nothing needs to be modified I'll merge once answered.

lib/ExpensiMark.js

__tests__/ExpensiMark-HTML-test.js

robertKozik · 2024-04-04T15:28:32Z

Hi! @kidroca
Can you check the case when label is a code block and shouldKeepRawInput is set to true? Seems wrong to me

    test.only('No html inside the alt attribute - pre tag', () => {
        const testString = '![```code```](https://example.com/image.png)';
        const resultString = '<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);
    });

Support the `!(url)` to create images without alt text 🧪 Add test for image md shorthand conversion to html tag without alt text ℹ️ Added tests for single image with alt text, single image with empty alt text, and single image short syntax without alt text in ExpensiMark-HTML-test.js.

…markdown

…matching The update aims to avoid matching and converting these rules if the match happens to be within an HTML attribute - in that case the content should not be converted to HTML

kidroca · 2024-04-05T14:13:43Z

@robertKozik, @chiragsalian, I have updated the pull request with a revised approach to handling HTML attributes. The original method aimed to clean up HTML within attributes but proved unreliable because distinguishing embedded HTML from the attribute content was challenging. Consider the example:

<img src="https://example.com/image.png" alt="<pre data-code-raw="code">code</pre>" data-raw-href="https://example.com/image.png" data-link-variant="labeled" /> />

In this case, the alt attribute includes additional quotes that complicate detecting the attribute's end due to the embedded HTML. To address this, I've modified the Regex patterns for the italic, bold, and strike rules to prevent matches within HTML attributes. Additionally, I have added tests to cover these scenarios (thanks to @robertKozik for the suggestion), and all existing tests for these rules continue to pass.

As the scope and focus of the PR have shifted, I kindly request a re-review. I have made an effort to limit the extent of changes for easier review.

kidroca · 2024-04-05T14:23:00Z

@robertKozik, regarding:

include #676 PR as patch package

While using a patch package might seem convenient, I've updated the PR, which necessitates creating the patch again.

Initially, my suggestion to use the commit hash from my fork was intended as a temporary measure for testing purposes in your PR. This approach would enable us to validate the integration, merge the changes into expensify-common, and then revert to the original package source.
(Here's the hash if you've reconsidered: "expensify-common": "git+ssh://git@github.com/kidroca/expensify-common.git#7a7c075bff9a07eb280fc720bec65b6d2acab161")

Alternatively, if it’s more feasible, I can open a new PR in NewDot using a previous version of live-markdown. Let me know if you prefer this method, and I can set it up later.

robertKozik · 2024-04-05T17:48:06Z

lib/ExpensiMark.js

-                replacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}" alt="${this.escapeMarkdownEntities(g1)}" />`,
-                rawInputReplacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}" alt="${this.escapeMarkdownEntities(g1)}" data-raw-href="${g2}" data-link-variant="labeled" />`
+                replacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}"${g1 ? ` alt="${this.escapeAttributeContent(g1)}"` : ''} />`,
+                rawInputReplacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}"${g1 ? ` alt="${this.escapeAttributeContent(g1)}"` : ''} data-raw-href="${g2}" data-link-variant="labeled" />`


One minor thing, sorry for bother @kidroca .
If user types !(link) data-link-variant should be set to "auto" rather than "labeled" indicating that it's typed using shorthand syntax

This is needed to distinguish between ![](link) and !(link)

robertKozik · 2024-04-05T17:49:14Z

Actually I'm still prefering patching the repo with your changes, rather than pointing to the forked repository.
I'll merge it and open the Expensify PR which will unblock you. We will get rid of the patch simply in the next library version.

I've added one more comment - do you think it's solvable? @kidroca

kidroca · 2024-04-05T17:58:20Z

I've added one more comment - do you think it's solvable? @kidroca

Thanks, I'll post an update in a minute

robertKozik · 2024-04-05T17:58:59Z

I've came up with something like that:
data-link-variant="${typeof(g1) === 'string' ? 'labeled': 'auto'}"

kidroca · 2024-04-05T18:07:38Z

Updated

robertKozik · 2024-04-09T08:58:01Z

@kidroca This PR will unblock you

robertKozik · 2024-04-11T18:41:20Z

@kidroca react-native-live-markdown bump is merged to main. You should be unblocked now ⛓️ 💥

robertKozik · 2024-04-15T06:33:10Z

Pls merge main, bold -> <strong/> seems to go under the radar in HTML attributes

# Conflicts: # __tests__/ExpensiMark-HTML-test.js # __tests__/ExpensiMark-Markdown-test.js # lib/ExpensiMark.js

ℹ️ Skip replacing bold text within code blocks during parsing for ExpensiMark-HTML-test

kidroca · 2024-04-15T17:12:34Z

__tests__/ExpensiMark-HTML-test.js

+test('Test bold within code blocks is skipped', () => {
+    const testString = 'bold\n```*not a bold*```\nThis is *bold*';
+    const replacedString = 'bold<br /><pre>*not&#32;a&#32;bold*</pre>This is <strong>bold</strong>';
+    expect(parser.replace(testString)).toBe(replacedString);
+});


Added this test to ensure the merge conflict was resolved correctly
The test is related to code added in: #681

kidroca · 2024-04-15T17:13:36Z

Merged main there was a conflict related to bold handling, but it's resolved

kidroca requested a review from a team as a code owner April 2, 2024 18:30

melvin-bot bot requested review from chiragsalian and removed request for a team April 2, 2024 18:31

kidroca commented Apr 2, 2024

View reviewed changes

This was referenced Apr 3, 2024

Add support for inline image Markdown - attribute fix #661

Merged

HIGH: [Polish] Add support for image-only (no alt text) inline image Markdown Expensify/App#38952

Closed

chiragsalian previously approved these changes Apr 3, 2024

View reviewed changes

lib/ExpensiMark.js Outdated Show resolved Hide resolved

__tests__/ExpensiMark-HTML-test.js Show resolved Hide resolved

robertKozik added a commit to Expensify/react-native-live-markdown that referenced this pull request Apr 5, 2024

include Expensify/expensify-common#676 PR as patch package

4e8dc93

kidroca dismissed chiragsalian’s stale review via c3ac1a6 April 5, 2024 14:00

kidroca added 5 commits April 5, 2024 17:11

♻️ ExpensiMark: refine html attribute cleanup

67eddf8

🧪 ExpensiMark-Markdown-test.js: add test for alt text with unescaped …

65e6781

…markdown

⚙️ Fix escaping of HTML attribute values in ExpensiMark 🛠️

69023f2

♻️ Refactor ExpensiMark.js regex for italic, bold, and strikethrough …

7a7c075

…matching The update aims to avoid matching and converting these rules if the match happens to be within an HTML attribute - in that case the content should not be converted to HTML

kidroca force-pushed the kidroca/md-image-short-syntax branch from c3ac1a6 to 7a7c075 Compare April 5, 2024 14:12

robertKozik reviewed Apr 5, 2024

View reviewed changes

⚙️ ExpensiMark: Updated image raw data attributes for short image syntax

046b6ec

kidroca force-pushed the kidroca/md-image-short-syntax branch from 22e8a4d to 046b6ec Compare April 5, 2024 18:05

robertKozik approved these changes Apr 11, 2024

View reviewed changes

kidroca mentioned this pull request Apr 12, 2024

Feature: Short-hand image Markdown syntax Expensify/App#39759

Closed

48 tasks

kidroca added 2 commits April 15, 2024 20:10

Merge branch 'refs/heads/main' into kidroca/md-image-short-syntax

3a4eb72

# Conflicts: # __tests__/ExpensiMark-HTML-test.js # __tests__/ExpensiMark-Markdown-test.js # lib/ExpensiMark.js

🧪 Test bold within code blocks is skipped

e01d828

ℹ️ Skip replacing bold text within code blocks during parsing for ExpensiMark-HTML-test

kidroca commented Apr 15, 2024

View reviewed changes

chiragsalian approved these changes Apr 15, 2024

View reviewed changes

chiragsalian merged commit f112a00 into Expensify:main Apr 15, 2024
5 checks passed

tgolen mentioned this pull request May 9, 2024

[HOLD for payment 2024-06-28] [$500] HIGH: [Polish] Editing a comment with a video removes the video Expensify/App#41952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔧 ExpensiMark: Markdown image shorthand syntax #676

🔧 ExpensiMark: Markdown image shorthand syntax #676

kidroca commented Apr 2, 2024

kidroca Apr 2, 2024

kidroca Apr 2, 2024

chiragsalian left a comment •

edited

Loading

robertKozik commented Apr 4, 2024

kidroca commented Apr 5, 2024

kidroca commented Apr 5, 2024

robertKozik Apr 5, 2024

robertKozik commented Apr 5, 2024 •

edited

Loading

kidroca commented Apr 5, 2024

robertKozik commented Apr 5, 2024

kidroca commented Apr 5, 2024

robertKozik commented Apr 9, 2024

robertKozik commented Apr 11, 2024

robertKozik commented Apr 15, 2024 •

edited

Loading

kidroca Apr 15, 2024

kidroca commented Apr 15, 2024

🔧 ExpensiMark: Markdown image shorthand syntax #676

🔧 ExpensiMark: Markdown image shorthand syntax #676

Conversation

kidroca commented Apr 2, 2024

Fixed Issues

Tests

QA

kidroca Apr 2, 2024

Choose a reason for hiding this comment

kidroca Apr 2, 2024

Choose a reason for hiding this comment

chiragsalian left a comment • edited Loading

Choose a reason for hiding this comment

robertKozik commented Apr 4, 2024

kidroca commented Apr 5, 2024

kidroca commented Apr 5, 2024

robertKozik Apr 5, 2024

Choose a reason for hiding this comment

robertKozik commented Apr 5, 2024 • edited Loading

kidroca commented Apr 5, 2024

robertKozik commented Apr 5, 2024

kidroca commented Apr 5, 2024

robertKozik commented Apr 9, 2024

robertKozik commented Apr 11, 2024

robertKozik commented Apr 15, 2024 • edited Loading

kidroca Apr 15, 2024

Choose a reason for hiding this comment

kidroca commented Apr 15, 2024

chiragsalian left a comment •

edited

Loading

robertKozik commented Apr 5, 2024 •

edited

Loading

robertKozik commented Apr 15, 2024 •

edited

Loading