Add a polyfill for IntersectionObserver

[philipwalton]

This script builds on top of the original work done in #121 with the following changes:

• Adds support for the rootMargin property
• Adds logic to account for parent elements with non-visible overflow
• Adds support for elements on a delayed intersection (e.g. CSS transitions)
• Adds detection of elements on the boundary
• Adds detection of zero-sized elements within root's bounds
• Updates to support targets and roots being added/removed from the DOM
• Removes newer ES6 constructs (like Map) for broader support

With these changes, this polyfill test suite passes in native implementations (Chrome 52+), the latest version of all major browsers, and IE back to version 8 (I didn't test 6-7, but they may work as well).

TODO:

This version uses polling on a 100ms timeout to get the greatest legacy support. I plan to add new logic to use a combination of scroll, resize, and MutationObserver listeners + temporary polling to detect CSS transitions soon, but I wanted to start the pull request with my initial set of changes so we could discuss.

/cc @surma @ojanvafai @triblondon

[ojanvafai]

@szager-chromium

[surma]

also cc @triblondon

[surma]

Also talked to @philipwalton OOB: We would like to get this landed ASAP and start iterating with smaller PRs that are easier to review :)

[triblondon]

Um, ok. So this is a complete rewrite?

Happy to see rootMargin support and legacy compatibility. Concerned about the 100ms loop, which we don't like in the polyfill service. We prefer a feature that we're polyfilling to have poor perf rather than risk having a negative effect on the perf of everything else on the page.

Will try importing this today. Polyfill service work is here polyfillpolyfill/polyfill-service#684

[philipwalton]

@triblondon it's not as much of a rewrite as the diffs would make it seem. To get it working in IE8, I had to remove a lot of the es5 object literal notation, which makes the diffs somewhat meaningless now, unfortunately.

Concerned about the 100ms loop, which we don't like in the polyfill service.

I'm absolutely planning on enhancing the polyfill for browsers that support MutationObserver and the event capture phase to not have to poll, but since the current code will still be needed as a fallback for browsers that don't, I wanted to get this checked in ASAP and improve upon it iteratively.

I also wanted to get a set of test cases that pass for browsers with the native implementation, so future enhancements could always be measured against that.

[triblondon]

The polyfill service is designed to serve this polyfill for you, and do the feature detection. Just use (unconditionally):

<script src="https://cdn.polyfill.io/v2/polyfill.min.js?features=getComputedStyle"></script>

[triblondon]

@philipwalton Could an argument be made that for browsers that lack MutationObserver, an implementation that uses scroll events only is 'good enough'. The primary use case for this is detecting interactions that occur as a result of scrolling. Intersections from repositioning of elements are less useful and I would prefer to avoid a tight interval loop in slow JS engines, which have the potential to cause more problems than they solve. If you're loading a lot of polyfills, and several of them do that, then it could be (ever) hard(er) to debug stuff.

I've updated our IO PR, and am currently testing it on all browsers we run on the Polyfill service CI. I found that the new suite does pass in Chrome with native implementation, albeit with some fairly slow ones that almost hit Mocha's timeout on my fairly elderly Macbook air

On IE8, it's not looking so good:

IE9 (emulated in IE11):

IE10 and 11 seem fine.

Our CI: https://circleci.com/gh/Financial-Times/polyfill-service/277

[triblondon]

These failures in IE8-9 could be a result of interaction between your polyfill and those that we are loading prior to it as dependencies, or possibly due to a different version of Mocha. I'lll poke into this a bit more.

[philipwalton]

These failures in IE8-9 could be a result of interaction between your polyfill and those that we are loading prior to it as dependencies, or possibly due to a different version of Mocha. I'lll poke into this a bit more.

The tests pass in IE8 for me, though I'm running them on a VM that has IE11 and I'm using the browser emulation mode to run IE8. I can run them on Sauce Labs in IE8 to double check.

[philipwalton]

@philipwalton Could an argument be made that for browsers that lack MutationObserver, an implementation that uses scroll events only is 'good enough'.

The only problem with that is the scroll event doesn't bubble, so in IE8, if a dev built a site with multiple scrollable containers (or even a single scrollable container that wasn't <body>, then a single scroll listener on document won't get those events. In newer browsers you can workaround this by adding the scroll handler to the capture phase.

In terms of "good enough", I do agree that developers should have options. It'd be great if there were a few good versions out there, some that strove for correctness, and some that intentionally didn't support certain features in favor of performance.

My opinion is that the version in the spec repo should strive for correctness, as the primary point of the polyfill is to give developers a chance to play around with the API and offer feedback.

Would the polyfill service be open to maintaining a less correct but more performant version that we could recommend for developers who understand that tradeoffs?

[philipwalton]

Just as an update, I ran the tests on Sauce Labs and they pass in IE8:
https://saucelabs.com/beta/tests/fadeb12ff34a4a09b18be93db616aecf/watch

Note: originally they were failing, but it turns out this was only because Sauce Labs was running the test page in quirks mode. Once I added 7e5c50c they all passed.

[triblondon]

• Yes we'd be happy to maintain a small modification to the polyfill to make it less aggressive by default. You have already exposed the poll interval, so I think we'd just change the default. Ideally by pulling in the polyfill from npm, turning on commonjs build, and referencing it like this:

const IO = require('intersection-observer-polyfill');
IO.prototype.POLL_INTERVAL = 10000;
module.exports = IO;

