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

prevent images and iframes reloading on patch? #117

Closed
zaceno opened this issue Feb 27, 2017 · 24 comments
Closed

prevent images and iframes reloading on patch? #117

zaceno opened this issue Feb 27, 2017 · 24 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@zaceno
Copy link
Contributor

zaceno commented Feb 27, 2017

Hi!

I have a view which renders two iframes (one on screen, and one translated off-screen). I slide the visible one off-screen, and the hidden one slides in, using CSS transitions.

There are three states to my transition:
1 when we are just showing the current slide (just one iframe),
2 when the next slide is loading off screen while the current slide is still on screen,
3 when the current slide is sliding out, and the new one is sliding in

(and it goes in a loop, so after 3, we go back to 1 for Xms, then 2 for Yms and so on)

I have it all working correctly using reducers to set the states, and a "transition" effect that calls the reducers after setTimeouts. My view sets classes on the iframe-tags to make it use a CSS transition and slide

The only problem is, that the view we render at stage 1 causes the iframe to reload (making a blinking effect, which defeats the whole idea of this "load and slide in")

My only guess as to why this is, is that the patch-function is somehow determining that the whole iframe tag needs to be updated (thereby changing the src attribute as well), when I remove the "entering" class. But it doesnt seem to blink when I add the "entering" class.

I tried to read the patch-logic to find a way to add and remove classes without touching the src attribute but couldn't figure it out. Is there a way?

Or perhaps there is a way to use the lifecycle methods to capture and prevent the iframe from being updated, and it could let me "manually" modify the class property?

@jorgebucaran
Copy link
Owner

I tried to read the patch-logic to find a way to add and remove classes without touching the src attribute but couldn't figure it out. Is there a way?

What does the src attribute has to do with adding/removing classes?

@zaceno
Copy link
Contributor Author

zaceno commented Feb 27, 2017

Ok, to explain my issue more simply (probably should have started here): I have a view that renders an iframe basically according to this:

function (model, actions) {
    var cls;
    if (model.state === 'loading') cls = 'loading'
    if (model.state === 'transitioning') cls = 'entering'
    if (model.state === 'showing') cls = 'showing'
    return h('iframe', {src: model.url, class: cls)})
}

I have noticed that when model.state goes from 'loading' to 'transitioning', there is no visible reload of the iframe. Indicating to me that hyperapp did not touch the actual dom-node, but only updated the class attribute of the domNode (as I intended)

From 'transitioning' to 'showing' there IS a visible reload of the entire iframe. Indicating either that the entire DOMNode was replaced (or at the very least, the src attribute was fiddled with.) -- which is what I would like to avoid.

When all i change is the class name, I don't want the entire dom node replaced, because that makes the browser reload the iframe, causing a visual "blink" which is exactly what I want to prevent with this preloading scheme.

@zaceno
Copy link
Contributor Author

zaceno commented Feb 27, 2017

Perhaps one reason hyperapp behaves differently in the cases of 'loading'->'transitioning' and 'transitioning'->'showing', is that in the former case, I still have two iframes in the before and after. But in the second case, I go from having two iframes (the old that slid out, and the new that slid in) to just one (currently showing)

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 27, 2017

@zaceno I'll read this more carefully when I am home. In the meantime, it could help us speed up any possible solution if you could come up with a fiddle or codepen that demonstrates the problem.

I hope that's not a lot of work 🙏

@zaceno
Copy link
Contributor Author

zaceno commented Feb 27, 2017

Absolutely! Thanksk for taking the time to consider my question.

Here is a codepen example: http://codepen.io/anon/pen/BWNmjG

Note that there are three rectangles where an iframe can be, and it is the center one I call "on screen". In an actual implementation I would have this "zoomed in" so the left and right areas would be off screen and invisible.

Anyway: notice how as iframes start to slide from the right area to the middle, they don't reload. They start sliding because I changed the class (from 'loading' to 'leaving'). Hyperapp seems smart enough to understand that when I render the same iframe with a different class name, I don't need a new iframe-element, but just need the class updated on the existing iframe.

However, once the frame reaches the middle, there is a clear reload. This is what I am trying to prevent. I want to make this smooth. At this point I render with class: 'showing', instead of 'entering'. And also stop rendering the sibling iframe. This time, it appears hyperapp thinks the iframe needs to be completely swapped out for a new one.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 1, 2017

Any ideas anyone?

I was thinking: if there's no way of avoiding the virtual-dom replacing the iframe, then perhaps this sort of problem shouldn't be managed by hyperapp at all. Perhaps the "right way" would be to capture the DOMElement in onRender, and then set up the timers and dom manipulations to do the transitions outside and parallel with hyperapp's state management.

... I'm not exactly sure how that would work, but it seem's like a hacky and messy way to go about things.

I realize I'm not explaining the issue well, but take a look at the code-pen example above. As it seems to me right now, hyperapp is unable to satisfy what I believe to be a relatively common and important use case. That should give this issue some priority, imo.

If no solution is found, I suggest making it clear in the docs, what it is (more generally) that is not supported, and providing a recommended workaround.

I would provide a PR, except I have no idea why the iframe is replaced in some calls to the patch-functions, and not other times. Someone who understands how the patch-function works needs to look closer at this.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 1, 2017

... of course as always, there's always the chance that the patch-function is fine, and it is me who made a dumb mistake in my code. But the example is there on the codepen for anyone to check.

@jorgebucaran jorgebucaran modified the milestone: 0.5.0 Mar 1, 2017
@jorgebucaran
Copy link
Owner

@zaceno Sorry, I just haven't had time to look into this issue at all because of work. I'm using my free time to work towards the 0.7.0 release and the new plugin architecture, plus docs. If I manage to get all that done before the weekend, I may be able to take a look at this then.

