fix(ui-markdown-editor): linebreak - #263, #267, #269

[d-e-v-esh]

Closes #263 #267

Update

This was my thought process of finding the problem and the solution.

Identifying the problem

• As you can see here, we do not have any function for the softbreak.

• The functions that exist there are insertLinebreak and insertHeadingbreak.

• So I thought that maybe softbreak was not working because there is no function for it. But that is actually not the case.

• The softbreak was not working because of this conditional statement here. This basically means that if Enter is being pressed from a non-heading block then we totally skip out hotkey actions. That is also the reason why pressing Ctrl + Enter did not inset a page break. If you remove that statement then softbreak and page break works like normal.

• And this should TOTALLY prevent us from getting a linebreak in a paragraph. And the only reason pressing Enter is still functional is because of this and this functions where we are overriding insertBreak(). You can put a few console.log() there and check which functions work and which do not.

• That is also the reason why inserting a page break (Ctrl + Enter) and softbreak(Shift + Enter) works absolutely fine for all the heading blocks because this conditional statement only restricts non-heading blocks from all the Enter functions.

Exchange in types of break Bug

• As I mentioned earlier in the second point that the only functions that existed are the insertLinebreak and insertHeadingbreak.
• To find out which function is doing what, we need to remove this conditional statement here and simply add a console.log() statement after this line like this:

    hotkeys.forEach((hotkey) => {
      if (isHotkey(hotkey, event)) {
        event.preventDefault();
        const { code, type } = HOTKEYS[hotkey];
        console.log(code) <===
        hotkeyActions[type](code);
      }
    })

By doing this, we get:

Here:

Pressing Enter results in a => headingbreak Pressing Shift + Enter results in a => linebreak

This is the case because this is how it is mentioned in the hotkeys file as you can see here.

• And now we know that pressing Shift + Enter is creating a softbreak as it should but it is named as a linebreak and
  • Pressing Enter is always creating a false headingbreak no matter the type of block. It is false headingbreak and does not do anything because of the problem I mentioned above that is pressing Enter is useless in a non-heading block because of this conditional statement here

---

What is happening till now: Every time we press Enter, a heading break is going to get inserted.

How it was implemented was:

1. We created a boolean which tells us if the current block is a heading.
2. We created a restriction on the enter keyword when being pressed from a non-heading block. This was causing #263
3. If enter is called from a headingblock then we will go to our custom hotkeyActions and create a heading break
4. But if the enter is called from a non - heading block then we will skip our custom hotkeyActions and a default enter linebreak will occur

Changes

1. Converted headingbreak to linebreak
2. This converts every single enter press to a simple linebreak and if enter is pressed from a heading block, only then we will make it a headingbreak
3. This removes the restriction on the enter key in the paragraph so the page break works.

Related Issues

• Issue #263 #267 #269
• Pull Request #220

Author Checklist

• [x] Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
• [x] Vital features and changes captured in unit and/or integration tests
• [ ] Commits messages follow AP format
• [ ] Extend the documentation, if necessary
• [x] Merging to master from fork:branchname
• [x] Manual accessibility test performed
  • [ ] Keyboard-only access, including forms
  • [ ] Contrast at least WCAG Level A
  • [ ] Appropriate labels, alt text, and instructions

[d-e-v-esh]

@irmerk That is great. Will that take very long? Should I write the description of changes and limitations in more detail?

[jolanglinais]

@irmerk That is great. Will that take very long? Should I write the description of changes and limitations in more detail?

Yeah I think so. This is a sensitive change, so as much helpful context would be great.

[d-e-v-esh]

@dselman @irmerk I have updated the pull-request body and added more context highlighting the problem. Does this bring more clarity? Ask me any question that comes to your mind or regarding anything that was unclear.

[dselman]

When I test this using the Netlify preview there's something strange happening with the selection. Try selecting multiple paragraphs forwards, or backwards, and you see that the selection is getting extended outside of the selection target.

[d-e-v-esh]

@dselman I don't see that behavior. Can you make a screen recording to show exactly what is strange?

[K-Kumar-01]

@d-e-v-esh https://www.loom.com/share/56a2f8833f9f481e96dd5582fce0b745 I think @dselman may be talking about this thing.

[dselman]

@dselman I don't see that behavior. Can you make a screen recording to show exactly what is strange?

It seems to be specific to Safari - I don't see it on Chrome.

[d-e-v-esh]

@K-Kumar-01 Is this happening only on this update or is this on the master branch? You cut out the URL.

[K-Kumar-01]

