Skip to content

[Fiber] Support SVG #8475

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 7 commits into from
Closed

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 1, 2016

Replaces #8417.

Instead of keeping a stack, we're notifying the renderer when the work begins and ends.
This lets DOM renderer avoid the stack because namespace logic is not complex.

This is not done yet (missing portals and unwinding on error handling) but I want to get early feedback on the approach.

Instead of keeping a stack, we let the renderer keep track of the current namespace.
@sebmarkbage
Copy link
Collaborator

Like we talked about, this needs to support nested SVG tags.

Another case is mtext. We don't actually support this today but I believe that in MathML, mtext can be used to nest anything. The spec is a bit ambivalent on this. It is also not well supported by browsers so we can probably ignore this use case.

@@ -268,6 +272,7 @@ module.exports = function<T, P, I, TI, C>(
// Abort and don't process children yet.
return null;
} else {
pushHostContext(workInProgress.type);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we pass props too? We have cases like props.is where props determine the type. I could imagine this being useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pass when we need them?

@gaearon gaearon force-pushed the svg-fiber-take-two branch from 4572fd0 to 50bfa83 Compare December 1, 2016 21:10
We allocate an array with one number, and increment it on any nested <svg>.

We add a new counter to the array when we encounter a <foreignObject>.

The counter lets us avoid a stack for nested <svg>s in the general case.
@gaearon gaearon force-pushed the svg-fiber-take-two branch from f8e1076 to 530eb7a Compare December 2, 2016 16:16
The mutable state we introduced above becomes irrelevant whenever we push a portal because a portal represents a different DOM tree. We store a snapshot of this state and restore it when the portal is popped.
@sebmarkbage
Copy link
Collaborator

Thanks for exploring this option. This is getting pretty complicated as well.

How do you feel about reverting back to the one that uses a stack for the host namespace and document? Then we can unify that stack with the context stack later on like we talked about.

I feel like a generic stack solution seems pretty useful for a lot of things.

@gaearon gaearon changed the title [Fiber][WIP] Support SVG [Fiber] Support SVG Dec 2, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Dec 2, 2016

Yeah to be honest I have lost track of what's going on here completely. :-)
On the positive side I have new tests.

Do you want to get back to #8417 as it was, or do you have some changes to it in mind?

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Dec 2, 2016

We can go back to #8417 but let's use a stack for the portal context (container) instead of storing it on the portal. Then we can iterate from there to unify the stacks.

@gaearon gaearon closed this Dec 2, 2016
@gaearon gaearon deleted the svg-fiber-take-two branch December 2, 2016 23:32
@gaearon gaearon mentioned this pull request Dec 3, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants