Add IntersectionObserver polyfill – Take #02

[surma]

Continued from #116

[surma]

Improved the intersection detection to match the native implementation (technically, my previous behavior was just straight up non-compliant with the spec). PTAL :)

[triblondon]

Cool. Is margin support coming, or should we ship a v1 at this point?

[surma]

I vote for shipping a v1 without margin support.

[jeremenichelli]

Hey there,

I just want to point out some stuff and do some questions around this polyfill.

• in line 120 I'm seeing a return around intersectionRect that comes from a private function that always returns non-empty object, so !intersectionRect will always be true making the if statement pointless.
• instead of this scope.IntersectionObserver = scope.IntersectionObserver || IntersectionObserver it would be better to return at the beggining of the closure if 'intersectionObserver' in window is true so we don't execute unnecessary code when the API is present natively.
• wouldn't it be better to throttle the action on scroll using requestAnimationFrame?
• There's some ES2015 notation when the constructor prototype is declared, but also seeing some hack for old IE around getBoundingClientRect which is not consistent, the polyfill itself won't work in old IE because of this new object notation. I think it would be good to do it all in ES5 or don't add hack for legacy browsers.
• Similar to the point from above, the code is asumming that performance is globally available, which is not in IE8/9, something like this https://gist.github.com/paulirish/5438650 should be added if the aim is to cover old IE, if not then makes no sense to add a getBoundingClientRect hack.
• Remember to remove console.log statements.
• this._update should be updated on resize too, at least that's the API's behavior in Chrome Canary right now.
• In lines 207 and 208 there's no separation around the - operator making it less readable in my opinion.

I hope this feedback is useful, you probably have answers and reasons for most of these, so sorry in advance if I missed something 😀

[triblondon]

Personally I don't much care about relying on recent APIs or syntax because
syntax can be dealt with by transpiling and APIs by including further
polyfills. However, it does make sense to embed workarounds for browser
inconsistencies in older APIs where the API is widely supported but with
inconsistencies.

But it's really important to me to avoid embedding polyfills for things we
are not primarily trying to polyfill here. That's a slippery slope to
heavily duplicated front end code if a developer combines multiple
polyfills together.

So in summary:

• I'd favour being es5 compliant, I think that's a good practice for
  polyfills
• we should not embed polyfills for other APIs which can be polyfilled
  separately.
• it's ok to defensively code around inconsistencies in older APIs

On Sat, 16 Apr 2016 at 08:56, Jeremias Menichelli notifications@github.com
wrote:

Hey there,

I just want to point out some stuff and do some questions around this
polyfill.

• line 120: I'm seeing a return around intersectionRect that comes
  from a private function that always return non-empty object, so
  !intersectionRect will always be true making the if statement
  pointless.
• instead of this scope.IntersectionObserver =
  scope.IntersectionObserver || IntersectionObserver it would be better
  to return at the beggining of the closure if intersectionObserver in
  window is true so we don't execute unnecessary code when the API is
  present natively.
• wouldn't it be better to throttle the action using
  requestAnimationFrame?
• I'm seeing some ES2015 notation when the constructor prototype is
  declared, but also seeing some hack for old IE around
  getBoundingClientRect which is not consistent, the polyfill itself
  won't work in old IE because of this new object notation. I think it would
  be good to do it all in ES5 or don't add hack for legacy browsers.
• Similar to the point from above, the code is asumming that
  performance is globally available, which is not in IE9, something like
  this https://gist.github.com/paulirish/5438650 should be added is the
  aim is to cover old IE, if not then makes no sense to add a
  getBoundingClientRect hack.

I hope this feedback is useful, you probably have answers and reasons for
most of these, so sorry in advance if I missed something [image:
😀]

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#121 (comment)

[jeremenichelli]

@triblondon I was actually thinking about that multi polyfilled scenario and I agree, that's a good point. I added three more points recently since I forgot them while adding the comment.

[triblondon]

