New line added with bold formatting

[shreyaa-s-zz]

Please describe the problem (or idea)

When a snippet of plain text is selected and bold formatting is applied, a new line is added to the bottom of the plain text.

Representation of the same:

Thanks

[shreyaa-s-zz]

@jywarren @emilyashley @cesswairimu please verify the same.

[cesswairimu]

Hey @Shreyaa-s yeah I see that is happening...it doesn't happen on the PL website implementation of the editor though its worth checking whats up...would be interested in working on this? thanks

[shreyaa-s-zz]

hey @cesswairimu sure! I would love to give this a try. Thanks

[govindgoel]

@Shreyaa-s Any updates, are you still working on this? I guess this issue is with the text present in a bulleted list or in the row and column format. Can you look further and confirm.

[shreyaa-s-zz]

@govindgoel I checked but the bug doesn't follow a fixed pattern. Sometimes it adds a new line to plain text as well and sometimes it doesn't. I have trouble pinpointing the exact snippet of code that breaks. I will keep you posted if I come across anything.

[shreyaa-s-zz]

@keshav234156 @NitinBhasneria @Shulammite-Aso if any of you has worked with woofmark before, can you help me figure out how this function works? Thanks.

var rleading = /^(\**)/;
var rtrailing = /(\**$)/;
var rtrailingspace = /(\s?)$/;
var strings = require('../strings');

function boldOrItalic (chunks, type) {
  var rnewlines = /\n{2,}/g;
  var starCount = type === 'bold' ? 2 : 1;
  chunks.trim();
  chunks.selection = chunks.selection.replace(rnewlines, '\n');

  var markup;
  var leadStars = rtrailing.exec(chunks.before)[0];
  var trailStars = rleading.exec(chunks.after)[0];
  var stars = '\\*{' + starCount + '}';
  var fence = Math.min(leadStars.length, trailStars.length);
  if (fence >= starCount && (fence !== 2 || starCount !== 1)) {
    chunks.before = chunks.before.replace(new RegExp(stars + '$', ''), '');
    chunks.after = chunks.after.replace(new RegExp('^' + stars, ''), '');
  } else if (!chunks.selection && trailStars) {
    chunks.after = chunks.after.replace(rleading, '');
    chunks.before = chunks.before.replace(rtrailingspace, '') + trailStars + RegExp.$1;
  } else {
    if (!chunks.selection && !trailStars) {
      chunks.selection = strings.placeholders[type];
    }

    markup = starCount === 1 ? '*' : '**';
    chunks.before = chunks.before + markup;
    chunks.after = markup + chunks.after;
}

[keshav234156]

@Shreyaa-s This function is for Italic and Bold formatting in markdown mode.
This variable var rleading,... are defined in terms of Regular Expression (use this to know about what each of regular expression means https://regexr.com/ ). chunk.selection is the text which is selected, start count is 1 or 2 depending on whether it's italic or bold, as in markdown mode italic text is denoted by text and bold by text

  var leadStars = rtrailing.exec(chunks.before)[0];
  var trailStars = rleading.exec(chunks.after)[0];

This checks number of leading and trailing

You can get to know more about the function by adding a console.log(name_of_variable) statement and trying different test cases.

[shreyaa-s-zz]

The problem with bold tags only occurs in rich text mode, or when we switch modes. From what I've observed the problem goes like this.

Sample Text:
Here are a few things that I've noted.
Desired Output:
Here are a few things that I've noted.
Actual Output:
Here are a few things that I've
noted.

This is because the bold formatting functions like this:

While selecting text:
Before selected text: <p>Here are a few things that I've
Selected text: noted.</p>
After selected text: .....

After the bold operation:
Before selected text: <p>Here are a few things that I've</p><strong>
Selected text: <p>noted.</p>
After selected text: </strong> .....

As you can see an additional <p></p> tag is introduced, which shifts the data to a new line. Similarly a <ul></ul> is introduced while dealing with lists. This happens due to the implementation of function wrapping and pushoverOtherTags. Adding images for better reference:

[shreyaa-s-zz]

The function wrapping is used for three formatting options: The boldOrItalic, blockquote and codeblock. They behave similarly though it is not noticeable since they inherently shift the selected text to the next line.

[shreyaa-s-zz]

@jywarren I had a query. Since you've worked with this piece of code before, is there any case where closing any open tag before formatting would be absolutely necessary? Because from what I've observed simply not calling the pushover() function will solve this issue. I tried for various test cases but didn't encounter any major drawback.
We can specify which tags to check for and only close specific ones. I tried implementing it but it didn't work.
I'm afraid I'm missing something crucial here, it would be great if any of you could take a look and guide me down the right path. Thanks!

[jywarren]

Hi, what tremendous investigating is going on here! Really great and super good collaboration too. Congrats all! This is especially tough code to dig through, too.

I can't think of a case, but a few thoughts:

1. we should try running the woofmark tests because they describe a LOT of cases
2. we should consider adding a test that illustrates the issue you've found here, to justify the change
3. if we can brainstorm/develop any tests that illustrate when pushover() is actually needed, that'd be great...?
4. we can possibly ping the original dev of woofmark to double-check
5. if there are cases where it's needed, maybe we need some kind of conditional to skip the pushover() in only some cases, and writing tests can help us establish when that ought to happen or not

[ebarry]

Wow, this is really impressive detective work! I'm amazed!

[shreyaa-s-zz]

Could you point me to where the woofmark tests are defined? I went through the code and description of woofmark but couldn't pinpoint them. I'm thinking of cases where pushover() is needed and going through the domador part of woofmark to understand parsing to HTML and markdown a little better. Thanks!

[keshav234156]

I think that woofmark is not protected by tests.

[jywarren]

Hmm, apologies, i believe what happened is that woofmark is built on an HTML=>Markdown converter and a Markdown=>HTML converter:

• https://github.com/bevacqua/megamark
• https://github.com/bevacqua/domador

Each of these have tests. And woofmark depends on them but has no tests of its own. My apologies for not remembering this! Gosh, i suppose this means we may need to try to create some basic integration tests for woofmark if we aren't able to reproduce this interaction in a controlled environment with pushover(). What do you all think?

[keshav234156]

@Shreyaa-s Are you able to figure out what pushover() and pushoverOthersTag() are doing? I am still not able to get it.

[shreyaa-s-zz]

Hi @keshav234156 , here's a rough idea:
pushover() looks for opening and closing of tags and calls pushoverOtherTags() .
This defines the opening and closing of tag.

var rclosed = new RegExp('<\/' + tag.replace(/</g, '</') + '>', 'i');
    var ropened = new RegExp('<' + tag + '( [^>]*)?>', 'i');

Here we check if a a tag is opened but not closed in the selected part, then it adds a closing tag after the selected part. An example would be, if the selected part is

my goals are as follows: <ul>
<li>travel... 

The end result would be :

my goals are as follows: <ul>
<li>travel...</li></ul>

Code:

if (open && !rclosed.test(chunks.selection.substr(i))) {
      chunks.selection += '</' + tag + '>';
      chunks.after = chunks.after.replace(/^(<\/[^>]+>)/, '$1<' + tag + attrs + '>');
    }

Similarly this looks if a tag is closed but not opened in the selected text. For example </p> at the end of line then it adds an opening tag at the start of selected text.

    if (closing && !ropened.test(chunks.selection.substr(0, i))) {
      chunks.selection = '<' + tag + attrs + '>' + chunks.selection;
      chunks.before = chunks.before.replace(/(<[^>]+(?: [^>]*)?>)$/, '</' + tag + '>$1');
    }

[shreyaa-s-zz]

The more I work on this, the more I feel that I'm not looking in the right direction. People tend to bolden a word or a line, not a complete list. And even if we want to apply it to a list the ideal way would be:

<li><strong></strong></li>
<li><strong></strong></li>

And how it implements right now is:

<strong>
<li>...</li>
<li>..</li>
</strong>

This breaks if we convert it to md and back, the reason why I was learning how data is parsed.
The author of woofmark must've had some case in mind for which he felt that pushover() is necessary. I'm trying to understand that.
I'll keep digging into it, and have a look at the tests that @jywarren mentioned. The bold tag has 3 open issues related to it or wrapping, and I'm trying to connect the dots to see if they're somehow connected. Meanwhile, I'll pick up a new issue that can fetch some immediate results. Thanks!

[jywarren]

This is really good investigation, thank you @Shreyaa-s !! I tend to agree with you that the existing model for nesting boldness and lists is not right. It could be worth looking if HTML even strictly allows for lists inside of <strong>? That could help us make a case for a different way of doing it in woofmark.

What we could do is implement a secondary model for it, nested differently, and include a switch in the options. domador, woofmark, and megamark all have an options parameter (https://github.com/bevacqua/domador#domadorinput-options) where things can be customized, and we could introduce an option called boldingListOrder or something, documented nicely in the README, which allows switching this.

The advantage would be that this wouldn't break any existing tests or downstream usages in the old model, and would allow us to introduce our "better" model with a test of our own which would side-step the debate about which model is better :-)

What do you think?

[shreyaa-s-zz]

Hey @jywarren ! I looked into it and HTML does allow lists inside <strong>. I absolutely agree with you, it's better to implement a secondary model rather than making changes in the existing ones, leaves less room for error and code breaking. The options parameter is something I discovered just now, so it'll take me some time to go through the documentation and think about the implementation.

Not getting the desired output in the above-mentioned test case is due to how we parse the code between different modes. On that note, this is just one test case, there's a high possibility that somewhere else too the code breaks because of a similar reason. We could club all such bugs together and then think of a solution. What do you all think?

[jywarren]

This sounds perfect, @Shreyaa-s. And, any PR and/or tests we use could help to explain/demo the various cases and why it's a complex situation. woofmark is nice and cleanly written but it is not great at explaining itself, you know? It's a bit convoluted and takes a while to figure out! Let's aim to be a little more legible.