Allow to pass optional attributes to splitListItem

eric-burel commented

2023-05-30 15:06:09 +02:00

(Migrated from github.com)

Hi,

When splitting a list item to create a new one, we may want the new item to have specific attributes. My use case is splitting items for a task list: if you split a checked item, you may want the new item to be unchecked. But as a default, the splitted node is copied with the same attribute, here checked: true.

Problem: somehow with the proposed code, canSplitItem is false. Yet if I uncomment this check, the splitting works well. I haven't yet figured the change needed in canSplitItem.

I haven't fully investigated but I suspect that in "prosemirror-transform/src/structure.ts" this check doesn't pass if I specify some attributes:

    if (after != node) rest = rest.replaceChild(0, after.type.create(after.attrs))
    if (!node.canReplace(index + 1, node.childCount) || !after.type.validContent(rest))
      return false

Hi, When splitting a list item to create a new one, we may want the new item to have specific attributes. My use case is splitting items for a task list: if you split a checked item, you may want the new item to be unchecked. But as a default, the splitted node is copied with the same attribute, here `checked: true`. **Problem:** somehow with the proposed code, `canSplitItem` is false. Yet if I uncomment this check, the splitting works well. I haven't yet figured the change needed in `canSplitItem`. I haven't fully investigated but I suspect that in "prosemirror-transform/src/structure.ts" this check doesn't pass if I specify some attributes: ``` if (after != node) rest = rest.replaceChild(0, after.type.create(after.attrs)) if (!node.canReplace(index + 1, node.childCount) || !after.type.validContent(rest)) return false ```

marijnh commented

2023-05-31 08:27:27 +02:00

(Migrated from github.com)

Looks good. Merged.

eric-burel commented

2023-05-31 08:55:07 +02:00

(Migrated from github.com)

@marijnh thanks but did you take a look at the "canSplit" issue? It's somehow never true when I pass an attribute, despite the split working correctly when I comment the "canSplit" call, so passing an attribute will have no effect

marijnh commented

2023-05-31 09:29:06 +02:00

(Migrated from github.com)

