Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Regression in DomParser style matching from 1.21.0 to 1.21.1 #1473

Closed
nperez0111 opened this issue Jun 11, 2024 · 5 comments
Closed

Regression in DomParser style matching from 1.21.0 to 1.21.1 #1473

nperez0111 opened this issue Jun 11, 2024 · 5 comments

Comments

@nperez0111
Copy link

Hi @marijnh, I'm one of the maintainers of Tiptap and we've hit a regression when upgrading from [email protected] to [email protected]

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:
image
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: ProseMirror/prosemirror-model@1.21.0...1.21.1

Really appreciate your time, no rush to fix, we will just stay on 1.21.0 for now.
@nperez0111 nperez0111 mentioned this issue Jun 17, 2024
Closed
1 task
@Nantris
Copy link

Nantris commented Jun 25, 2024

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
Copy link
Member

marijnh commented Jun 26, 2024

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
Copy link
Member

marijnh commented Jun 26, 2024

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
Copy link
Member

marijnh commented Jun 26, 2024

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
Copy link
Author

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

Thank you for your quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@marijnh @nperez0111 @Nantris