Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Iframe's reinitialise when moving down within a group of components #858

Closed
pieterv opened this issue Jan 9, 2014 · 22 comments
Closed

Iframe's reinitialise when moving down within a group of components #858

pieterv opened this issue Jan 9, 2014 · 22 comments

Comments

@pieterv
Copy link
Member

pieterv commented Jan 9, 2014

When moving a component with an iframe down within a group of components it reinitialises the iframe. The iframe only reinitialises when moving down, moving upward seems to work fine.

Example: http://jsfiddle.net/pieterv/fnkLf/

@vjeux
Copy link
Contributor

vjeux commented Jan 9, 2014

I can't see anything wrong with your code from a 2 min inspection. You are properly using key and the reconciliation should work fine. cc @jordwalke

@sophiebits
Copy link
Collaborator

Curious. I'll take a look later.

@benjamn
Copy link
Contributor

benjamn commented Jan 10, 2014

<iframe>s reload whenever they are reattached to the DOM. So it might not be reasonable to expect this to work. Consider two frames that need to trade places. One has to get removed from the DOM and reattached, no matter what you do. Hoping to get away with lucky re-renderings is a recipe for fragile code.

@pieterv
Copy link
Member Author

pieterv commented Jan 10, 2014

@benjamn yes that makes sense thinking about the DOM operations required to reorder the elements, it was just because of reacts preference to move things downward as @plievone pointed out that the up operation was working without reinitialising.

@sophiebits
Copy link
Collaborator

You might think that you could just call insertBefore or appendChild without ever removing the elements from the DOM but at least in Chrome, that still makes one of the iframes reload. Maybe that means we can't fix this.

@syranide
Copy link
Contributor

Yeah, just did some manual tests as well, this actually appears to be impossible to workaround in the DOM, and some internet searching suggests the same.

The only universal solution I see is to somehow be able to mark a component as mustNotMove which will apply all the way up to the root, which would prevent any move operation on those nodes and the siblings would be used for reordering instead. It would work for all cases except for when two mustNotMove components changes sides.

However, I'm curious how much of an issue this actually is today? Does this make iframe in React virtually pointless because of conditionals further up in parents, or is it really only an issue when explicitly moving components with iframes around?

@benjamn
Copy link
Contributor

benjamn commented Jan 10, 2014

This is one of the most regrettable misfeatures of <iframe>s, in my opinion. And, yes, it makes them pretty much useless in situations where DOM manipulations are abstracted out of your control, as they are in React.

@plievone
Copy link
Contributor

To prevent reloading of <iframe>s, one can keep each of them in separate divs, somewhere safe from DOM manipulations, and move/scale them with absolute positioning in sync with some placeholder elements. :)

@syranide
Copy link
Contributor

@plievone Yeah, but the only way to keep them perfectly in sync would be to poll every frame, and this would cause lag on IE8 as it doesn't support requestAnimationFrame. Also, you'll have issues with z-index.

@syranide
Copy link
Contributor

@spicyj @benjamn I had a look at React and it is possible to solve this as good as it gets (i.e. an iframe may never move to the other side of another iframe) by applying a immovable-flag up the hierarchy for iframes. When React wants to move such a component it should instead shuffle the siblings over to the other side of it instead of moving the node itself. This should have virtually zero cost when no immovable component is moved.

Been trying to wrap my head around ReactMultiChild and DOMChildrenOperations for a bit, it seems like it should be possible. But my head is apparently not up for the task right now. :)

@pieterv
Copy link
Member Author

pieterv commented Jan 11, 2014

Just for reference i got an implementation going as @plievone suggested by absolute positioning the iframe into place, http://jsfiddle.net/pieterv/fnkLf/6/, but as expected it seems to be pretty fragile code, you would really need to have the siblings as set height elements to get away with not polling their position on every frame.

@syranide
Copy link
Contributor

@pieter-vanderwerff FYI, I have a proof-of-concept for a React PR working right now, just fixing the last issues. With it, iframes will work as they should as long as two iframe containing components never switch sides. Meaning, it should basically solve most common use-cases, except for moving say comments with iframes up/down in a list, but static iframes and blog style pages will work perfectly.

It seems like it's a relatively light-weight fix too so hopefully they accept it.

@pieterv
Copy link
Member Author

pieterv commented Jan 11, 2014

@syranide awesome, i look forward to seeing it!

@syranide
Copy link
Contributor

@pieter-vanderwerff I dare you to check my updated PR, I found something way way better than attributes to use! :D

@pixelcort
Copy link

Would using CSS Flexbox's box-ordinal-group to rearrange iframes help? That way you don't need to remove either iframe from the DOM. Of course, this would only work on newer browsers, and would be something one would write themselves on top of React.

@syranide
Copy link
Contributor

@pixelcort Probably not, the issue is specifically when they (or any of their ancestores) are detached from the DOM, which always happens if you try to move them in any way it seems.

benjamn referenced this issue in facebookarchive/react-meteor Jun 3, 2015
This effectively removes the requirement of providing a fixed root
container for React.render to use!
@Prinzhorn
Copy link

I have a related issue with iframes which needs a different fix. I have a component using the Twitter API to embed a tweet in componentDidMount. This works great, until the component is rearranged by react (it's part of a list and has a key). When this happens, the iframe becomes completely empty. It can't just reload the src because the Twitter API does JavaScript magic directly with the iframe. I would need to clean up the iframe and load a new one using the API when this happens.

So the problem here is that the component has no way of knowing that this just happened. There is no lifecycle method telling it about the DOM manipulation (sth. like componentDidRemount?). Since the component also uses the PureRenderMixin literally nothing happens to it when it gets moved in the DOM. It doesn't seem possible for me to fix this outside of react.

Edit: I found a workaround for my particular use-case to at least have a chance to reload the tweet. The iframe will fire an onload event after it has been reloaded (even though the twitter generated iframe doesn't have a src).

@aduth
Copy link

aduth commented Apr 28, 2017

Would using CSS Flexbox's box-ordinal-group to rearrange iframes help? That way you don't need to remove either iframe from the DOM. Of course, this would only work on newer browsers, and would be something one would write themselves on top of React.

I was able to find some success in using Flexbox's order property:

More a hack than a general solution really, but seems to be working well enough for my purposes.

The trick being that in the "After" example, the order of elements in the DOM never truly changes, just visually reordered with the order styling.

@afercia
Copy link

afercia commented May 22, 2017

5.4.1. Reordering and Accessibility
https://www.w3.org/TR/css-flexbox-1/#order-accessibility

@gaearon
Copy link
Collaborator

gaearon commented Oct 1, 2017

This problem is both rare and hard enough that it is unlikely we’ll be spending our time on this.

The complexity and increase in the code size this will introduce is probably not worth the effort.
So I’ll close this issue as unsolved.

I’m open to reviving it if somebody comes up with a simple solution to this, but I just don’t see it.

@flackeryy
Copy link

18.07.2022 this issue still here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests