Fix iframe reloading when moved

[syranide]

#858

My implementation automatically detect any "immovable" descendant nodes (iframe, object and embed) inside DOMChildrenOperations and then perform a strategy where it pre-shifts these immovable DOM nodes into place before proceeding with the regular updates. If there are no immovable DOM nodes then this has virtually zero cost.

It uses getElementsByTagName to achieve this magic, and contrary to what one might suspect, it is crazy fast. http://jsperf.com/queryselectorall-vs-getelementsbytagname/83 It's so fast that it's certainly faster than propagating and checking attributes on nodes.

You can see it in action here:
http://dev.cetrez.com/jsx/2/index2.html (no longer online)

This PR solves the iframe issue as far as is physically possible, stopping only short of being able to have iframes swap places (which is impossible). So iframes are actually useful now in React, at (what should be) virtually zero cost!

---

I should perhaps also mention that I would personally find it very useful to have this fixed (be it this PR or any other), as I require iframe support for the editor at work. It's fixed in place, but there may be changes in the structure outside of the editor that would cause it to reload. Also, most contentEditable editors use iframes to isolate themselves, so this should find some quite universal usefulness.

---

PS. I tried benchmarking it with react-bench, but it's beyond useless as I can get up to ~30% consistent perf diff, while benchmarking with the same React-library.

---

   raw     gz Compared to master @ 3d47177596e9782de36a5cd7065e01c69897b2bb
     =      = build/JSXTransformer.js
 +6240  +1561 build/react-with-addons.js
  +771   +268 build/react-with-addons.min.js
 +6240  +1570 build/react.js
  +771   +275 build/react.min.js

[syranide]

cc @benjamn @vjeux

[syranide]

Updated the PR to use getElementsByTagName instead of propagating attributes, it's now a universal solution, it even handles the case where React didn't create the iframe. And still it's certainly faster than checking for the existence of an attribute, have a look yourselves: http://jsperf.com/queryselectorall-vs-getelementsbytagname/82

[syranide]

Thanks to @pieter-vanderwerff for noticing that object and embed also reload if they are detached/moved. Which means that Flash/SWF is also affected by this issue.

[syranide]

I investigated further and found that based on the assumptions on ReactMultiChild it could be "vastly" simplified and improved. It now has very strict assumptions on ReactMultiChild, but for every bit of the assumptions we drop, the more expensive this gets, and now it's super fast. I don't see why the current assumptions on ReactMultiChild would be an issue in the foreseeable future (or at least easily patched)

[benjamn]

This would be really great to have! Can you add some tests?

[sophiebits]

know if [0] is any faster than .length?

[syranide]

@spicyj I had tested for 0 in which apparently is slower (but tested now, and apparently it isn't?)... anyway, it depends on the browser, but [0] is faster in the slower browsers it seems, so that's the way to go (not that it really matters anyway with 250k call per second on IE8 even). Good call!

http://jsperf.com/queryselectorall-vs-getelementsbytagname/83

[syranide]

@benjamn Definitely, but I'm "new" to testing and I have no idea how I would test this. I could add some "torture test" that makes sure DOM updates never break, that shouldn't be hard. But how would I test "immovable nodes", is it actually feasible to insert an iframe an inject something that I can test for after the test?

[benjamn]

You could have the <iframe> load a page that increments a data-reload-count attribute of window.frameElement every time the frame reloads, then assert stuff about that attribute.

[syranide]

@benjamn I chose the simpler route of just adding a variable to window and making sure that it stays there until after. Anyway, I've added a super crappily coded test file right now, I'll add one or two more tests unless you say otherwise and clean it up (any feedback would be great, new ground for me :)).

Right now it basically generates a random number of nodes, some with keys, some without. Then permutates them randomly a number of times (including adding and removing) and makes sure that the DOM is exactly what it expects it to be after every render. Then the same with an iframe, and I intend to add at least another one with 2 iframes that may never cross to make sure that works as well.

[syranide]

@benjamn I've added 4 tests now, all based on the same strategy, mutating a randomly generated set of items based on keys and indices, X number of times and verifying the output at each step. Each test represent a progressively more complex scenario.

The last one also tests that the environment behaves as it should (i.e. that immovable objects reload when detached), although it's possible that some browser doesn't have this behavior (test is still useful by itself, but will need to disable the check then).

It passes travis-ci which you guys have hooked up to saucelabs right? So seems fine then.

[syranide]

Thanks to @pieter-vanderwerff for pointing out that audio and video are also affected. This was a bigger issue than I initiall thought.

If anyone can benchmark this PR as-is and with immovableTagNames set to only one tag (while including that tag as a node in the markup), that would be greatly appreciated. It currently always tests all 5 tag names, I'm pondering whether it is worth to make it smarter so that it narrows down the list to only the tags that the parent was found to have, or if it's so ridiculously fast that it doesn't really matter.

[syranide]

@benjamn I rebased it, but my tests now fails because iframe.contentWindow is no longer defined. Has something changed for the grunt test fake browser environment?

