Squire icon indicating copy to clipboard operation
Squire copied to clipboard

Newlines/divs being added when the last child element of the root is a block element other than _config.blockTag

Open TehShrike opened this issue 1 year ago • 1 comments

You can reproduce this using the current Fastmail editor:

  1. copy in a header element, or click the button in the UI to add a list
  2. type some text into the header/list item
  3. make sure there are no lines below the header/list
  4. with your cursor inside the header/list, press backspace to delete one of the characters inside of it

Expected behavior: the header/list should still be the last child element inside the root element.

Observed behavior: a new div is inserted after the header/list, containing a br. This results in a new blank line.

This does not happen if the header/list item has other elements after it, it only happens when the last child element has a nodeName that differs from _config.blockTag.

The bug is caused by a tricksily wrong conditional check here:

https://github.com/fastmail/Squire/blob/cbd8881e78c2af0305a0b6a06cdff16c0c0b05ed/source/Editor.ts#L1924-L1930

It's a little easier to see why that's wrong after sprinkling some De Morgan over the last part of the condition:

last.nodeName !== this._config.blockTag || !isBlock(last)

turns into:

!(last.nodeName === this._config.blockTag && isBlock(last))

this will be false whenever the nodeName is equal to the blockTag, but it will be true for all other block types.

You can fix this by removing the check against _config.blockTag entirely, the isBlock check should be sufficient:

        if (
            !last ||
            !isBlock(last)
        ) {
            root.appendChild(this.createDefaultBlock());
        }

If you expect someone to pass in a blockTag that doesn't pass the isBlock check, you could do this (I really doubt this is desirable):

        if (
            !last ||
            (last.nodeName !== this._config.blockTag && !isBlock(last))
        ) {
            root.appendChild(this.createDefaultBlock());
        }

Does this make sense? Would you like a PR?

TehShrike avatar Dec 06 '24 20:12 TehShrike

I encountered the same behavior while using the editor, which unnecessarily complicated my logic for determining if the field was empty. I agree that this qualifies as a bug.

Discolai avatar Dec 20 '24 06:12 Discolai