@surma where are you going to host this polyfill? Is this WICG repo going to be the canonical source? If so I can require it from here. Ideally the polyfill would have its own repo though, and be published to npm.

[surma]

I thought the WICG repo would be a good fit, but you are right in that it’s not really npm install-able this way. I have no strong opinion on this. @ojanvafai WDYT?

[surma]

The ES2015 code was never meant to make it into the repo. Oversight on my part. Definitely was trying to use ES5 only.

I implement most of your remarks, @jeremenichelli. PTAL :)

[jeremenichelli]

Looks great @surma, nice job 👌

[jeremenichelli]

Just noticed one thing. Now that we're listening to the resize event, it should be removed inside the disconnect function https://github.com/surma-dump/IntersectionObserver/blob/981e7d355ef14069405e9131eca695be3cc5abfa/polyfill/intersectionobserver-polyfill.js#L88

[surma]

Oh good catch! Fixed

[surma]

@triblondon I appreciate any time you spend on this. Feel free to contribute your code to this PR :)

If you want to ditch IO and add margin support, I am obviously fine with that. I just didn’t want to implement a CSS string parser and thought IO were a good choice. Apparently they aren’t. Also, I don’t want to be held responsible for @slightlyoff opening those old wounds 😉

I don't believe perfect feature replication is a goal worth achieving at all costs

Since I was actively ignoring margins all this time, that is perfectly fair to say. If no one is veto-ing your assessment of the 99% use-case, go for it :)

Regarding the feedback from @aFarkas:

• WeakMap seems like a good idea. Not sure what needs to change in terms of implementation
• Currently, setTimeout(..., 100) is used to deliver the data to the callbacks. I thought that was pretty rIC-like.
• That should only be a problem if the scroll event is actively stopped from propagating/bubbling, right? Or am I missing something? Personally, I’d be fine with ignoring that circumstance.

[triblondon]

Scroll doesn't bubble.

I've updated my tests. I think this pattern works really well, offers a more linear read of the tests and assertions, tests run faster, and they're compatible with older browsers. It's actually slightly shorter too. I think your margin tests were slightly awry because the test case does not set up the IO with any margins! So I reset that test case to the steps I had in my last work on it.

I'm now finding that Chrome 52 (canary) native passes all except the margin tests:

Chrome 50 (stable) using the polyfill:

I don't have push on either your fork or the WICG, so I can make a new surma-dump fork if you'd like these changes proposed for merge into your PR, but in the meantime I pushed them to https://github.com/Financial-Times/polyfill-service/compare/intersect-observer#diff-c0d2dbf8759d522d6028477a3a43497f

[surma]

Great! Awesome stuff, @triblondon. Made you collab on my fork. So feel free to push your changes. Looking forward to it.

[triblondon]

OK, I pushed. Notes in the commit messages should be fairly explanatory. I guess your next steps are to investigate the difference in native vs polyfill test results, and look at @aFarkas's points?

[triblondon]

@surma will you be picking this back up at some point? We have a PR open in the polyfill service to merge this in once its ready.

[surma]

Sorry for the absence, but Google I/O was happening and was quite the marathon. Just got back and will continue working on this soon. Thanks for your work and for the patience :)

[birkir]

Safari throws an read-only error on setting this property. I fixed it by checking if the property is writable using the property descriptor getter.

var desc = Object.getOwnPropertyDescriptor(r, 'width');
if (desc && desc.writable) {
  r.width = r.width || r.right - r.left;
  r.height = r.height || r.bottom - r.top;
}

[jeremenichelli]

I have this problem when using getBoundingClientRect, I think the best way to get around this is to return a different object with the rects.

var r = el.getBoundingClientRect();
  if(!r) {
    return null;
  }
  // old IE
  return {
    top: r.top,
    bottom: r.bottom,
    left: r.left,
    right: r.right,
    width: r.width || r.right - r.left,
    height: r.height || r.bottom - r.top
  }

Extra code, but safe.

[surma]

Continued in #135