@K-Kumar-01 Is this happening only on this update or is this on the master branch? You cut out the URL.

@d-e-v-esh It is happening in the deployment of this PR only. I have rechecked it. I clicked on the deploy preview and then made the above video.

[d-e-v-esh]

@K-Kumar-01 That is actually a really bad bug. I'll see what is causing the problem and update.

[d-e-v-esh]

@dselman Can you show which merge started converting blank lines to \ in markdown that you talked about here down in #269? I think that could be the one causing this problem. Because that was not happening in the initial commit and that did not happen when I run it locally until a did a git pull and now nothing I do reverses this thing.

[d-e-v-esh]

@irmerk Does this look good now?

[jolanglinais]

Yes looks good. I think wait to rebase again (because there are conflicting files after I merged a couple other PRs) until we review.

[jolanglinais]

I've finally been able to look more into this. This is really thorough and I think is on the right track. Some residual awkward behavior I'm seeing is when you press ENTER at the end of "My Heading" at the top of the demo, it inserts two new paragraphs, both of which are not headings, and places the cursor at the beginning of the second one. Could this be because we are inserting lineBreakNode in insertLineBreak?

[jolanglinais]

As of right now #263 and #269 are solved for sure. I believe #267 is resolved, will want extra eyes on this from @dselman and/or @jeromesimeon.

[d-e-v-esh]

@irmerk Oh yes, I remember that being very annoying. I don't know why I kept it, probably because it was there from the beginning. And we were talking about implementing this after the linebreak issue is resolved so that would be getting solved anyway. It is fixed for now.

[jolanglinais]

Great! It looks like there are still merge conflicts, so you'll want to rebase to resolve those.

[d-e-v-esh]

@irmerk I am not sure if this worked. Did the conflicts go away? I can't see them.

[jolanglinais]

Suspicious, I refreshed and it said the conflicts were there, refreshed again and they were gone.

[jolanglinais]

There are still two issues with headings:

1. Place cursor at beginning of "My Heading" Press ENTER See that new normal line is created before and cursor is placed above "My Heading"
2. Place cursor in the middle of "My Heading" Press ENTER See that new normal line is created in between "My Heading" where cursor was placed.

[jolanglinais]

Note: I'm looking at your code locally to see if I can see the solution.

[d-e-v-esh]

@irmerk I think we can create a new and fully working headingBreak with all the bugs fixed under the PR of #260 after this is merged. I think that would be better, isn't it? I think fixing the headingBreak is whole another thing in itself.

[jolanglinais]

OH yes! Good point. There's so many related Issues/PRs here I'm getting confused. Okay great, even better. This PR looks good so I'm going to merge!

[jolanglinais]

@d-e-v-esh we're going to put the changes in this PR on the working group call tomorrow to try to make sure it doesn’t lead to further confusion around linebreaks/softbreaks. If you’ll be on during the call, feel free to add to the conversation. If not, I think @dselman and I can present what the changes will do.

[d-e-v-esh]

@dselman I think the reason for that seems to be this that I talked about earlier. It seems like something changed in the conversion. That behavior is not present in the master and it was also not present when I made the PR. Do you know where it could be coming from?

[dselman]

@dselman I think the reason for that seems to be this that I talked about earlier. It seems like something changed in the conversion. That behavior is not present in the master and it was also not present when I made the PR. Do you know where it could be coming from?

Sorry, no idea. Can you reproduce the issue when you run locally using this branch?

[d-e-v-esh]

@dselman Yes I can. It doesn't go back to when this was not happening.

[jolanglinais]

Update: This PR should no longer address #267

[jolanglinais]

Also, this should unblock #260 and #284

[d-e-v-esh]

@irmerk I have implemented the new paragraphBreak and also fixed the error that @dselman pointed out. I think it should be perfect now. Now we just need to update the demo text on the editor.

[jolanglinais]

Pinging @dselman on this again.

[dselman]

@d-e-v-esh looks great. Can you please resolve the merge conflicts and then I will merge.

[dselman]

Would love to get this into the next release (this week). @d-e-v-esh will you have time to resolve the merge conflicts?

[adithyaakrishna]

@d-e-v-esh Any Updates on this? If not, I would love to fix it up and send a PR

[d-e-v-esh]

@adithyaakrishna Got extremely busy. I'll get to it in a few days. You can find other issues. This PR is complete, just needs the merge conflicts to be resolved.

[d-e-v-esh]

@dselman I removed all the merge conflicts and it works totally great. You can check it out.