@lexical/markdown Fix strikethrough TextFormatTransformer order

[hanford]

This seems to fix it on my end!

closes #2303

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Jun 1, 2022 at 6:09PM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Jun 1, 2022 at 6:09PM (UTC)

[fantactuka]

Unfortunately it's not about order of transformers, but more about order of opening tags, e.g.

**bold ~~strikethrough~~**
~~strikethrough **bold**~~

Looks like this part will need to carry a stack of open tags in order to close them in a proper order: When adding open tag put into array, and when adding close tag(s) then append them to string in reverse order they are in the array + remove from array

[hanford]

I wasn't able to do this in one iteration given the for of iterates over each transform one at a time, given the small number of TextTransforms that doesn't seem to be an issue?

[hanford]

may also be nice to leverage the applied Set but seems like iteration order isn't guaranteed, so we could also make the applied an array and then do a lookup on line 140 but not certain what preferences are

[hanford]

I don't mind any JS style changes, will make whatever is recommended, I just want this change to make 0.3 😅

[fantactuka]

Was testing this thing locally, and it'll need more changes on how open/close tags are adjusted. E.g. imagine two cases:

{bold strikethrough} {bold}
{bold strikethrough} {strikethrough}
->
**~~bold strikethrough~~ bold**
~~**bold strikethrough** bold~~

It appears we can only add open tags when we know in what order they'll be closed, same applies to closing tag - need to know in which order previous exported text nodes had it opened. I'll try to check it too, but if want to give it another try - definitely do!

[hanford]

Spent most of my day looking at this and made some progress, but not with a solution I'm happy with:

function getTransformOrder(node, textTransformers, reverse) {
  let appliedOrder = [];
  const applied = new Set();
  for (const transformer of textTransformers) {
    const format = transformer.format[0];

    if (hasFormat(node, format) && !applied.has(format)) {
      // Multiple tags might be used for the same format (*, _)
      applied.add(format); // Prevent adding opening tag is already opened by the previous sibling
      // Add most recent textTransform to beginning of appliedOrder where
      // we'll later iterate last-in-first-out when appending closing tags
      appliedOrder = [transformer, ...appliedOrder];
    }
  }

  return appliedOrder
}

function exportTextFormat(node, textContent, textTransformers) {
  let frozenString = textContent.trim()
  let output = frozenString
  
  // get siblings
  const previousSiblingNode = getTextSibling(node, true);
  const nextSiblingNode = getTextSibling(node, false);


  // get previous node formats
  let prevNode = getTransformOrder(previousSiblingNode, textTransformers)
  // get next node formats
  let nextNode = getTransformOrder(nextSiblingNode, textTransformers)

  // get current node formats
  let appliedOrder = getTransformOrder(node, textTransformers)

  // filter out common formats between current and previous nodes
  const prevFormats = appliedOrder.filter(x => !prevNode.includes(x))
  // filter out common formats between current and next nodes
  const nextFormats = appliedOrder.filter(x => !nextNode.includes(x))

 // reorder current node transforms contextually with which formats are surrounding us
  const reordered = [
    ...new Set([
      ...prevFormats,
      ...appliedOrder,
      ...nextFormats,
    ])
  ]

  reordered.forEach((transformer) => {
    const format = transformer.format[0];
    const tag = transformer.tag;
    if (!hasFormat(previousSiblingNode, format)) {
      output = tag + output
    } 

    // Prevent adding closing tag if next sibling will do it
    if (!hasFormat(nextSiblingNode, format)) {
      output += tag;
    }
  });

  return textContent.replace(frozenString, output);
} 

The best solution I could come up with is just being more naive and removing the next/previous sibling code, it's a nice optimization to have, but I'm able to break it in a number of different ways in the playground (especially when over selecting and getting whitespace involved)

i.e. why not just wrap whatever lexical gives us as a leaf node with the appropriate tags rather than having to be smart about it what's around and traversing nearby nodes?

Would you be opposed to me removing the getTextSibling function and simplifying the tag additions?

[fantactuka]

Thanks for putting so much effort into this one! One problem with removing check for prev/next nodes will cause tags duplication:

<b>bold text <s>strikethrough</s></b>
->
**bold text ****~~strikethrough~~**
instead of
**bold text ~~strikethrough~~**

I'm thinking whether import/export would need to have double pass through:
Where in first pass we process nodes tree into objects like

{
  elementTag: '# ',
  children: [
    { text: 'hello', tags: ['*', '~~'] },
    { text: 'world', tags: ['*'] },
  ]
}

and then second one will generate final string using stack for open/close tags. Maybe it's still possible to do it in a single run tho. I'll give it a try over the weekend, but appreciate any ideas or suggestions!

[JGonzalvoZipDev]

This is great!

__
 Summary: 🏆 This code is awesome  |  Tags: Readable, Efficient, Reusable

[acywatson]

@fantactuka @hanford can we get this merged or closed?

[digitalgopnik]

@hanford What do we need to finish merging this fix? 🙂

[yunhanw5]

Hi @hanford @fantactuka @acywatson , Is there an alternate solution that made this closed? If not, will this be merged soon to fix the wrong format order? We are facing similar issues when applying underline and bold; the wrong markdown is **aa++bb**++ where it should be **aa++bb++**. Thanks in advance for any related fixes!