Add IntersectionObserver polyfill #116

surma · 2016-03-16T14:10:50Z

r: @ojanvafai
cc: @ianvollick @paullewis @flackr

This adds a polyfill for the IntersectionObserver API. It is compatible to the current version of the spec but ignores margins for now, as I wanted to avoid implementing a CSS string parser.

There’s also a simple test suite by @ojanvafai. All tests pass except for the margin tests for the reasons mentioned above. Please take a look at the tests to check that the polyfill is behaving as you’d expect.

Any other feedback obviously welcome as well :)

mpb · 2016-03-16T18:59:36Z

polyfill/intersectionobserver-polyfill.js

+    }
+
+    this._callback = callback;
+    this._root = options.root || null;


If 'root' is passed in, there should probably be some check that it's valid, and not an object or function or something random.

All observe() calls will fail if root is invalid. So this check is not strictly needed, but I agree it’s probably a good idea to add that check.

mpb · 2016-03-16T19:05:59Z

Should this polyfill throw an exception or otherwise indicate an error if someone tries to use it in a cross-domain iframe with root === null? In all other situations, I think this polyfill will provide correct behavior, but for this case the behavior will diverge pretty substantially.

triblondon · 2016-03-17T04:44:08Z

polyfill/intersectionobserver-polyfill.js

+      this._observationTargets = new Map();
+      this._boundUpdate = this._update.bind(this);
+      this.root.addEventListener('scroll', this._boundUpdate);
+      this._intervalId = window.setInterval(this._boundUpdate, 100);


This seems less efficient than typical lazyload libraries, in that it polls the state of the observed element on every scroll event, and every 100ms. I would like to put this polyfill into the polyfill.io service library but am concerned about the frequency of these events. How about throttling the callback to one invocation per 100ms?

Also I assume we're polling separately to the scroll events to capture scenarios where the element moves from outside to inside the viewport without the document scroll position changing. Could this lean on MutationObserver to avoid setting up a tight interval timer that runs indefinitely?

That's a good point, "every scroll event" should probably be throttled to 100ms too.

So, I agree that the polling and the scroll-events overlap. I can do something about that.

However, I am not sure about rate-limiting the updates to 100ms in general. If you want to use this data in animation, you ideally get an update from the IntersectionObserver every frame, right? Or did I miss something in the spec about this? I was thinking to use requestAnimationFrame() so you get frame-by-frame updates in the polyfill.

Hm it would be great if the library can collect events as often as necessary, but batch delivering them and cap invoking the callback to no more than once every 100ms. Unfortunately, calling getBoundingClientRect on every scroll event is expensive. Maybe someone with more knowledge about the performance implications can weigh in? @KenjiBaheux @ojanvafai ?

getBoundingClientRect() is expensive, but that’s the polyfill only. The actual browser implementation will be more efficient and will probably dispatch on every frame, right? Otherwise the entire thing becomes rather useless for 60fps animations. Also, I thought you would use the thresholds to limit the number of events.

It’s a tough call and I’d appreciate more feedback. Personally, I’d rather emulate the same behavior of an actual browser implementation than worry too much about performance. It is a polyfill afterall. For more performance input, lemme loop in @paullewis.

The native implementation calculates intersections every frame, but it uses an idle callback with a timeout of 100ms to deliver them. To match that, you would need to separate the code that calculates the intersection from the code that delivers the notifications.

I agree that calculating intersections in a scroll handler is problematic from a performance perspective. It may very well trigger additional layouts, which is really anathema to whole purpose of this feature.

Even with the native implementation, notifications may be delayed by up to 100ms, so code that relies on up-to-the-frame information (e.g., input event handlers that want to validate the position of the clicked element before taking action) will need to use the takeRecords() method to get the most current information. With that in mind, my suggestion is this:

Do the intersection calculations on a 100ms timer. If takeRecords() is invoked, and too much time has passed since the last intersection calculation, then do the intersection calculation right away and return the up-to-date information to takeRecords().

Yes, the actual implementation fires on every rendered frame, basically. It can do this because it knows layout is clean at the time it runs, and doesn't have to worry about getBoundingClientRect triggering a layout.

The polyfill is closer to the actual implementation if it fires on every scroll and buffers, and triggers a callback based on a timer. But it will have performance implications.

I did not know that rIC will be used for dispatch. Agreed then. I will adjust the code accordingly.

surma · 2016-03-19T11:50:06Z

Review has been implemented. PTAL :)

triblondon · 2016-03-28T10:20:44Z

I'm still concerned at the 100ms setInterval that is constantly polling the bounding rect of the element. Was there a reason you can't use mutation observer for this? If no elements on the page are changing I don't see why we need to keep polling, and it makes debugging a nightmare if your timeline is full of timer events. With polyfills, performance of the feature being polyfilled is not a primary concern, but causing perf issues in other code certainly is. We want polyfills to make older browsers MORE reliable, not less. On that basis, I would argue for a less aggressive, more conservative timer policy so that we reduce risk of crash or introducing jank, at the expense of a perfect reproduction of native perf on this specific feature.

We're also building on getClientBoundingRect here, which lacks width/height support in IE8 and below, so the polyfill is not likely to work in IE < 9, though I guess you could just use the innerWidth and innerHeight properties of the element if the boundingrect call returns no width or height.

I've started porting your tests into the polyfill.io library, and will take the polyfill once we've concluded this discussion. Can you declare the licence terms under which you are publishing this code? For polyfill.io we prefer MIT or CC0 but Apache (which appears to be one of the licences used in this repo) is fine too.

surma · 2016-03-28T13:56:44Z

You are totally right. MutationObserver are a way better approach than setTimeout, will fix that. I would vote for not worrying about IE 8, 9 or 10 as they are officially deprecated and also don’t have MutationObserver.

triblondon · 2016-03-29T13:08:31Z

In polyfill.io we care about IE 6+ currently (yes really), but I take a pragmatic view of it. It seems like falling back to element width and height is easy, so why not. No need to do any heroics to get support in old browsers but this seems easy enough that you may as well do it.

surma · 2016-03-29T13:23:48Z

Hats off to you for supporting IE6 😄

As you suggested, I moved the implementation to use MutationObserver which makes this only work in IE11 and above, so there’s no point adding that fallback, or am I missing something?

triblondon · 2016-03-29T16:25:38Z

Well, polyfill.io polyfills for MutationObserver, and we can make the IO polyfill depend on MO, then in browsers that don't have MO we ship that polyfill as well!

Also, we can offer some useful functionality here without MO, because many sites will only require the detection on scroll, so if you can shield the MO instantiation in a feature-detect, we can offer the polyfill on a wider range of browsers.

surma · 2016-03-29T17:25:38Z

Fair enough. I’ll add the fallback dimensions :)

surma · 2016-04-01T21:46:39Z

Done :)

mpb · 2016-04-01T22:07:11Z

polyfill/intersectionobserver-polyfill.js

+        return;
+      }
+      var context = this, args = arguments;
+      clearTimeout(timer);


Won't timer always be null here?

Not on rapid calls on the function being returned in line 213.

Oh, nvm, you are right.

triblondon · 2016-04-02T12:45:42Z

polyfill/intersectionobserver-polyfill.js

+        this._descheduleCallback();
+        this._callback(this._queue, this);
+        this._queue = [];
+      }.bind(this), {timeout: 100});


Magic number, move to constant?

triblondon · 2016-04-02T12:55:43Z

@surma I am stuck in an airport for 6 hours so I did a more thorough code review. Hope that's OK - and some of these are points you may well disagree with.

surma · 2016-04-04T12:08:01Z

@triblondon Thank you very much for the thorough feedback. It’s highly appreciated :) Updated the code accordingly.

triblondon · 2016-04-04T12:33:08Z

Looks good. I'm working on getting the tests ported then I can test it in the polyfill service.

triblondon · 2016-04-04T15:15:03Z

polyfill/intersectionobserver-polyfill.js

+      if (timer) {
+        return;
+      }
+      callback.apply(this, arguments);


callback => fn (sorry, this was wrong in my suggested code)

I’m so lazy... fix’d :D

triblondon · 2016-04-04T15:25:31Z

OK, I ported the tests and pushed them here:

https://github.com/Financial-Times/polyfill-service/blob/intersect-observer/polyfills/IntersectionObserver/tests.js

If you install and run the intersect-observer branch of the polyfill service, go to http://localhost:3000/test/tests?feature=IntersectionObserver&mode=all to run the tests in a local browser. Change mode=all to mode=control to run the tests without the polyfill.

Pretty good results in FF 38 and Chrome 49 using the polyfill, though it does not appear to support margins:

Not so good in Chrome 49 with experimental web platform flag enabled, testing the native support:

