Regression in DomParser style matching from 1.21.0 to 1.21.1

nperez0111 commented 1 month ago

Hi @marijnh, I'm one of the maintainers of Tiptap and we've hit a regression when upgrading from prosemirror-transform@1.21.0 to prosemirror-transform@1.21.1

This is a reduced version of what we are doing over the course of many functions calls

import { DOMParser } from 'prosemirror-transform';

const parser = DOMParser.fromSchema(schema)

const htmlElement = new window.DOMParser().parseFromString('<body><p><span style="text-decoration: underline">Example Text</span></p></body>', 'text/html').body;

const result = parser.parse(htmlElement).toJSON()

In 1.21.0 we get:

{
    "type": "doc",
    "content": [
        {
            "type": "paragraph",
            "content": [
                {
                    "type": "text",
                    "marks": [
                        {
                            "type": "underline"
                        }
                    ],
                    "text": "Example Text"
                }
            ]
        }
    ]
}

And, in 1.21.1 we get:

{
    "type": "doc",
    "content": [
        {
            "type": "paragraph",
            "content": [
                {
                    "type": "text",
                    "text": "Example Text"
                }
            ]
        }
    ]
}

So the underline mark is not matching, when it should be. This is what we are using as the underline spec:

In our code: https://github.com/ueberdosis/tiptap/blob/b941eea6daba09d48a5d18ccc1b9a1d84b2249dd/packages/extension-underline/src/underline.ts#L52-L56

I'm not familiar enough with CSSStyleDeclarations to totally understand what the difference could be, but here is the diff: https://github.com/ProseMirror/prosemirror-model/compare/1.21.0...1.21.1

Really appreciate your time, no rush to fix, we will just stay on 1.21.0 for now.

Nantris commented 2 weeks ago

Friendly bump. This is something we can work around but it does have the potential to break any new Tiptap projects started on the stable versions, or for anyone running yarn upgrade.

marijnh commented 2 weeks ago

You're going to have to provide a full reproduction (using plain ProseMirror). I don't see anything going wrong when I test a parse rule like that.

marijnh commented 2 weeks ago

Oh never mind, I see what's going on. Browsers (but not JSDOM) 'normalize' composite properties like text-decoration or border into more detailed ones (such as text-decoration-line and border-width), causing them to not match.

This is kind of annoying because it means our style matching system, which treats styles as property name + value pairs, doesn't really match the reality of CSS, where there's multiple property names used to express the same thing. I.e. the issue is not just that a rule for text-decoration should match elements that have the normalized text-decoration-line style, but also that it wouldn't be unreasonable to expect rules for a text-decoration-line property to match elements that have text-decoration in their style.cssText. So the old approach is also flaky.

marijnh commented 2 weeks ago

Found a workaround — you can still query style[shorthandprop] even if it doesn't show up in that form in the style declaration's normalized items. So with this patch the parser just iterates over all style properties that have a parse rule defined for them, checking whether they exist in a given (non-empty) style set.

nperez0111 commented 2 weeks ago

Thank you @marijnh, all of our tests are passing now, so I'm confident in your fix.

Thank you for your quick response!

ProseMirror / prosemirror

Regression in DomParser style matching from 1.21.0 to 1.21.1 #1473