IE8 - Skrollr gets caught in infinite reflow loop when refreshing dynamic elements

ourmaninamsterdam commented 11 years ago

Hi,

I've come across a strange occurrence of the IE8 window resize bug causing an infinite reflow loop when dynamically adding elements to a page when calling the refresh() method.

Here's my test case. Add a console.log to to _reflow function to see the event retriggering.

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>skrollr - parallax scrolling for the masses</title>
    <style type="text/css">
        .bg{
            background: #ff0000; 
            height: 300px;
            width: 300px;
        }
        button{
            position: fixed;
            top:0;
            right: 0;
        }
    </style>
</head>

<body>

    <div id="mydiv" data-0="top: 0px;" data-100="top: 100px;"></div>

    <script type="text/javascript" src="src/skrollr.js"></script>

    <!--[if lt IE 9]>
    <script type="text/javascript" src="dist/skrollr.ie.min.js"></script>
    <![endif]-->

    <button id="addelembtn">Add elements</button>

    <script type="text/javascript">
    var div = '<div class="bg" data-200="left:40px;" data-300="left: 300px;">Test div</div>';
    var s = skrollr.init({
        edgeStrategy: 'set'
    });

    document.querySelector('#addelembtn').onclick = addElems;

    function addElems(){
        document.querySelector('#mydiv').innerHTML += div;
        s.refresh();
    }

    </script>
</body>
</html>

Prinzhorn commented 11 years ago

Interesting, I need to take a closer look at it.

For now, does it work as a workaround to only refresh the elements that need to be refreshed? E.g. s.refresh(document.querySelectorAll('#mydiv *'));

ourmaninamsterdam commented 11 years ago

The reflow still happens. It's an interesting one.

On 23 July 2013 15:34, Alexander Prinzhorn notifications@github.com wrote:

Interesting, I need to take a closer look at it.

For now, does it work as a workaround to only refresh the elements that need to be refreshed? E.g. s.refresh(document.querySelectorAll('#mydiv *'));

— Reply to this email directly or view it on GitHubhttps://github.com/Prinzhorn/skrollr/issues/271#issuecomment-21418151 .

Prinzhorn commented 11 years ago

