[Fiber] Support SVG by adding a container stack #8417

gaearon · 2016-11-24T21:06:55Z

I added a test for SVG. For it to pass, we need to keep track of the closest host container (e.g. svg or foreignobject) on the stack. Since we need the stack anyway, I also used it to append initial children in the begin phase instead of the complete phase. I am using two separate stacks: one for "container" nodes (such as svg and foreignobject) and another one for any "new" nodes so that we can add children to them immediately. Changed to use a single container stack per review comments. I can do the parent stack in a follow up.

gaearon · 2016-11-25T19:23:35Z

@sebmarkbage This is sort of ready for review. Portals might be broken but I have a todo for that.

gaearon · 2016-11-25T21:54:09Z

Should work with portals too now.

gaearon · 2016-11-25T22:01:24Z

gaearon · 2016-11-25T23:17:58Z

Note that I haven't tried unifying this with portals yet. I'm not sure if it's helpful or not.

sebmarkbage · 2016-11-25T23:41:19Z

Any way we can split the container piece from the parent piece (that creates in beginWork)? The second one is a hot path and needs further evaluation.

gaearon · 2016-11-26T01:12:52Z

Removed the parent stack and moved initial insertions back into complete phase.

We still need to create the instances in the begin phase so that we have access to root container instance in ReactDOMFiberComponent.createElement() and can determine the namespace and owner document.

gaearon · 2016-11-26T01:13:58Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -453,8 +497,15 @@ module.exports = function<T, P, I, TI, C>(
      case HostComponent:
        return updateHostComponent(current, workInProgress);
      case HostText:
-        // Nothing to do here. This is terminal. We'll do the completion step
-        // immediately after.
+        const newText = workInProgress.pendingProps;


Technically didn't have to move this one but did this for consistency.
Can move it back too.

sebmarkbage · 2016-11-28T16:45:07Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    // and use tagName (which is upper case for HTML DOM elements). Or we could
+    // let the renderer "normalize" the fiber type so we don't have to read
+    // the type from DOM. However we need to remember SVG is case-sensitive.
+    var tag = domElement.tagName.toLowerCase();


This is likely going to be an issue to do for every update. We might want to consider just banning non-lowercase uses instead.

I can omit this change if it's better to discuss separately.

Also, we already doing this in commitUpdate() before my PR. So I don't see why doing this during mounting is worse. (We should fix both?)

sebmarkbage · 2016-11-28T18:19:37Z

The creation order is only needed if we want to read the namespace from the DOM. We don't have to design it that way. We can pass a flag or something instead. It might be more efficient than touching DOM accessor methods anyway and doesn't lock us in to this creation order. (Even though we might want that anyway.)

Will review further today. The reason it is taking time is that I want to a deep review and consideration because this change is a compromise only for the purpose of letting users avoid switching renderers in the rare cases you switch modes. It is also very specific only to the DOM renderer. Anything that adds perf overhead or locks us into a particular creation phase seems like a big tradeoff.

sebmarkbage · 2016-11-28T19:53:10Z

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

@@ -517,22 +517,22 @@ describe('ReactIncrementalSideEffects', () => {
      );
    }
    ReactNoop.render(<Foo tick={0} idx={0} />);
-    ReactNoop.flushDeferredPri(40 + 25);
+    ReactNoop.flushDeferredPri(40 + 20);


Why are all these taking more cycles?

Less cycles, isn't it? I'm not sure. Should I look into this?

Otherwise, the parent gets skipped next time. We could probably create it later but this seems simpler.

I don't understand why they changed but probably related to us moving some work into begin phase?

Previously I was pushing nodes on the parent stack regardless of whether they were already in current or not. As a result, insertions during updates were duplicated, and nodes were added to existing parents before commit phase. Luckily we have a test that caught that.

I had to wrap HostContext API into a closure so that it's parameterizeable with I and C.

I'm not 100% sure this is right but I can't get tests to fail.

I was confused by th HACK comment so I learned how DOM and SVG work with casing and tried to write a more descriptive comment. It also seems like passing fiber.type into finalizeInitialChildren() is a potential problem because DOM code assumes tag is lowercase. So I added a similar "hack" to finalizeInitialChildren() that is identical to the one we have prepareUpdate() so if we fix them later, we fix both.

We can address this later separately as it is a more hot path. This doesn't affect correctness of SVG container behavior.

This tests the "jump" reuse code path in particular.

gaearon · 2016-11-29T13:57:52Z

letting users avoid switching renderers in the rare cases you switch modes

I don’t understand this part. What case are you talking about?

This way createInstance() depends on the innermost container only for reading the namespace.

gaearon · 2016-11-29T15:46:39Z

I’m looking into tracking namespaces separately so that we don’t need to change when we create.

While we might want to create instance in the begin phase, we shouldn't let DOM guide reconciler design. Instead, we are adding a new concept of "host context". In case of ReactDOMFiber, it's just the current namespace. We are keeping a stack of host context values, ignoring those that are referentially equal. The renderer receives the parent context and type, and can return a new context.

gaearon · 2016-11-29T16:31:22Z

@sebmarkbage Should be ready for another review. Now only tracking the namespace, instances are created like before.

sebmarkbage · 2016-11-29T16:48:56Z

letting users avoid switching renderers in the rare cases you switch modes

I don’t understand this part. What case are you talking about?

I mean that we could also just make people write:

return <div>{ReactDOM.createSVGPortal(
  <svg>
     <path />
  </svg>
)}</div>;

I'm merely making the point that this whole thing is mostly just a convenience. Any additional cost in file size and performance needs to take that tradeoff into account.

But it seems like we can get it pretty small.

sebmarkbage · 2016-11-29T17:05:45Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+      }
+    }
+    if (parentNamespace === SVG_NAMESPACE && type === 'foreignObject') {
+      return null;


I believe the foreignObject itself should be the SVG namespace but then get next namespace underneath it should be HTML.

Can you add an assertion that the <svg /> tag itself has the SVG namespace?

Shame on me!

It wasn't quite clear from the API which context was being returned by the renderer. Changed the API to specifically ask for child context, and thus to pop it before getting the current context. This fixes the case with <foreignObject> to which I intended to give SVG namespace.

sebmarkbage · 2016-11-29T17:54:49Z

This gave me an idea. Feel free to ignore if you're tired of exploring my whims :) and I can take a look myself.

What if we tagged a new type of work HostContextComponent or something. If the type is a context provider, it gets tagged as such. That way we don't need to keep the stack of fibers around to know if we need to pop. HostContextComponent always pushes and pops. It also eliminates any extra work from the normal host component.
What if just assumed that when you pop from MathML or SVG, you pop into HTML and when you pop from HTML you pop to SVG? That way we don't need a stack at all.

gaearon · 2016-11-29T18:05:00Z

I’ll try that. 😄

We create stacks lazily so that if portal doesn't contain <svg>s, we don't need to allocate. We also reuse the same object for portal host context state instead of creating a new one every time.

gaearon · 2016-11-30T21:12:46Z

Closing as we decided to go with a cheaper approach.

gaearon assigned sebmarkbage Nov 24, 2016

facebook-github-bot added GH Review: review-needed CLA Signed labels Nov 24, 2016

gaearon force-pushed the fiber-svg branch 2 times, most recently from cb3e491 to a755f74 Compare November 25, 2016 17:42

gaearon changed the title ~~[Fiber][WIP] Support SVG and append children in the begin phase~~ [Fiber] Append children in the begin phase and support SVG Nov 25, 2016

gaearon force-pushed the fiber-svg branch from 66d2245 to 34f8462 Compare November 25, 2016 21:53

gaearon changed the title ~~[Fiber] Append children in the begin phase and support SVG~~ [Fiber] Support SVG by adding a container stack Nov 26, 2016

gaearon commented Nov 26, 2016

View reviewed changes

sebmarkbage mentioned this pull request Nov 26, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

sebmarkbage reviewed Nov 28, 2016

View reviewed changes

gaearon added 11 commits November 29, 2016 13:27

Test that SVG elements get created with the right namespace

18a9869

Pass root to the renderer methods

b8b2638

Keep track of host instances and containers

a450a08

Keep instances instead of fibers on the stack

3bc1b07

Create text instances in begin phase

21e7b04

Create instance before bailing on offscreen children

a810979

Otherwise, the parent gets skipped next time. We could probably create it later but this seems simpler.

Tweak magic numbers in incremental tests

62b6241

I don't understand why they changed but probably related to us moving some work into begin phase?

Fix lint

2c5caa0

Fix Flow

ae91e59

I had to wrap HostContext API into a closure so that it's parameterizeable with I and C.

Use the same destructuring style in scheduler as everywhere else

063199e

gaearon added 6 commits November 29, 2016 13:42

Remove branches that don't seem to run anymore

00c75b2

I'm not 100% sure this is right but I can't get tests to fail.

Save and restore host context when pushing and popping portals

e6dd706

Revert parent context and adding children in the begin phase

0d41720

We can address this later separately as it is a more hot path. This doesn't affect correctness of SVG container behavior.

Add a test for SVG updates

dd93f0c

This tests the "jump" reuse code path in particular.

Record tests

94cd7db

gaearon force-pushed the fiber-svg branch from cb36047 to 94cd7db Compare November 29, 2016 13:51

Read ownerDocument from the root container instance

2099a2a

This way createInstance() depends on the innermost container only for reading the namespace.

gaearon force-pushed the fiber-svg branch from e7d27fe to c08a24a Compare November 29, 2016 16:28

sebmarkbage reviewed Nov 29, 2016

View reviewed changes

Give SVG namespace to <svg> itself

5ee8dcf

Don't allocate unnecessarily when reconciling portals

5f4f36d

We create stacks lazily so that if portal doesn't contain <svg>s, we don't need to allocate. We also reuse the same object for portal host context state instead of creating a new one every time.

gaearon closed this Nov 30, 2016

gaearon deleted the fiber-svg branch December 1, 2016 17:11

gaearon mentioned this pull request Dec 1, 2016

[Fiber] Support SVG #8475

Closed

gaearon restored the fiber-svg branch December 2, 2016 23:34

gaearon mentioned this pull request Dec 3, 2016

[Fiber] Support SVG #8490

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Support SVG by adding a container stack #8417

[Fiber] Support SVG by adding a container stack #8417

gaearon commented Nov 24, 2016 •

edited

Loading

gaearon commented Nov 25, 2016 •

edited

Loading

gaearon commented Nov 25, 2016

gaearon commented Nov 25, 2016

gaearon commented Nov 25, 2016

sebmarkbage commented Nov 25, 2016

gaearon commented Nov 26, 2016

gaearon Nov 26, 2016

sebmarkbage Nov 28, 2016

gaearon Nov 28, 2016

gaearon Nov 29, 2016

sebmarkbage commented Nov 28, 2016

sebmarkbage Nov 28, 2016

gaearon Nov 28, 2016

gaearon commented Nov 29, 2016

gaearon commented Nov 29, 2016

gaearon commented Nov 29, 2016

sebmarkbage commented Nov 29, 2016

sebmarkbage Nov 29, 2016

gaearon Nov 29, 2016

sebmarkbage Nov 29, 2016

gaearon Nov 29, 2016

sebmarkbage commented Nov 29, 2016 •

edited

Loading

gaearon commented Nov 29, 2016

gaearon commented Nov 30, 2016

[Fiber] Support SVG by adding a container stack #8417

[Fiber] Support SVG by adding a container stack #8417

Conversation

gaearon commented Nov 24, 2016 • edited Loading

gaearon commented Nov 25, 2016 • edited Loading

gaearon commented Nov 25, 2016

gaearon commented Nov 25, 2016

gaearon commented Nov 25, 2016

sebmarkbage commented Nov 25, 2016

gaearon commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Nov 29, 2016

gaearon commented Nov 29, 2016

gaearon commented Nov 29, 2016

sebmarkbage commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Nov 29, 2016 • edited Loading

gaearon commented Nov 29, 2016

gaearon commented Nov 30, 2016

gaearon commented Nov 24, 2016 •

edited

Loading

gaearon commented Nov 25, 2016 •

edited

Loading

sebmarkbage commented Nov 29, 2016 •

edited

Loading