Updated Selector escapeName to accommodate special characters used in tailwind class names

[bernesto]

Tailwind class names use the special characters [, ], (, ), ., %, @, :, /, and +. Several of these were missing from the regex that escapes class names, thereby breaking Taliwind support.

[bernesto]

Added checks for comment processing that cause errors outlined in #5720

[artf]

Thanks @bernesto I left some comments.

[artf]

Not sure what you were trying to fix here (the current logic should handle already all white spaces) but your change removes all the text nodes.

Before

After

Also, no changes to critical parts (like the Parser) without appropriate tests, please.

ps: it's good to know tests are failing properly in this PR.

[bernesto]

This wasn’t supposed to be part of this pull request. Not fully vetted yet.

(Full disclosure, we use subversion not git. So I’m sure I botched the process. I have two branches. One for commits to the origin, and one for stuff never intended for public consumption lol)

The point of this change is to handle tabs, carriage returns etc. in the input HTML. For instance, imagine text within two spans separated by a carriage return instead of a space between them. Perfectly valid html yielding a space on render, but would fail the current logic and merge the words together.

In our use case, we have a huge corpus of existing HTML generated by users of CKEditor over the years that needs to be processed by GrapesJS. In my testing, I've found GrapesJS works great with GrapesJS generated HTML. But it can have issues with outside generated HTML. Especially dirty code where users have had their hand in things. Most have been edge cases like the above example. Some we are fixing by running a xml parser on the code first. But some are probably just things that have not been encountered before because of the grapes in grapes out process above. We should call the result "wine" or "whine" depending on who you ask. Regardless, we need it to process crap code as properly as it can, and hopefully be kind of backwards compatible.

That’s said. This was not tested nor ready for prime time (if ever). I will be more carful with my PR management.

(edited, original was from my phone)

[bernesto]

Looking further at this change.

Yes, this will trigger a failure in any test where there is a tab, carriage return, or multiple spaces in the test that were not accounted for, as is should. The SVG test is a good example to look at as it has multiple.

It does look like it is causing a few other test failures as well where methods do not exist on test objects. I did not dig into those any further.

That said, I'm not sure why you were seeing the behavior in your screen shots. It is working in the following example, and it does fix the issue in #5724

Before:
https://jsfiddle.net/zwo0mdqf/5/

After:
https://jsfiddle.net/bernesto/pmw1cx2o/

I did revert the change and will submit on a separate branch and pull request since there are 18 tests failing right now, and I don't have time currently to rewrite them.

[artf]

That said, I'm not sure why you were seeing the behavior in your screenshots. It is working in the following example, and it does fix the issue in #5724

From what I see content!.length > 0 && content!.trim() !== '' is skipping textnodes with a text value, eg.

<div>This will be removed <b>Text</b> This will be removed</div>

But I got your point now from the demo, definitely something to fix and freeze with a test.
Weird enough, your updated demo doesn't present this issue, did you change the condition?

I'd also like to point out the reason behind the original condition, it was mainly to clean up "useless" textnodes.
Let's take for example the case with a tab in the middle with this HTML

<div>
  <span class="red">One</span>	<span>Two</span>
</div>

Enabling keepEmptyTextNodes would generate this kind of output inside the div

textnode: "\n  "
text: 'One'
textnode: "\t"
text: 'Two'
textnode: "\n"

so the initial textnode and the last one are quite useless and in a real project would only enlarge the project file without a real benefit. Maybe it would make sense to clean up only those opening/closing white spaces?

[bernesto]

Yes, you were "totally" right on that line. The logic was flawed. I didn't see it a first. "Aaannnddd that is why we have tests..." lol. The demo has a new version with the below logic.

Also, I did understand what your were trying to do there. Less nodes equals less cycles, and less bytes equals better performance. I totally get it. I am a performance nutt too.

But... (lol)

My take is that that this is the job of a minifier, not an editor. Separation of concerns and all. I feel an editor should be as non-destructive as possible. Meaning the act of opening, not making any modifications, and saving, is "expected" to yield the same output as the input. And when changes "are" made, they should "only" effect the area of interest. A minifier on the other hand is "expected" to mangel and compile the code to new version, yet maintain the same desired result in a more performant way.

We have all used editors that rewrite our code; and it is annoying as hell when they won't let us do what we want or change the output unnecessarily.

Now, I know that this is somewhat unrealistic expectation when we start injecting browser DOMs from various vendors, and RTEs like CKE and Tiny in to the mix... LMAO. But we should certainly try not to add to that hot mess. :)

---

Below is the updated logic based on further testing. I replaced it with a regex instead for now which works as intended when compared against the prior logic. That said, we may want run this through a 10-20k test cycles and to check it's performance against other options for larger page loads. It is a regex after all.

Let me know your thoughts.

if (!/^\s+$/.test(content!) && !content!.trim()) {
      continue;
}

[artf]

Let's see these changes in the dedicated PR, for this one we only need a few test cases 🙂

[artf]

@bernesto looks all good, will wait for the tests with the new updated characters in escapeName

[Yorgv]

Are there any updates on this? Currently running into the same problem where my Tailwind classes are being escaped incorrectly.

[Yorgv]

Also, I noticed this fix doesn't account for classes with a hex color. For example, the class border-[#E9DECC] will still be converted to border-[-E9DECC] after this fix. So I think # also needs to be added to the regex.

[bernesto]

I haven't had time to write the tests to commit this pull. On another project right now. Probably won't until the week after next.

[Yorgv]

I haven't had time to write the tests to commit this pull. On another project right now. Probably won't until the week after next.

Totally understandable, thanks for your efforts!

[artf]

Added the missing # + tests

[bernesto]

Thank you!