Thanks, I just learned about the IE8 bug through this issue (more: http://www.piotrovski.com/2012/10/resize-event-on-ie8-and-other-browsers.html).

Now the endless loop is obviously caused by https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L277 which causes https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L1273 which in turn generates a resize event in IE8 (I guess, I don't have access to a Windows VM atm).

Could you confirm this by setting the forceHeight option to false?

So we should probably throttle the resize event (which would be nice in general, because we don't need to do a ton of caluclations while the user is resizing the browser). I don't know how fast IE8 triggers the event, maybe it's enough do set a flag on resize (instead of calling _reflow right away) and have it processed inside the render loop (~16ms).

ourmaninamsterdam commented 11 years ago

Can confirm that enabling forceHeight: false stops the reflow from happening. If I get some time I might be able to measure the trigger time.

I'm running it using IE8 mode in IE10 on a Windows 8 VirtualBox VM.

Prinzhorn commented 11 years ago

If I get some time I might be able to measure the trigger time.

I guess it's fired right when changing the body style, but then again it's IE :-D.

Here's a jsbin to get you started: http://jsbin.com/ecidis/1/edit

ourmaninamsterdam commented 11 years ago

You are correct in that it triggers on the <body> height style update.

I finally managed to get the timestamp before the browser crashed. Not sure if they're much help but here are the trigger timestamps.

1: 1374870621221 2: 1374870621227 3: 1374870621251 4: 1374870621266 5: 1374870621277 6: 1374870621286 7: 1374870621299 8: 1374870621313

Prinzhorn commented 11 years ago

Great. But the timestamps themselves don't help much without reference. I need the difference (timestamp when the resize event fires minus timestamp when the style was changed). If that's close to 0, that'd be great.

But never mind, I'm gonna change the code the way I guess it's correct and will post a link to a dev version you can drop in and test with.

ourmaninamsterdam commented 11 years ago

Sorry man, got a deadline today so haven't had time to fully look at this, hence my fuzzy thinking with the timestamps.

Look forward to seeing the fix as we are using this on a production site going live in a couple of weeks.

On 26 July 2013 15:04, Alexander Prinzhorn notifications@github.com wrote:

Great. But the timestamps themselves don't help much without reference. I need to difference (timestamp when the resize event fires _minus_timestamp when the style was changed). If that's close to 0, that'd be great.

But never mind, I'm gonna change the code the way I guess it's correct and will post a link to a dev version you can drop in and test with.

— Reply to this email directly or view it on GitHubhttps://github.com/Prinzhorn/skrollr/issues/271#issuecomment-21622548 .

Prinzhorn commented 11 years ago

Please try the issue_271 branch https://github.com/Prinzhorn/skrollr/blob/issue_271/src/skrollr.js

For testing you can hotlink this file https://rawgithub.com/Prinzhorn/skrollr/issue_271/src/skrollr.js

Edit: Thinking about it, I guess it won't solve it completely, only slow down the endless loop.

ourmaninamsterdam commented 11 years ago

Tested. Still freezes I'm afraid, but not immediately, as before.

RobertinoValue commented 11 years ago

After reading the page explaining the IE8 bug, at the first link in the comment by @Prinzhorn above : https://github.com/Prinzhorn/skrollr/issues/271#issuecomment-21444195 the following test might makes things clearer :

If event data is available in resize handler _reflow, you could check the event.target and ignore every other target than document.body.

But first, to determine if event.target really also contains other targets than document.body, add to _reflow :

console.log(event.currentTarget === window); //Should be true. Where the resize event was registered.
console.log(this === window); //Should be true. Where the resize event is processed.
console.log(event.target === document.body);  //true when the body was resized (the target of the resize event). false if anything else on the page triggered the resize.

Apparently your IE8 Dev Bar / Developer Tools need to be opened for console.log to work in IE8. See the first comment to the question here : http://stackoverflow.com/questions/690251/what-happened-to-console-log-in-ie8

Prinzhorn commented 11 years ago

If someone could confirm that e.target is not always window, then this would be a great (one-liner) fix.

So the only thing I'm interested in is if e.target === window is sometimes false inside the resize handler.

ourmaninamsterdam commented 11 years ago

Have logged the event in the resize handler in the intermediate function within _addEvent and e.target is always null. Thus e.target === window always equals false.

Prinzhorn commented 11 years ago

Have logged the event in the resize handler in the intermediate function within _addEvent and e.target is always null. Thus e.target === window always equals false.

Hm. Just to double check that: you put the logging after this line? https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L1232

ourmaninamsterdam commented 11 years ago

Yes. The event object is being passed as I can log some properties, i.e. e.clientX and e.type.

ourmaninamsterdam commented 11 years ago

It's because the srcElement (target) doesn't exist as the window is the one triggering the event, not a specific element.

document.querySelector('#resize').attachEvent('onclick', function(e){
    console.log( window.event.srcElement ); // LOG: [object HTMLButtonElement]
});
window.attachEvent('onresize', function(e){
    console.log( window.event.srcElement ); // LOG: null
});

RobertinoValue commented 11 years ago

What you write makes sense. I had hoped that the result would proof the theory. You/We were just trying to determine if the following was also the case here (from the Blogger article) :

But some browsers fire resize events when something changes on page. And such a browser is IE8.

And thanks for catching my bad, I wrote earlier :

console.log(event.target === document.body);

while it should have been : console.log(event.target === window);

One thing, if there are more active intermediate event handlers, and you are only logging event.target, you have no indication from which event handler you're logging the event.target. You will need to add at least console.log(this === window);. (true if it is the event we're after.) Although I assume that you already did that.

P.S. I wish that I could also do my own checking and logging. But I forgot to take my notebook back with me (Win7) while I was at a client last Friday. There was no turning back, because the Intercity was already on its way when I noticed.

EDIT: Replace event. with window.event., or e where appropriate.

ourmaninamsterdam commented 11 years ago

There's only a single intermediate event handler in skrollr.js to shim the IE events.

I had done this === window and it was/is validating as true and should always be true, regardless of the event trigger, as the event in old IE is a sub-property of window. So in theory you could check for null on window.event.srcElement (not sure on the solidity of this method) to indicate that window triggered the event.

I don't mind doing some testing where I have the time. I know what it's like when you just want to get your hands dirty within some testing and your hands are tied.

RobertinoValue commented 11 years ago

What I meant is that the same intermediate handler, can be attached to different events. resize, click, hover, etc. While only resize and orientationchange would be of interest for this case. Here intermediate added to window : https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L277 And here it's added to documentElement : https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L603

So I would expect this === window to evaluate to false in the second case, because the event would not be processed at the window object, but at the element object returned by the documentElement property. And then we wouldn't be interested in the target. But you are right, IE8 doesn't exist on mobile. So we have only one instance of intermediate handling events. Also I didn't realise that event is a sub-property of window in old IE. My awareness and knowledge of IE8 has dwindled towards the dungeons of the internet.

Have you already tried throttling the event?

Prinzhorn commented 11 years ago

Have you already tried throttling the event?

That's what I tried in the issue_271 branch. But it's still firing endlessly, just slower.

I just added a check inside the event handler if the size actually changed (and throttling at ~60fps). https://github.com/Prinzhorn/skrollr/commit/0213aa03269e1aec151535ddec51b20b78051a33

Please check with this skrollr file (hot linking OK) https://rawgithub.com/Prinzhorn/skrollr/issue_271/src/skrollr.js

RobertinoValue commented 11 years ago

I wish that I could test this, but I'm not picking up my Win7 notebook until tomorrow morning. It's about 4 hours roundtrip to that client. And I have to be there again tomorrow anyway. Better to combine it.

But then you're still handling each resize event. Don't you want to wait until resizing has finished, and then handle the last event? If not, I misunderstood. (Just asking. Not trying to know it better than you.)

I was thinking more about ignoring all resize events, but the last one. The idea is that as long as the resize event fires, you keep cancelling the timed handler of the previous event and do nothing else. :

// Parameter scope can be : this, window, etc.
// Parameter throttleDelay is in ms.
// When scope is not passed, the method will be executed in the global scope/context (the window object).
//     null and undefined are replaced by the global object (window).
function throttle(method, throttleDelay, scope) {
    clearTimeout(method.timerId); // Cancel previous.
    method.timerId = setTimeout(function(){
        method.call(scope);
    }, throttleDelay);
}

And change https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L1253

element.attachEvent('on' + names[nameCounter], intermediate);

into one of these : The 100ms will most likely need tweaking.

element.attachEvent('on' + names[nameCounter], throttle(intermediate, 100));

element.attachEvent('on' + names[nameCounter], throttle(intermediate, 100, window));

I'm doing something similar in my grid overlay scripts (see below) : The code in the event handler below is way too heavy to execute in each resize event. I eliminated the throttle function though (one call less) by partially integrating it into the resizeGridCheck function.


    // Variables and functions.

/***** SHORTENED *****/

    function resizeGridCheck() {
        $(window).on( 'resize.hashgrid', function(e) {
            clearTimeout(resizeTimer); // Cancel previous.
            resizeTimer = setTimeout(function() {
                if (!overlayOn) {
                    return;
                }
                removeVerticalGridOverlay();
                removeHorizontalGridOverlay();
                buildVerticalGridOverlay(setGridOverlayContainerWidths()); // Rebuild the vertical grid overlay.
                buildHorizontalGridOverlay(); // Rebuild the horizontal grid overlay.
                setGridColumns();
                if (/webkit/.test( navigator.userAgent.toLowerCase() )) {
                    forceRepaint();
                }
                checkForSavedState();
            }, 100);
        });
    }

/***** SHORTENED *****/

    // Mainline script

/***** SHORTENED *****/

    /* Add window resize handler for the active grid system. */
    resizeGridCheck();

Prinzhorn commented 11 years ago

But then you're still handling each resize event. Don't you want to wait until resizing has finished, and then handle the last event?

It's what I want. I want the changes to be visible while resizing the window.

I just want this to be fixed in IE8. Everything else should stay as it is.

RobertinoValue commented 11 years ago

OK, I borrowed a Win7 notebook from someone.

And I ran some quick tests at current master with the test case from the OP. On Win7, IE9 in Browser Mode: IE8

Test 1) When I comment the following line : https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L1273 The problem is completely gone. Which I expected.

body.style.height = (_maxKeyFrame + documentElement.clientHeight) + 'px';

Test 2) When I comment this line : https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L1266 IE and Dev Tools both hang and become completely unresponsive, with CPU usage at 59%. (Had to kill the processes in Task manager.)

body.style.height = 'auto';

Test 3) When I comment this line : https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L1269 The problem remains. Doesn't get worse, or better.

_updateDependentKeyFrames();

EDIT: The following file in the test case in the OP is getting 404. I removed it. :

<!--[if lt IE 9]>
    <script type="text/javascript" src="dist/skrollr.ie.min.js"></script>
    <![endif]-->

Prinzhorn commented 11 years ago

And I ran some quick tests at current master with the test case from the OP. On Win7, IE9 in Browser Mode: IE8

But did you test the issue_271 branch? https://rawgithub.com/Prinzhorn/skrollr/issue_271/src/skrollr.js

RobertinoValue commented 11 years ago

In the line mentioned at Test 1), body.style.height initially increases with 100px. Then I clicked the button to create the first div, and body.style.height increased with 300px. Then the browser/script enters the infinite loop. In this loop body.style.height increases with 16px, each time the event occurs 316px, 332px, etc.

On this line in the infinite loop : https://github.com/Prinzhorn/skrollr/blob/master/src/skrollr.js#L738-L739 _maxKeyFrame is recalculated as increased by 16 pixels, for each event occurrence. So the problem is in function _getDocumentHeight.

//#133: The document can be larger than the maxKeyFrame we found.
_maxKeyFrame = Math.max(_maxKeyFrame, _getDocumentHeight());

Alas, I have to shoot off to this client, and I do not have the Win7 notebook anymore which I borrowed. I will not be back until later this evening. But maybe this is already enough to ring a bell.

Below a screenshot showing loggng of _maxKeyFrame where it increases with 16px at each iteration of the infinite loop. After this, I debugged _maxKeyFrame as described above. (Just ignore LOG: [object Event]. I forgot to take out logging of variable e.)

skrollr-ie8-reflow-issue

RobertinoValue commented 11 years ago

Just noticed your question. Yes I also did test with the issue_271branch (saved it). But that made me think that the problem is not resolved with any kind of throttling, nor checking for changed window/viewport size. It would be like ignoring the real problem. The (IE8) bug would still be there.

Prinzhorn commented 11 years ago

I just booted a Win7 VM (which worked well even though it expired long time ago). And the issue_271 branch solves the issue for the test @ourmaninamsterdam provided. After pushing the button the element gets added without an infinite loop. I can add as many elements as I want.

If someone confirms this, I'm gonna merge it to master.

ourmaninamsterdam commented 11 years ago

Great. I can test this later on today and get back to you. On 30 Jul 2013 09:05, "Alexander Prinzhorn" notifications@github.com wrote:

I just booted a Win7 VM (which worked well even though it expired long time ago). And the issue_271 branch solves the issue for the test @ourmaninamsterdam https://github.com/ourmaninamsterdam provided. After pushing the button the element gets added without an infinite loop. I can add as many elements as I want.

If someone confirms this, I'm gonna merge it to master.

— Reply to this email directly or view it on GitHubhttps://github.com/Prinzhorn/skrollr/issues/271#issuecomment-21775391 .

ourmaninamsterdam commented 11 years ago

@Prinzhorn - Good news! Have tested with IE8 in my Windows7 VM and with IE8 IETester and all works well. Like you I can add as many elems as I like without issue.

Prinzhorn commented 11 years ago

Great! I guess it's fixed then.

Version is 0.6.10

ourmaninamsterdam commented 11 years ago

Fantastic! Great work on fixing this. Thank you.

RobertinoValue commented 11 years ago

It's a ghetto fix. The problem is still there. It only has a band-aid on it now. IE8 needs a different calculation in _getDocumentHeight. But it's up to you. Peace.

Prinzhorn / skrollr

IE8 - Skrollr gets caught in infinite reflow loop when refreshing dynamic elements #271