improve legacy shortcode handling (#1349)

* add support for closing shortcodes

* allow Shortcode.regex name param to be a regex

* use Shortcode.regex for legacy shortcode matching

* allow dashes in shortcode param names

static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts

@@ -5,7 +5,7 @@ import LegacyShortcodes from "./LegacyShortcodes"
 import { LEGACY_SHORTCODES } from "./constants"
 import Paragraph from "@ckeditor/ckeditor5-paragraph/src/paragraph"
 
-const quizTestMD = `{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}sound, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="true" >}}sound, valid{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, true{{< /quiz_choice >}}{{< /quiz_choices >}}{{< quiz_solution / >}}{{< /quiz_multiple_choice >}}`
+const quizTestMD = `{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}sound, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="true" >}}sound, valid{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, true{{< /quiz_choice >}}{{< /quiz_choices >}}{{< quiz_solution >}}{{< /quiz_multiple_choice >}}`
 
 const getEditor = createTestEditor([Paragraph, LegacyShortcodes, Markdown])
 
@@ -31,20 +31,42 @@ describe("ResourceEmbed plugin", () => {
   test.each(LEGACY_SHORTCODES)(
     "should take in and return %p closing shortcode",
     async shortcode => {
-      const md = `{{< /${shortcode} >}}`
+      const md1 = `{{< /${shortcode} >}}`
+      const editor1 = await getEditor(md1)
+      // @ts-ignore
+      expect(editor1.getData()).toBe(md1)
+
+      /**
+       * Hugo documentation uses the first version so it should be preferred.
+       * But Hugo accepts this version and image-gallery uses it.
+       * The editor will normalize it to the first version.
+       */
+      const md2 = `{{</ ${shortcode} >}}`
+      const editor2 = await getEditor(md2)
+      // @ts-ignore
+      expect(editor2.getData()).toBe(md1)
+    }
+  )
+
+  test.each(LEGACY_SHORTCODES)(
+    "should take in and return %p shortcode with positional parameters",
+    async shortcode => {
+      const md = `{{< ${shortcode} arguments "and chemistry H{{< sub 2 >}}0" >}}`
+      const expected = `{{< ${shortcode} "arguments" "and chemistry H{{< sub 2 >}}0" >}}`
       const editor = await getEditor(md)
       // @ts-ignore
-      expect(editor.getData()).toBe(md)
+      expect(editor.getData()).toBe(expected)
     }
   )
 
   test.each(LEGACY_SHORTCODES)(
-    "should take in and return %p shortcode with arguments",
+    "should take in and return %p shortcode with named parameters",
     async shortcode => {
-      const md = `{{< ${shortcode} arguments foo=123 html=<for some reason/> >}}`
+      const md = `{{< ${shortcode} foo=123 html="<for some reason/>" >}}`
+      const expected = `{{< ${shortcode} foo="123" html="<for some reason/>" >}}`
       const editor = await getEditor(md)
       // @ts-ignore
-      expect(editor.getData()).toBe(md)
+      expect(editor.getData()).toBe(expected)
     }
   )
 

static/js/lib/ckeditor/plugins/LegacyShortcodes.ts

@@ -5,6 +5,7 @@ import { editor } from "@ckeditor/ckeditor5-core"
 import MarkdownSyntaxPlugin from "./MarkdownSyntaxPlugin"
 import { TurndownRule } from "../../../types/ckeditor_markdown"
 import { LEGACY_SHORTCODES } from "./constants"
+import { Shortcode } from "./util"
 
 const shortcodeClass = (shortcode: string) => `legacy-shortcode-${shortcode}`
 
@@ -18,33 +19,35 @@ class LegacyShortcodeSyntax extends MarkdownSyntaxPlugin {
 
   get showdownExtension() {
     return function legacyShortcodeExtension(): ShowdownExtension[] {
-      return LEGACY_SHORTCODES.map(
-        (shortcode: string): ShowdownExtension => {
-          const shortcodeRegex = new RegExp(`{{< /?${shortcode} (.*?)>}}`, "g")
-          const closingShortcodeRegex = new RegExp(
-            `{{< /${shortcode} (.*?)>}}`,
-            "g"
-          )
-
-          return {
-            type:    "lang",
-            regex:   shortcodeRegex,
-            replace: (stringMatch: string, shortcodeArgs: string | null) => {
-              const isClosing = JSON.stringify(
-                closingShortcodeRegex.test(stringMatch)
-              )
-
-              const tag = `<span ${DATA_ISCLOSING}="${isClosing}" ${
-                shortcodeArgs ?
-                  `${DATA_ARGUMENTS}="${encodeURIComponent(shortcodeArgs)}"` :
-                  ""
-              } class="${shortcodeClass(shortcode)}"></span>`
-
-              return tag
-            }
+      const nameRegex = new RegExp(LEGACY_SHORTCODES.join("|"))
+      return [
+        {
+          type:    "lang",
+          /**
+           * It's important that there's a single regex for all the legacy
+           * shortcodes, rather than one per shortcode. Otherwise the order of
+           * the replacements is important.
+           *
+           * For example, image-gallery-item and sub are both legacy shortcodes
+           * and sometimes they are used together:
+           *  {{< image-gallery-item ... "H{{< sub 2 >}}O" >}}
+           * If separate regexes are used, then image-gallery-item would need to
+           * come before sub so that the sub-replacement is not used on the above
+           * example.
+           */
+          regex:   Shortcode.regex(nameRegex, false),
+          replace: (stringMatch: string) => {
+            const shortcode = Shortcode.fromString(stringMatch)
+            const { isClosing } = shortcode
+            const params = shortcode.params.map(p => p.toHugo()).join(" ")
+            const tag = `<span ${DATA_ISCLOSING}="${isClosing}" ${
+              params ? `${DATA_ARGUMENTS}="${encodeURIComponent(params)}"` : ""
+            } class="${shortcodeClass(shortcode.name)}"></span>`
+
+            return tag
           }
         }
-      )
+      ]
     }
   }
 
@@ -64,7 +67,7 @@ class LegacyShortcodeSyntax extends MarkdownSyntaxPlugin {
 
           return `{{< ${isClosingTag ? "/" : ""}${shortcode} ${
             rawShortcodeArgs !== undefined && rawShortcodeArgs !== null ?
-              decodeURIComponent(rawShortcodeArgs) :
+              `${decodeURIComponent(rawShortcodeArgs)} ` :
               ""
           }>}}`
         }

static/js/lib/ckeditor/plugins/util.test.ts

@@ -46,15 +46,15 @@ describe("unescapeStringQuotedWith", () => {
 })
 
 describe("Shortcode", () => {
-  describe("Shortcode.parse", () => {
+  describe("Shortcode.fromString", () => {
     it("parses shortcodes with named params", () => {
       const text =
-        '{{< some_shortcode cool_arg="cats and dogs" href_uuid=uuid456 >}}'
+        '{{< some_shortcode param-with-dash="cats and dogs" with_underscore=uuid456 >}}'
       const result = Shortcode.fromString(text)
 
       const params = [
-        { name: "cool_arg", value: "cats and dogs" },
-        { name: "href_uuid", value: "uuid456" }
+        { name: "param-with-dash", value: "cats and dogs" },
+        { name: "with_underscore", value: "uuid456" }
       ].map(makeParams)
       expect(result).toStrictEqual(
         new Shortcode("some_shortcode", params, false)
@@ -165,6 +165,28 @@ describe("Shortcode", () => {
     )
   })
 
+  it.each([
+    {
+      text:     "{{< /some_shortcode >}}",
+      expected: new Shortcode("some_shortcode", [], false, true)
+    },
+    {
+      text:     "{{< / some_shortcode >}}",
+      expected: new Shortcode("some_shortcode", [], false, true)
+    },
+    {
+      text:     "{{</ some_shortcode >}}",
+      expected: new Shortcode("some_shortcode", [], false, true)
+    },
+    {
+      text:     "{{% /some_shortcode %}}",
+      expected: new Shortcode("some_shortcode", [], true, true)
+    }
+  ])("parses closing shortcodes", ({ text, expected }) => {
+    const result = Shortcode.fromString(text)
+    expect(result).toStrictEqual(expected)
+  })
+
   it("does not allow mixing named and positional params", () => {
     const text = '{{< resource uuid123 href_uuid="uuid456" >}}'
     expect(() => Shortcode.fromString(text)).toThrow(
@@ -367,21 +389,37 @@ describe("Shortcode", () => {
       ])
     })
 
+    it("captures closing and opening shortcodes", () => {
+      const regex = Shortcode.regex("some_shortcode", false)
+      const text = `
+      a {{< some_shortcode xyz >}} b
+      c {{< /some_shortcode >}} d
+      e {{</ some_shortcode >}} f
+      `
+      const match = text.match(regex)
+      expect(match).toStrictEqual([
+        "{{< some_shortcode xyz >}}",
+        "{{< /some_shortcode >}}",
+        "{{</ some_shortcode >}}"
+      ])
+    })
+
     it.each([
       {
-        name:     "some_shortcode",
-        text:     "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}}",
-        expected: "{{< some_shortcode xyz >}}"
+        name: "some_shortcode",
+        text:
+          "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}} {{< /some_shortcode xyz >}}",
+        expected: ["{{< some_shortcode xyz >}}", "{{< /some_shortcode xyz >}}"]
       },
       {
         name:     "some_shortcode_two",
         text:     "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}}",
-        expected: "{{< some_shortcode_two 123 >}}"
+        expected: ["{{< some_shortcode_two 123 >}}"]
       }
     ])("captures shortcodes of specified name", ({ text, expected, name }) => {
       const regex = Shortcode.regex(name, false)
       const match = text.match(regex)
-      expect(match).toStrictEqual([expected])
+      expect(match).toStrictEqual(expected)
     })
 
     it("tolerates an odd number of escaped quotes", () => {
@@ -390,5 +428,12 @@ describe("Shortcode", () => {
       const match = text.match(regex)
       expect(match).toStrictEqual(['{{< shortcode "cat \\" >}}" >}}'])
     })
+
+    it("can take a regex as name parameter", () => {
+      const regex = Shortcode.regex(/cat|dog/, false)
+      const text = "a {{< cat meow >}} {{< dog woof >}} {{< dragon roar >}}  b"
+      const match = text.match(regex)
+      expect(match).toStrictEqual(["{{< cat meow >}}", "{{< dog woof >}}"])
+    })
   })
 })

static/js/lib/ckeditor/plugins/util.ts

@@ -102,14 +102,18 @@ export class Shortcode {
 
   isPercentDelimited: boolean
 
+  isClosing: boolean
+
   constructor(
     name: string,
     params: ShortcodeParam[],
-    isPercentDelimited = false
+    isPercentDelimited = false,
+    isClosing = false
   ) {
     this.name = name
     this.params = params
     this.isPercentDelimited = isPercentDelimited
+    this.isClosing = isClosing
 
     const hasPositionalParams = params.some(p => p.name === undefined)
     const hasNamedParams = params.some(p => p.name !== undefined)
@@ -127,19 +131,22 @@ export class Shortcode {
    */
   toHugo() {
     const stringifiedArgs = this.params.map(p => p.toHugo())
-    const interior = [this.name, ...stringifiedArgs].join(" ")
+    const name = this.isClosing ? `/${this.name}` : this.name
+    const interior = [name, ...stringifiedArgs].join(" ")
     if (this.isPercentDelimited) {
       return `{{% ${interior} %}}`
     }
     return `{{< ${interior} >}}`
   }
 
+  private static IS_CLOSING_REGEXP = /\s*(?<isClosing>\/)?\s*/
+
   /**
    * Regexp used for matching individual shortcode arguments.
    */
   private static ARG_REGEXP = new RegExp(
     [
-      /((?<name>[a-zA-Z_]+)=)?/.source,
+      /((?<name>[a-zA-Z\-_]+)=)?/.source,
       "(",
       /("(?<qvalue>.*?)(?<!\\)")/.source,
       "|",
@@ -156,9 +163,15 @@ export class Shortcode {
   static fromString(s: string): Shortcode {
     Shortcode.heuristicValidation(s)
     const isPercentDelmited = s.startsWith("{{%") && s.endsWith("%}}")
-    const [nameMatch, ...argMatches] = s
-      .slice(3, -3)
-      .matchAll(Shortcode.ARG_REGEXP)
+    const interior = s.slice(3, -3)
+    const isClosingMatch = interior.match(Shortcode.IS_CLOSING_REGEXP)
+    // IS_CLOSING_REGEXP will always match, hence the non-null assertion !
+    const isClosing = isClosingMatch!.groups!.isClosing === "/"
+    const nameAndArgs = interior.slice(isClosingMatch![0].length)
+
+    const [nameMatch, ...argMatches] = nameAndArgs.matchAll(
+      Shortcode.ARG_REGEXP
+    )
     const name = Shortcode.getArgMatchValue(nameMatch)
     const params = argMatches.map(match => {
       return new ShortcodeParam(
@@ -167,7 +180,7 @@ export class Shortcode {
       )
     })
 
-    return new Shortcode(name, params, isPercentDelmited)
+    return new Shortcode(name, params, isPercentDelmited, isClosing)
   }
 
   /**
@@ -231,13 +244,15 @@ export class Shortcode {
    * Returns a global regex that matches shortcodes of given name. Useful for
    * use in Showdown rules.
    */
-  static regex(name: string, isPercentDelimited: boolean) {
+  static regex(name: string | RegExp, isPercentDelimited: boolean) {
     const opener = isPercentDelimited ? "{{%" : "{{<"
     const closer = isPercentDelimited ? "%}}" : ">}}"
     const regex = new RegExp(
       [
         opener,
-        String.raw`\s${name}\s`,
+        Shortcode.IS_CLOSING_REGEXP.source,
+        name instanceof RegExp ? `(${name.source})` : name,
+        String.raw`\s`,
         /**
          * Non-greedily capture anything up until the closer. Except if there is
          * a non-escaped quotation mark, then there must be another.