Oh, I missed that. (Next time, please mark the PR as a draft if it's not ready to be merged.)

I can't reproduce the issue. Can you provide the schema you're using?

Oh, I missed that. (Next time, please mark the PR as a draft if it's not ready to be merged.) I can't reproduce the issue. Can you provide the schema you're using?

eric-burel commented

2023-06-01 07:40:23 +02:00

(Migrated from github.com)

Yes sorry for the messy PR!

The relevant part of the schema is like so (nothing fancy for lists):

export function commonAttributes() {
    return {
        style: { default: null },
        class: { default: null },
        id: { default: null },
    };
};
const nodes = {
    doc: { content: "block+" },
    paragraph: {
        content: "inline*",
        group: "block",
        parseDOM: [{ tag: "p" }],
        toDOM() { return ["p", 0] }
    },
    text: { group: "inline" },
    ordered_list: {
        content: "list_item+",
        group: "block",
        attrs: {
            ...commonAttributes(),
            type: { default: null },
            order: { default: 1 },
        },
        parseDOM: [
            {
                tag: "ol",
                getAttrs: (dom) => {
                    if (!(dom instanceof HTMLElement)) return false
                    return {
                        ...getAttributes(dom),
                        order: dom.hasAttribute("start")
                            ? parseInt(dom.getAttribute("start") || "1", 10)
                            : 1,
                    };
                },
            },
        ],
        toDOM: (node) => {
            return node.attrs.order === 1
                ? hasAttrs(node.attrs, "order")
                    ? ["ol", getAttrs(node.attrs, "order"), hole]
                    : ["ol", hole]
                : [
                    "ol",
                    { ...getAttrs(node.attrs, "order"), start: node.attrs.order },
                    hole,
                ];
        },
    },
    bullet_list: {
        content: "list_item+",
        group: "block",
        attrs: { ...commonAttributes() },
        parseDOM: [{ tag: "ul", getAttrs: getAttributes }],
        toDOM: (node: Node) => hasAttrs(node.attrs) ? ["ul", getAttrs(node.attrs), hole] : ["ul", 0],
    },
    list_item: {
        content: "block*",
        attrs: { ...commonAttributes() },
        parseDOM: [{ tag: "li", getAttrs: getAttributes }],
        toDOM: (node: Node) => hasAttrs(node.attrs) ? ["li", getAttrs(node.attrs), hole] : ["li", 0],
        defining: true,
    },

So the problem is that canSplit(tr.doc, $from.pos, 2, types)):

is false when types === [{type: schema.nodes.list_item, attrs: {}} /* this prevents copying the item attributes*/, {type: nextType }
while it is true for types === [null /* this will copy the item attributes*/, { type: nextType}]

Also I've tried crafting a unit test like so:

let splitResetAttrs = splitListItem(schema.nodes.list_item, {})
it("can split a list item without copying its attributes", () =>
      apply(doc(ul(li({ class: "foo" }, p("foo<a>bar")))), splitResetAttrs, doc(ul(li(p({ class: "foo" }, "foo")), li({}, p("bar"))))))

But it doesn't seem to actually check for attributes so it's not reliable

Yes sorry for the messy PR! The relevant part of the schema is like so (nothing fancy for lists): ```ts export function commonAttributes() { return { style: { default: null }, class: { default: null }, id: { default: null }, }; }; const nodes = { doc: { content: "block+" }, paragraph: { content: "inline*", group: "block", parseDOM: [{ tag: "p" }], toDOM() { return ["p", 0] } }, text: { group: "inline" }, ordered_list: { content: "list_item+", group: "block", attrs: { ...commonAttributes(), type: { default: null }, order: { default: 1 }, }, parseDOM: [ { tag: "ol", getAttrs: (dom) => { if (!(dom instanceof HTMLElement)) return false return { ...getAttributes(dom), order: dom.hasAttribute("start") ? parseInt(dom.getAttribute("start") || "1", 10) : 1, }; }, }, ], toDOM: (node) => { return node.attrs.order === 1 ? hasAttrs(node.attrs, "order") ? ["ol", getAttrs(node.attrs, "order"), hole] : ["ol", hole] : [ "ol", { ...getAttrs(node.attrs, "order"), start: node.attrs.order }, hole, ]; }, }, bullet_list: { content: "list_item+", group: "block", attrs: { ...commonAttributes() }, parseDOM: [{ tag: "ul", getAttrs: getAttributes }], toDOM: (node: Node) => hasAttrs(node.attrs) ? ["ul", getAttrs(node.attrs), hole] : ["ul", 0], }, list_item: { content: "block*", attrs: { ...commonAttributes() }, parseDOM: [{ tag: "li", getAttrs: getAttributes }], toDOM: (node: Node) => hasAttrs(node.attrs) ? ["li", getAttrs(node.attrs), hole] : ["li", 0], defining: true, }, ``` So the problem is that `canSplit(tr.doc, $from.pos, 2, types))`: - is false when `types === [{type: schema.nodes.list_item, attrs: {}} /* this prevents copying the item attributes*/, {type: nextType }` - while it is true for `types === [null /* this will copy the item attributes*/, { type: nextType}]` Also I've tried crafting a unit test like so: ```ts let splitResetAttrs = splitListItem(schema.nodes.list_item, {}) it("can split a list item without copying its attributes", () => apply(doc(ul(li({ class: "foo" }, p("foo<a>bar")))), splitResetAttrs, doc(ul(li(p({ class: "foo" }, "foo")), li({}, p("bar")))))) ``` But it doesn't seem to actually check for attributes so it's not reliable

marijnh commented

2023-06-01 11:34:17 +02:00

(Migrated from github.com)

Thanks, that helps. That was a bug in canSplit — prosemirror-transform 1.7.3 should fix it.

Thanks, that helps. That was a bug in `canSplit` — prosemirror-transform 1.7.3 should fix it.

❤️ 1

Rows
Columns

Allow to pass optional attributes to splitListItem #11