triblondon · 2016-04-04T15:27:53Z

The problem with the native impl seems to be that records do not seem to be available synchronously with takeRecords(). Maybe. What do you think?

triblondon · 2016-04-04T15:29:19Z

Ah, the polyfill has a TODO against margin support, so I guess that my port of the tests is working then :-) So it's mostly a case of why the native impl fails the tests.

surma · 2016-04-04T15:38:07Z

Thanks a lot for your work, @triblondon.

I’ll loop in @ojanvafai to check if my tests are wrong or the native implementation of Chrome is.

surma · 2016-04-05T11:58:24Z

For the record: Talked to @ojanvafai and rest of the team. I am making assumptions in the tests that are not correct and break with the native implementation. I’ll fix that. @triblondon You might have to re-port some of the tests.

triblondon · 2016-04-05T12:08:17Z

Would you consider using mocha/expect? It seems to make for much neater tests, and you should be able to run the tests I made without the overhead of the polyfill service. Just grab the Mocha demo, add Expect.js from CDNJS and replace the test cases with the contents of my file.

surma · 2016-04-05T12:13:01Z

Will do.

surma · 2016-04-09T15:02:39Z

Tests have been moved to mocha and I also made most of the tests properly asynchronous as takeRecords() isn’t very reliable for tests.

Some checks are commented out as the native implementation in Chrome 51 is missing the intersectionRatio field (will be fixed soon).

Other than that, all tests pass on both native and the polyfill implementation except for the margin tests. Margin tests fail on both, ironically, even on native. Looking into that.

szager-chromium · 2016-04-09T18:59:27Z

@surma Maybe it will help you to refer to the root margin test for the native chromium implementation:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/intersection-observer/root-margin.html&sq=package:chromium&type=cs

triblondon · 2016-04-12T02:22:27Z

@RByers why close?

RByers · 2016-04-12T03:29:11Z

Whoops, sorry - that must have been a side-effect of switching to the gh-pages branch (#120) sorry. You'll probably need to update the PR to be based on gh-pages instead of the (now-deleted) master branch.

surma · 2016-04-14T08:38:28Z

Continued in #121

Add IntersectionObserver polyfill

e69581f

mpb reviewed Mar 16, 2016
View reviewed changes

triblondon reviewed Mar 17, 2016
View reviewed changes

surma added 4 commits March 19, 2016 16:51

Check root element in constructor

90d45f4

Typo

e21b29b

Debounce scroll event

78a2214

Fix tests

0cbc753

surma added 2 commits March 28, 2016 14:15

Use MutationObserver

dbab51e

Add license header

9fca7fe

Calculate a rects width and height if not given

dcd9cc1

mpb reviewed Apr 1, 2016
View reviewed changes

triblondon reviewed Apr 2, 2016
View reviewed changes

Code review

5b78388

triblondon reviewed Apr 4, 2016
View reviewed changes

Minor typo

d6da2e3

surma added 3 commits April 9, 2016 15:43

Move tests to mocha

c98bdd6

Fix margin tests

d7cc95b

Use consistent indents

dcebb1a

RByers closed this Apr 12, 2016

surma mentioned this pull request Apr 14, 2016

Add IntersectionObserver polyfill – Take #02 #121

Closed

Add IntersectionObserver polyfill #116

Add IntersectionObserver polyfill #116

Uh oh!

Conversation

surma commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpb commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surma commented Mar 19, 2016

Uh oh!

triblondon commented Mar 28, 2016

Uh oh!

surma commented Mar 28, 2016

Uh oh!

triblondon commented Mar 29, 2016 via email

Uh oh!

surma commented Mar 29, 2016

Uh oh!

triblondon commented Mar 29, 2016

Uh oh!

surma commented Mar 29, 2016

Uh oh!

surma commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

triblondon commented Apr 2, 2016

Uh oh!

surma commented Apr 4, 2016

Uh oh!

triblondon commented Apr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

triblondon commented Apr 4, 2016

Uh oh!

triblondon commented Apr 4, 2016

Uh oh!

triblondon commented Apr 4, 2016

Uh oh!

surma commented Apr 4, 2016

Uh oh!

surma commented Apr 5, 2016

Uh oh!

triblondon commented Apr 5, 2016

Uh oh!

surma commented Apr 5, 2016

Uh oh!

surma commented Apr 9, 2016

Uh oh!

szager-chromium commented Apr 9, 2016

Uh oh!