[sophiebits]

Yeah, renderIntoDocument was changed to not attach things to the document any more to make cleanup easier.

[syranide]

@spicyj Ah, that makes sense, best solution for this is perhaps just for me to attach it in my tests I guess? As it apparently was an issue before, is there something I should be "aware" of?

[sophiebits]

I don't think so, just remove it when you're done. See ce95c3d.

[syranide]

@spicyj Many thanks, so the issue was way simpler than I thought. :)

I've hacked a ReactTestUtils.renderAttachedIntoDocument which has the old behavior, and my tests also clean up after themselves. Input on how to do this properly is welcome.

[syranide]

@zpao I will be needing this fix for our service very soon, I have no problem at all proceeding with a custom branch. But I just want to make sure I don't head down a conflicting path/end up with the custom branch indefinitely. Is this fix something you are considering taking (for some future milestone) or are you looking towards an alternative solution/no solution?

[sophiebits]

Do you want a

invariant(childNode.previousSibling === beforeNode);

here?

[syranide]

@spicyj Not sure if I follow, seeing as that is the exact condition for the while-loop, it could not possibly be false and trigger the invariant?

EDIT: Nevermind, I see your point, yeah that sounds reasonable.

[sophiebits]

Now I'm having trouble making sense of what I meant but I'm happy if you understand. :)

[syranide]

I actually take it back, haha. It temporarily made sense to me in that it would validate that the result of the move was as intended, but if it isn't then it never leaves the loop anyway. So my original reaction stands it seems, if the invariant is reached it would be guaranteed to be true. Unless you had something else in-mind (other than putting it last in the function).

[sophiebits]

Yeah, I guess let's just have protection against the case that childNode somehow comes after index already, which will cause an infinite loop the way you have it written.

(Side note: I found beforeNode to be a confusing name; I interpreted it as "insert childNode before beforeNode", not "beforeNode should appear before childNode". Not sure what would be clearer though.)

[syranide]

Yeah that makes sense, invariant(childNode.nextSibling != null) should be sufficient and simple I think. As for the variable, tricky yes, my current naming was intended to reflect the idea that it's just a different implementation of the same behavior as insertBefore (insert X before node Y), perhaps index should rather be renamed to beforeIndex which could add some sense to beforeNode (which is just locally caching the value), possibly?

[sophiebits]

I think beforeIndex has the same naming problem.

[syranide]

Yeah it kind of does, insertBeforeIndex would be the spontaneous solution. But to me that eludes to it being a function and not an index. https://developer.mozilla.org/en-US/docs/Web/API/Node.insertBefore refers to it as referenceElement, which kind of makes sense and points to my function being badly named, moveChildToBefore moveChildBefore moveBefore ... ?

PS. It's also kind of weird that the arguments refers to elements but the descriptions refers to nodes in that URL.

[sophiebits]

Maybe moveChildBefore and referenceElement (or referenceNode).

[syranide]

Sounds fine to me and I think we've been using node everywhere else, but perhaps I've just assumed that. I'll take a look and settle on the naming based on what we use elsewhere.

[syranide]

Fixed docstring + invariant and rebased to fix conflict.

[sebmarkbage]

There are more immovables. The currently focused text field, the current mouse target (avoids an issue with mousedown+mouseup not triggering click if it's moved).

This is generally good but I'm not sure about the strategy around hasImmovableDescendants. I think we can do better. E.g. record keeping inside of React (not including dangerousInnerHTML).

[syranide]

This is generally good but I'm not sure about the strategy around hasImmovableDescendants. I think we can do better. E.g. record keeping inside of React (not including dangerousInnerHTML).

We can, but it was rather non-trivial as it only applies to ReactDOMComponent (and also adds a potentially depth-linear cost for each tree mutation)... the biggest reason why I chose to go with getElementsByTag is because it also extends the courtesy to non-React markup. Is that preferable? Perhaps not if we want "focused field"/etc to piggyback on the same "feature".

But I agree it does feel a bit dirty to use getElementsByTag, but then again it's also a fix for HTML-weirdness and not intrinsically useful to React. If you feel keeping a record is better then I'm game.

[syranide]

There are more immovables. The currently focused text field, the current mouse target (avoids an issue with mousedown+mouseup not triggering click if it's moved).

That's also rather tricky, which is more important if immovables cross paths? I would say:

iframe/video/audio/etc > focused text field <> current mouse target

Which means we should probably still have a decent fix for the last two.

[mridgway]

Are there any plans to introduce this or has this been scrapped?

[sophiebits]

There hasn't been much interest in it and it makes the core more complicated. Do you need it?

[gaearon]

Thank you for your work on this.

Technically it’s a problem we don’t have a good solution for, but it significantly complicates the logic for what seems to be a relatively low-priority use case.

This is why I’m closing this pull request and adding a new “Resolution: Unsolved” label. We can revisit it later if there is enough interest in seeing this implemented and later supporting that implementation.