fix: revert font-family escaping introduced by #4545
#5164
Conversation
Using `CSS.escape` is the wrong tool for the job here: - it is meant for CSS selectors and does not handle CSS variables properly. - you can't use `var(--title)` as a font-family because it was getting escaped to `var\(--title\)` Instead this introduces using a regex to see if we should quote the font-family. Quoting when: - font-family includes at least one number or whitespace character
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @nperez0111, reporter for the bug here. I understand not wanting to just do a true revert, but IMO TipTap should not be responsible for user error. I don't think anybody would assume that TipTap is modifying their input when it get's attached to the DOM. In fact, I'd wager most people assume what they put in is what they'll get out. A true revert in this case should be a patch, because it's fixing a commit which broke previous behavior and introduced a bug (if you can classify the existing behavior as a bug, which I would). The breaking commit did not patch a bug rather it papered over a misunderstanding of the browser style API. I don't think there's any reason to continue to support that, and instead it should be encourage that people interact with the browser correctly and fix their mistakes instead of assuming a library will. |
I totally agree with you, I was mulling it over after submitting this. This is just a weird edge case that I was trying to err towards backwards-compat. But honestly it will probably change content that shouldn't and that might be more breaking than this |
Yea, I find trying to maintain these kinds backwards compatibility patches can be sort of a time suck / painful as more edge cases start to be reported and you're expected to maintain it. |
|
Any update on this? |
|
@jonahallibone Unfortunately not, my colleague is going on vacation so likely won't see this for another couple weeks |
Changes Overview
Using
CSS.escapeis the wrong tool for the job here:var(--title)as a font-family because it was getting escaped tovar\(--title\)Instead, this reverts back to the previous behavior and forces the caller to specify an actual valid CSS font-family value.
Implementation Approach
I was notified that CSS variables don't work as the name.
CSS.escape did not look the right tool for the job.
I considered trying to quote the font-family using this stack overflow mentioned how to do it, but I ended up going back on that decision because it is just over complicated and it really should be the caller's responsibility to input valid font-family values. But you can see my previous working example in my previous commit
Testing Done
Added tests for font-family setting and new button for CSS variable
Verification Steps
Try it with: http://localhost:3000/preview/Extensions/FontFamily
Additional Notes
Checklist
feat: Implement new featureorchore(deps): Update dependencies)Related Issues
PR: #4545
The original issue could have been avoided if they quoted properly