Resolves #1618, preserve the markup if it's the last element

[ggrossetie]

No description provided.

[ggrossetie]

@RunDevelopment I didn't find any tests on this plugins, is there a way to add tests ?

[RunDevelopment]

@Mogztter No, plugins are not tested (at least right now).

[RunDevelopment]

It seems like the current implementation of Keep Markup assumes that every tag contains at least one character, causing #1618.

This PR does handle the case of the last tag correctly but it handles some other case worse than the current implementation.

1. Order swapping:
   The order of all zero-length tags which have the same start position will the reversed. E.g.:
   <x></x><y></y> -> <y></y><x></x>
2. Tag inclusion:
   Zero-length tags after a closing tag will be included into the preceding tag (in reversed order). E.g.:
   <x>foo</x><y></y><z></z> -> <x>foo<z></z><y></y></x>

Tag inclusion is probably just a symptom of #1640.

Right now, this implementation does more harm than good, so we will have to address the above points before we can merge this.

[ggrossetie]

OH! we really need tests 😄
Can I submit a proposal to add unit tests on this plugin ? Otherwise it will be a nightmare to fix 😐

[RunDevelopment]

Of course, you can submit the proposal but it will require to implement a very different test suite than the one we have right now. So, no small task.

Otherwise it will be a nightmare to fix

I don't see why that is. A little hacky but I just made a little test page based on the index page of the plugin. Insert the markup to be tested and verify yourself. Not pretty but it works.

[mAAdhaTTah]

If we wanted to go all-out, something w/ JSDOM + snapshot tests could work. We have a (admittedly complex) mocha/chai setup for the current tests. Might be nice to get some tests going for the plugins.

[ggrossetie]

If we wanted to go all-out, something w/ JSDOM + snapshot tests could work.

That's exactly what I had in mind 👍
I've added a failing tests but I don't know how to solve this issue. To be honest I'm a bit lost with all the start/end position...

$ npm run test:plugins

[mAAdhaTTah]

@Mogztter I really appreciate the enthusiasm and the contribution. I think if we're going to do this (introduce testing for plugins), we should break up this PR into one that introduces the framework and maybe some basic, passing tests, then a separate PR that adds tests for & fixes this bug. That'll allow us to evaluate each of those things separately.

[ggrossetie]

I think if we're going to do this (introduce testing for plugins), we should break up this PR into one that introduces the framework and maybe some basic, passing tests, then a separate PR that adds tests for & fixes this bug.

@mAAdhaTTah Sure no worries.
My initial bugfix is a one-liner. I can revert this change and only add a test that check that the extension has been registered ? ie. Prism.hooks.all['before-highlight'] and Prism.hooks.all['after-highlight'] are not empty ? Does it sound good to you ?

[mAAdhaTTah]

@Mogztter I'd prefer to evaluate the bugfix in a separate PR. It's not clear to me this comment isn't a result of introducing that change. What I'd suggest is ending up with 2 PRs:

1. Introduce the testing infrastructure, including some basic tests for this plugin that pass, without changing any plugins. We can then start adding tests for the plugins as we go.
2. A PR that fixes this bug and introduces tests to cover the bugfix.

How you want to break that up (modifying this PR to be just tests, or closing and creating them separately) is up to you, but that's the place I'd like to end up if we want to add tests at this time.

The smaller the PR, the easier it will be to get each of them in, and I'd like to evaluate & think through the testing infrastructure as well as whatever tests you're introducing without also needing to evaluate the change(s) you're making to the plugin.

Does that make sense? My apologies for making this complex; I threw out the suggesting not expecting you to run with it so fast. And I appreciate it, but it does ramp up the overall complexity of what you're doing, and I just want to simplify it as much as possible.

[ggrossetie]

@mAAdhaTTah Done, see #1646

[mAAdhaTTah]

Let's get the tests in first, then we'll get this fix.

[ggrossetie]

@mAAdhaTTah Rebased 👍
@RunDevelopment You're right, I can reproduce the "order swapping" regression. With this fix the test "should preserve markup order" is failing 😉

[mAAdhaTTah]

Hey, sorry for the delay on this, but coming back to this now. I see one code change, which I guess is fine? Does this change the commented out test? We should either delete it and replace it with a test that represents the intended behavior or update it.

That said, if this change removes that <a> tag in the test, that seems like a breaking change to me, and if the underlying issue is #1640, then I think this fix is invalid, unfortunately. We should probably tackle that problem holisitically.

I'm going to go ahead & close, but happy to reopen if anything above I've said it wrong.