Leave the CodePen around too please!

@jorgebucaran
Copy link
Owner

@zaceno Why are slides a local constant inside the view?

@zaceno
Copy link
Contributor Author

zaceno commented Mar 1, 2017

@jbucaran No worries, I get that we all have day-jobs too :) I didn't mean to try to rush you or anyone, just emphasize that I think this should be a priority (because I've rambled so much about it, I feel like that point got lost).

The Code pen is anonymous, so I don't know how long they will leave it around. I should probably sign up for an account to save it permanently. I'll look into that.

The const slides = [] is just an implementation detail, I could just as well have written the view like this:

view: function (model, actions) {

    if (!model) return ''


    if (model.current && model.next) {
        return h('div', {class: 'slide-container'}, [
            h('iframe', {src: model.current, class:  'slide' + (model.state === 'transitioning' ? ' leaving' : ' showing')})
            h('iframe', {src: model.next, class: 'slide' + (model.state === 'transitioning' ? 'entering' : 'loading')})
        ])
    }


    if (model.current && !model.next) {
        return h('div', {class: 'slide-container'}, [
            h('iframe', {src: model.current, class:  'slide' + (model.state === 'transitioning' ? ' leaving' : ' showing')})
        ])
    }


    if (!model.current && model.next) {
        return h('div', {class: 'slide-container'}, [
            h('iframe', {src: model.next, class: 'slide' + (model.state === 'transitioning' ? 'entering' : 'loading')})
        ])
    }
    
  },

But that means unnecessary repetition of code. I found it more concise to just set up an array of children for the slide container, and populate that array with one or both iframes depending on state, before rendering the container.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 1, 2017

Ok here's the exact same pen, but no longer anonymous. I presume it doesn't risk being deleted:

http://codepen.io/zaceno/pen/gmPbzV

Here is a fork of the pen where I changed the view function to the implementation above (without the local slides constant)

http://codepen.io/zaceno/pen/LWGErg ...No difference in behavior observed, but that's to be expected since I did not in any way change what the view returns.

@zaceno zaceno changed the title prevent iframe reloading on patch? prevent images and iframes reloading on patch? Mar 6, 2017
@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

Here is another pen exemplifying the problem. This time using images instead of iframes.

http://codepen.io/zaceno/pen/bqwdgG

(also I locked it to using the 0.6.0 tag. The previous pens were not locked to a specific version, and might break if hyperapp changes backward compatibility in 0.7.0 for example)

Notice, however, that the reload-blink doesn't happen for all images. Only two out of three. What makes those images special, I don't know. Maybe it's just more apparent for two out of three because they are larger, server is redirecting... who knows.

@jorgebucaran jorgebucaran added bug Something isn't working and removed QUESTION labels Mar 6, 2017
@jorgebucaran
Copy link
Owner

@zaceno Thanks! Now that 0.7.0 is out, I have more time to look at this. We'll get to the bottom of it 💪.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

Sweet! Congrats on getting 0.7.0 out. Didn't realize it had just been released. Will test this on 0.7.0 then, just to see if it magically resolved itself like bugs sometimes do :)

@jorgebucaran
Copy link
Owner

@zaceno Don't think so, but miracles do happen in movies.

I still haven't published any release notes mentioning the breaking change.

Rename: reducers/effects → actions. It's all a single object now.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

@jbucaran Thanks! I tested it and it is unfortunately still a bug.

http://codepen.io/zaceno/pen/JWRdVm

Edit: also the signature for effects changed to (model, data, actions, error), I figured out :)

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

Ok I "fixed" it. I had some time to look at this, and figured out the only way to make this work is with a minor change to the patch function, which allows you to specify that a node must be inserted as a new node, and not replace or update an existing node. I made a pull request (#135) for that. It may not be the best way to implement it, but at least it works.

I have updated this codepen, to demonstrate how I would use the new "force insert" functionality.

http://codepen.io/zaceno/pen/JWRdVm

But I haven't figured out how to get my modified hyperapp into the codepen. So it doesn't actually work on code pen, yet. I'll look into that.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

I have updated the pen to use my modification in PR 135, and it now demonstrates the desired behavior

http://codepen.io/zaceno/pen/JWRdVm

@jorgebucaran
Copy link
Owner

@zaceno The result looks great! We just need to figure out a way to do this without _insert.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

Yes, I agree the _insert flag is not pretty. But I don't see any other way at the moment.

By the way, the _insert flag allows a potentially is a potentially pretty good performance boost for apps working with long lists of things. Without it, if you add something to the top of a long list, the patch function has to perform DOM operations for every item in the list. But if you add something to the top of a list with the insert flag, the only DOM operation will be to insert the new item. Nothing else, since the rest of the lists won't diff.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

... so it's kind of like an optimization flag. You don't ever have to use _insert:true (except for use cases like mine), but you can, and if used judiciously will improve performance of your app.

@zaceno
Copy link
Contributor Author

zaceno commented Mar 6, 2017

... if used un-judiciously, it should not harm your performance in any way that I can tell (but your app might not work at all as intended ;) )

@zaceno zaceno mentioned this issue Mar 7, 2017
@zaceno
Copy link
Contributor Author

zaceno commented Mar 7, 2017

Following some discussions elsewhere, I feel it's best to discard the _insert: truesolution of PR 135, and instead pin my hopes to #141 which solves this problem through the use of "keys" to dom nodes, instructing the patch function to reuse dom-nodes when it sees identical keys being used.

@jorgebucaran jorgebucaran added this to the 0.8.0 milestone Mar 11, 2017
This was referenced Mar 25, 2017
@jorgebucaran
Copy link
Owner

@zaceno Close? 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants