Skip to content

Converting a selection can result in a proliferation of emtpy `<div>`s #1

Open
adam-p opened this Issue May 17, 2012 · 0 comments

1 participant

@adam-p
Owner
adam-p commented May 17, 2012

Reproduction:

  1. Create an email with a few lines separated by empty lines.

  2. Inspect the DOM. It should look like this:

    <div>my line 1</div>
    <div><br></div>
    <div>my line 2</div>
    <div><br></div> 
  3. Select a line but putting the cursor at the beginning of a non-empty line, holding the shift key, and pressing the down key.

  4. Markdown Toggle.

  5. Look at the markdown-here-wrapper that was created -- its data-md-original attribute has an empty <div> at the end of it that wasn't present in the original DOM.

If you then revert and repeat, there will be two empty <div>s, and so on.

The problem is that the range of the original selection has an endContainer of the <div><br></div> (outer) element, and a endOffset of 0, which means that it ends before the <br>.

Then in getSelectedHtml(), when we call selectedRange.cloneContents(), the partial <div> turns into an empty <div> element, and the original stays on the page (or it might be a new one enclosing the original <br> -- same result).

Impact

It's hard to know how severe this bug is. If it's just creating empty <div>s, then it's pretty minor. But, in theory, it could introduce more significant elements.

Even with <div>s, it can still mess up the Markdown. For example, if you have two plaintext lines with no empty line between then, they will be treated as the same line when rendering. But if you convert and revert the first line, then there'll be an empty <div> between then. If you then try to convert both lines, they'll be rendered as two separate lines. It's not difficult to recover from the situation, but it could be confusing and annoying.

Solutions

A simple solution might be to remove empty <div>s at the end of the output of getSelectedHtml(). But this doesn't account for the possibility that this bug can affect other tag types.

Maybe there will always be an empty element at the end of the outerHTML when the endOffset is 0? If so, that might be a good general solution.

A more robust solution might be to shrink or grow the end of the range so there's no partial element. But I'm not sure that this will actually work. And ranges are still a bit mysterious to me. And I fear the potential for bad side-effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.