Enhance support for SVG in RichText
#40496
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stokesman
added
the
[Feature] Rich Text
This was referenced Apr 21, 2022
|
Size Change: +43 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
I am not able to do a code review but I can confirm that the svg's are displaying in the Site Editor. |
|
I wonder if this can get another look; SVGs inside buttons do not display in the block editor at the moment and this is confusing for editors. Thanks! |
|
I rebased this |
scruffian
force-pushed
the
fix/support-svg-in-rich-text
branch
from
3781dea to
2196f5e
Compare
|
@ellatrix @draganescu what do you think about this? |
draganescu
reviewed
Sep 5, 2023
packages/rich-text/src/to-dom.js
Outdated
| child.setAttribute( key, attributes[ key ] ); | ||
| } | ||
| if ( 'type' in child ) { | ||
| child = contextuallyCreate( element, child ); |
There was a problem hiding this comment.
I would leave the original code there and add one special case for if type === 'svg'. I thinlk the contextuallyCreate does not properly serve the situation: almost all the time we're just dealing with regular DOM elements.
For special types such as SVG or MathML we should create one function for each createSVG, createMathML etc.
There was a problem hiding this comment.
I tried this in 5c4be5b. I didn't make a createSVG function because I forgot about that part of your comment until now. Not sure if it's necessary at this stage but we can do that if you like.
draganescu
requested changes
Sep 5, 2023
There was a problem hiding this comment.
I think allowing SVG inline in rich text should be part of a more thorrough approach, not just allowing it.
The use case of the original issue is more in the real of blocks supporting vectors, not in the realm of rich text allowing inline SVG. Just to illustrate, applying this PR results in this weird UX:
svg-in-rich-text.mp4
Should rich text allow MathML authoring as well? Probably not. It's just like ... it's own thing. What is the best UX for interacting with vectors inside rich text? It's definitely not text :)
stokesman
force-pushed
the
fix/support-svg-in-rich-text
branch
from
2196f5e to
5c4be5b
Compare
I thought this should be harmless enough as a first step but maybe a bit more is needed. It'd probably be good if everything inside the SVG could be inert in the visual editor. The caret shouldn't enter it. |
ellatrix
reviewed
Sep 7, 2023
stokesman
force-pushed
the
fix/support-svg-in-rich-text
branch
from
5c4be5b to
376cc58
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ellatrix
reviewed
Sep 14, 2023
| start: 0, | ||
| end: 0, | ||
| formats: [ [ svg ] ], | ||
| replacements: [ path ], |
There was a problem hiding this comment.
I don't like this. I don't understand why we need all this. Why not just reuse the existing logic we have for contenteditable false, and store innerHTML?
There was a problem hiding this comment.
Do we even need to think about namespaces if we just set innerHTML?
There was a problem hiding this comment.
Thanks for having another look!
Why not just reuse the existing logic we have for contenteditable false, and store innerHTML?
Outdated reason: this approach is three years old #24582 and back then I couldn't find innerHTML used in to-dom.js so I figured it was off-limits for some reason. Now, I will gladly explore this and very much appreciate the hint.
Do we even need to think about namespaces if we just set innerHTML?
I would sure love it if that's the case 🤞.
There was a problem hiding this comment.
Do we even need to think about namespaces if we just set innerHTML?
Right, namespaces are taken care of when setting innerHTML. I've learned that SVG in a format works already as long as it's nested inside. Still, I'd like if we can remove the need for a wrapper element. I've reduced this PR to be what I think is the minimal way to support that—let formats declare a namespace.
The latest commit is largely a revert, to see the cleanest diff just view changes from all commits.
stokesman
changed the title
RichTextRichText
stokesman
force-pushed
the
fix/support-svg-in-rich-text
branch
from
376cc58 to
3bb9d57
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Enables support for SVG (or other namespace-requiring MLs like MathML) as rich text Formats and adds a unit test to cover the expected behavior.
Why?
To enable SVG in rich text formats without wrapper elements. Presently, a format can use SVG as long as it’s within an HTML wrapper element.
How?
Adds
namespacetoWPFormatallowing authors to specify the namespace a format’stagNameshould be created with. Also adds it to the return offromFormatand uses that in creating elements inappend.Testing Instructions
Snippet to run in the console to add an SVG format.
Screenshots or screencast
bird-format.mp4