[Fiber] Support SVG #8490

gaearon · 2016-12-03T01:36:16Z

Based on #8417 + discussion in #8475 (comment).

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.

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

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.

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.

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-12-03T01:37:13Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+          </ErrorBoundary>
+          {
+            /*
+            * TODO: enable. Currently this leads to stack overflow


Note: this fails on master too so it's not a regression.

gaearon · 2016-12-03T01:39:32Z

Meh. This breaks other tests. :-(
I’ll look into it on Monday but is general approach viable?

sebmarkbage · 2016-12-03T04:30:09Z

src/renderers/shared/fiber/ReactFiberHostContext.js

+  function pushHostContext(fiber : Fiber) : void {
+    const parentHostContext = getHostContext();
+    const currentHostContext = getChildHostContext(parentHostContext, fiber.type);
+    if (parentHostContext === currentHostContext) {


Here's the goal: This whole thing needs to be inlinable so that this check doesn't cause extra cost for non-context changing components. Currently this function is called multiple times which makes this a tradeoff a compiler might not do. Also getHostContext is similar. And regardless this needs at least two checks, one to compare the type string and one to compare the object pointers.

sebmarkbage · 2016-12-03T04:32:03Z

src/renderers/shared/fiber/ReactFiberHostContext.js

+  }
+
+  // If we meet any portals, we'll go deeper.
+  let containerStack : Array<ContainerState> = [];


If you have a separate variable for the top value, it becomes faster to read since we do a lot more reads than we do push/pop. Same for the context.

Okay. I was doing that before but wasn't sure it's important. Is it because property access can be slow?

Since it's a numeric property it's not as bad as property access but at least it's two more indirections to figure out where the array object is and then where the content allocation is. Assuming the optimization is good. More with less optimization which is our common case.

sebmarkbage · 2016-12-07T04:08:58Z

What's the failing tests? What's remaining here before we can land and iterate?

gaearon · 2016-12-07T10:19:11Z

Shouldn't be much. I'll look today!

gaearon · 2016-12-08T18:47:57Z

Okay tests are passing now.
I still want to remove the array access but I'm not sure I understand your comments about inlining.

sebmarkbage · 2016-12-08T19:28:24Z

src/renderers/art/ReactARTFiber.js

+  getChildHostContext() {
+    return null;
+  },
+


We'll need to replicate this to the separate repo. cc @bvaughn

Roger that. ~~I'll append it to my existing sync PR.~~

Edit: Added to reactjs/react-art/pull/109 and merged.

nit: I think it would be nice for us to alpha-sort HostConfig props to make it easier to scan for differences between fiber renderers. (At least it helps when things are changing fairly often due to fiber being in-flux.)

We should make these optional so that they don't have to be added to every renderer. That way we can also do things like:

const { getChildHostContext } = config; if (getChildHostContext) { var data = getSomeData(); return getChildHostContext(data); }

That way if the compiler knows that it is always undefined, it can dead-code eliminate everything in the if statement. That's part of the reason I'd like to compile the renderer and host config together into one unit instead of making them share code.

sebmarkbage · 2016-12-08T19:29:59Z

I can see if I can restructure it for the inlining. I can just send a PR for my comments instead.

gaearon · 2016-12-08T21:11:25Z

I found a few annoying portal bugs but haven’t been able to fix them yet.
I’ll try to send a followup with fixes for them tomorrow and then look at removing array access.

* Test that SVG elements get created with the right namespace * Pass root to the renderer methods * Keep track of host instances and containers * Keep instances instead of fibers on the stack * Create text instances in begin phase * Create instance before bailing on offscreen children Otherwise, the parent gets skipped next time. We could probably create it later but this seems simpler. * Tweak magic numbers in incremental tests I don't understand why they changed but probably related to us moving some work into begin phase? * Only push newly created nodes on the parent stack 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. * Fix lint * Fix Flow 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 * Remove branches that don't seem to run anymore I'm not 100% sure this is right but I can't get tests to fail. * Be explicit about the difference between type and tag 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. * Save and restore host context when pushing and popping portals * Revert parent context and adding children in the begin phase 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 This tests the "jump" reuse code path in particular. * Record tests * Read ownerDocument from the root container instance This way createInstance() depends on the innermost container only for reading the namespace. * Track namespaces instead of creating instances early 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. * Pop child context before reading own context and clarify API 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. * Give SVG namespace to <svg> itself * Don't allocate unnecessarily when reconciling portals 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. * Add more tests for edge cases * Fix up math namespace * Maintain a separate container stack * Fix rebase mistakes * Unwind context on errors * Reset the container state when reusing the object * Add getChildHostContext() to ReactART * Record tests

gaearon added 27 commits December 3, 2016 00:33

Test that SVG elements get created with the right namespace

4f7455d

Pass root to the renderer methods

ebb80e8

Keep track of host instances and containers

ef639f6

Keep instances instead of fibers on the stack

9aaf31b

Create text instances in begin phase

d94aee1

Create instance before bailing on offscreen children

db2018c

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

Tweak magic numbers in incremental tests

2a090c1

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

Fix lint

b08ce9d

Fix Flow

44189e0

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

77fb926

Remove branches that don't seem to run anymore

ce39123

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

93de77f

Revert parent context and adding children in the begin phase

6ab662c

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

840285b

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

Record tests

c2b8888

Read ownerDocument from the root container instance

8dbd419

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

Give SVG namespace to <svg> itself

a37d5e9

Don't allocate unnecessarily when reconciling portals

1b3178b

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.

Add more tests for edge cases

84e23ca

Fix up math namespace

aa57380

Maintain a separate container stack

b31cb3d

Fix rebase mistakes

dfd37c8

Unwind context on errors

4240833

gaearon assigned sebmarkbage Dec 3, 2016

facebook-github-bot added the GH Review: review-needed label Dec 3, 2016

gaearon commented Dec 3, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Dec 3, 2016

sebmarkbage reviewed Dec 3, 2016

View reviewed changes

gaearon added 4 commits December 8, 2016 16:32

Merge master into fiber-svg-3

f80f094

Reset the container state when reusing the object

4ba9d64

Add getChildHostContext() to ReactART

f64f786

Record tests

79074b1

sebmarkbage reviewed Dec 8, 2016

View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Dec 8, 2016

sebmarkbage approved these changes Dec 8, 2016

View reviewed changes

gaearon merged commit c87ffc0 into facebook:master Dec 8, 2016

gaearon deleted the fiber-svg-3 branch December 8, 2016 21:11

gaearon mentioned this pull request Dec 9, 2016

[Fiber] Remove array indirection in host context #8544

Merged

[Fiber] Support SVG #8490

[Fiber] Support SVG #8490

Uh oh!

Conversation

gaearon commented Dec 3, 2016

Uh oh!

gaearon Dec 3, 2016

Choose a reason for hiding this comment

Uh oh!

gaearon commented Dec 3, 2016

Uh oh!

sebmarkbage Dec 3, 2016

Choose a reason for hiding this comment

Uh oh!

sebmarkbage Dec 3, 2016

Choose a reason for hiding this comment

Uh oh!

gaearon Dec 3, 2016

Choose a reason for hiding this comment

Uh oh!

sebmarkbage Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Dec 7, 2016

Uh oh!

gaearon commented Dec 7, 2016

Uh oh!

gaearon commented Dec 8, 2016

Uh oh!

sebmarkbage Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

bvaughn Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

sebmarkbage Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Dec 8, 2016

Uh oh!

gaearon commented Dec 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bvaughn Dec 8, 2016 •

edited

Loading

sebmarkbage Dec 8, 2016 •

edited

Loading