• You've made the poll interval configurable, but you're reading it from the global native object. Could you attach it to the polyfill object instead (otherwise the above code wouldn't work)
• I'll upgrade our version of Mocha and see if that helps my test results.

[philipwalton]

tl;dr: Update: I think I have a shippable version that meets everyone's requirements. Please take a look and give any feedback you have. I'd like to get this merged ASAP.

---

I've added the logic to listen to DOM events and mutations as a default rather than polling. I've kept the polling option available though, so users can opt-in if they want/need to (/ht @triblondon for his comment that made me realize this could be the default behavior).

The way the code is now, the polyfill does not poll by default, instead it listens to the resize and scroll events as well as creates a MutationObserver instance that listens for any DOM change. The combination of these three listeners should cover the vast majority of use-cases. Here's a list of the use-cases I could think of that it won't cover:

• CSS rules that reveal/hide elements on :hover/:active/:focus.
• CSS transitions/animations that reveal/hide elements after a >100ms delay.
• Resizable <textarea> elements whose resizing may cause target intersections.
• Scrolling of non-document elements in <IE8.

For users who care about any of these use-cases, they can opt-in to polling either for all IntersectionObserver instances or just for select instance.

To opt-in to polling for all instances:

if ( /* some fallback conditional */ ) {
  IntersectionObserver.prototype.POLL_INTERVAL = 100; // ...or any interval you want.
}

To opt-in to polling for only select instances:

var io = new IntersectionObserver(callback);
io.POLL_INTERVAL = 100;
io.observe(someTargetElement);

As long as POLL_INTERVAL is set prior to calling observe() on the first target, polling will be used instead of event/mutation listeners.

My suggestion is we document all of this in the README, so users who want to know the specifics can get that information, and users who don't want to read will get a reasonably performant polyfill that should handle most of their use-cases.

[triblondon]

These updates look splendid. I spent a long time trying to ascertain why I wasn't getting this to work in IE8 but then I realised that your getComputedStyle polyfill is also pulling in various ES5 bits and pieces. I've figured out which of those methods you are actually using and added them to the dependencies list for IO. It's now looking considerably better, but I still have one failure:

This is in fact actually an expectation failure that Mocha is not capturing:

I'm using a Win7/IE8 VM downloaded from modern.ie, running in VirtualBox on OSX. I have updated the Polyfill service Mocha to match the version you're using.

Other thoughts:

• Thanks for setting up your tests in script, that's very kind and extremely helpful for us.
• Would you consider adding your timeout config into your outer describe? You can do this.timeout(10000) and that will set the timeout for everything in that describe clause.
• You are still polyfilling Array.isArray but you're pulling it in from us as well. You could remove your polyfill

[triblondon]

Our PR polyfillpolyfill/polyfill-service#684

[philipwalton]

Would you consider adding your timeout config into your outer describe? You can do this.timeout(10000) and that will set the timeout for everything in that describe clause.

I ended up looking to see if the 10 second timeout was ever actually needed anywhere (I set it to that originally for debugging purposes), and only one of the tests was ever even coming close to timing out, so I slightly modified that test, and now the default timeout should be more than sufficient for all the tests, even in slower browsers.

If any of the tests start causing a lot of timeout flakiness (in the future), I think it's better to extend the timeout of the individual, offending test and explain why the extension is needed. This should be better for both of our use-cases.

[philipwalton]

This PR is just waiting on @szager-chromium to review and merge. I spoke with him today, and he's going to try to get to it as soon as possible.

[triblondon]

@philipwalton I can't get your tests to pass in many browsers. I cloned this repo, switched to the polyfill branch, ran a python simplehttpserver, set up a sauce tunnel and loaded localhost:8000 on a sauce VM.

FF42 / linux (expectation failure)
https://saucelabs.com/beta/manual/test_proxy/2b959a5ee4044ca5ae8d4c0b751279f9

Android 4.4 (timeouts):
https://saucelabs.com/beta/manual/test_proxy/780ef5ff0c004fd1909e00833b0eee0e

IE6 / WinXP (Lots of errors, but OK, fair enough!):
https://saucelabs.com/beta/manual/test_proxy/cba5317e30fb42e39b62529a6c60fdcc

IE7 / WinXP (timeout):
https://saucelabs.com/beta/manual/test_proxy/b000e34193de406488dafa9f3b2a1ef1

IE9 / Win7 (expectation failure)
https://saucelabs.com/beta/manual/test_proxy/9f4dac9859b54e988d7dcfd1148d5256

Safari 8 / OSX (timeout):
https://saucelabs.com/beta/manual/test_proxy/b3ed4c5e43134e00a0ce37505582bc7d

Safari 9 / OSX: pass

Do all these browsers pass the tests for you?

[philipwalton]

@triblondon I made a few changes, and now I'm getting the tests to reliably run in these browsers (I didn't have time to see what was failing in IE6).

[philipwalton]

@szager-chromium I rebased against your recent changes to gh-pages from #138, so the history should now be correct.

[szager-chromium]

This doesn't actually check that the entries are numbers. Maybe this:

return threshold.apply(Number).sort().filter(function(t, i, a) {
if (Number.isNaN(t)) {
throw new Error('threshold must be a number or array of numbers.');
}
if (t < 0 || t > 1) {
throw new Error(...)
}
return t != a[i-1];
});

[philipwalton]

The native implementation seems to ignore non-number values without throwing, as long as the non-number value is after a real value.

For example, this seems to work in Chrome threshold: [0, 'foo']. If the spec doesn't say to throw and Chrome doesn't throw, I'd rather not throw. But I do think the implementations should be consistent. Especially for an easy case like this. What do you think?

[szager-chromium]

I created a CL for the native implementation to throw an Error for non-number thresholds:

https://codereview.chromium.org/2132863002

I think the polyfill should follow suit.

[philipwalton]

Done.