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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend feature detection with isIntersecting #211

Closed
stefanjudis opened this issue May 18, 2017 · 21 comments

Comments

Projects
None yet
6 participants
@stefanjudis
Copy link
Contributor

commented May 18, 2017

馃憢 Hello.

I discovered that according to caniuse Edge 15 supports the Intersection Observer API. While playing around with it I discovered though, that Edge does not implement the isIntersecting property.

The Firefox implementation (behind a flag currently) is also missing this property on IntersectionObserverEntry.

Would it make sense to extend the feature detection which is currently:

if ('IntersectionObserver' in window &&
    'IntersectionObserverEntry' in window &&
    'intersectionRatio' in window.IntersectionObserverEntry.prototype) {
  return;
}

with a check for this property? I think it's really handy and this property missing could lead to "unexpected behavior" on developer's side.

if ('IntersectionObserver' in window &&
    'IntersectionObserverEntry' in window &&
    'intersectionRatio' in window.IntersectionObserverEntry.prototype &&
    'isIntersecting' in window.IntersectionObserverEntry.prototype) {
  return;
}

Thanks in advance. :)

@stefanjudis stefanjudis changed the title Extend feature detection with isIntersection Extend feature detection with isIntersecting May 18, 2017

@szager-chromium

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2017

It's possible to polyfill the isIntersecting property. The polyfill should use the native implementation where available, and polyfill in the missing property.

@stefanjudis

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

馃憤 Sounds reasonable to me, this would still mean, that the initial feature detection has to be changed to the proposed version, right?

@szager-chromium

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2017

I think it would something like:

if ('IntersectionObserver' in window &&
    'IntersectionObserverEntry' in window) {
    if (!('intersectionRatio' in window.IntersectionObserverEntry.prototype)) {
        addIntersectionRatioPolyfill();
    }
    if (!('isIntersecting' in window.IntersectionObserverEntry.prototype)) {
        addIsIntersectingPolyfill();
    }
    return;
}
@pastelsky

This comment has been minimized.

Copy link

commented May 30, 2017

The relevant edge bug is filed here. addIsIntersectingPolyfill should be straightforward. isIntersecting should be equal to intersectionRatio > 0.

Any progress on this?

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

There are some reports that, on Chrome anyway, there are cases where intersectionRatio === 0 and yet isIntersecting is true: tootsuite/mastodon#3502

So I was wrong when I said intersectionRatio > 0 is a good polyfill. Apparently the correct polyfill is described here: #69 (comment)

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

That polyfill apparently doesn't work either: tootsuite/mastodon#3502 (comment). Any ideas?

@philipwalton

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

There are some reports that, on Chrome anyway, there are cases where intersectionRatio === 0 and yet isIntersecting is true: tootsuite/mastodon#3502

Yes, this can happen when the target is on the boundary of root but doesn't overlap at all. This is the spec'ed behavior.

As for polyfilling the isIntersecting property in Edge, I don't think it's as easy as folks have suggested since it requires keeping track of all intersection changes (since a threshold of 0 can be either intersecting or not intersecting based on the previous value) -- something an entire polyfill can easily do but would be more complicated for something that was just polyfilling that one feature.

I'm also not so keen on changing the logic to determine whether the polyfill should apply because that would mean all Edge users would get the polyfilled version instead of the native version, even in cases where the application code doesn't depend on the isIntersecting property.

For the moment, I think a fair compromise would be for developers who depend on the isIntersecting flag and who also want to support Edge to selectively choose whether or not they want the polyfilled version or the native version. They could do that by conditionally deleting the global IntersectionObserver constructor if they discover it's supported without the isIntersecting flag. Something like the following code (executed prior to loading the polyfill) should work:

<script>
if ('IntersectionObserver' in window &&
    !('isIntersecting' in window.IntersectionObserverEntry.prototype)) {
  delete window.IntersectionObserver
}
</script>
<script src="/path/to/intersection-observer-polyfill.js"></script>
@stefanjudis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

@philipwalton This approach would also work for me.

For the moment, I think a fair compromise would be for developers who depend on the isIntersecting flag and who also want to support Edge to selectively choose whether or not they want the polyfilled version or the native version. They could do that by conditionally deleting the global IntersectionObserver constructor if they discover it's supported without the isIntersecting flag. Something like the following code (executed prior to loading the polyfill) should work:

The only problem I see with this approach (鈽濓笍) is, that we're lacking showing the information.

E.g. the browser support info of the polyfill and caniuse claim full support in edge, which means that devs most likely won't be aware of it and only discover it by accident.

I could propose a PR to at least share this information (including your delete window.intersectionObserver snippet) in this repo and at caniuse 鈥 if we agree that this is the desired solution. :)

@philipwalton

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

The only problem I see with this approach (鈽濓笍) is, that we're lacking showing the information.

Yes, we'd definitely want to update the README with this at minimum. You can PR or I can do that after we decide which action to take.

For the moment, I think a fair compromise would be for developers who depend on the isIntersecting flag and who also want to support Edge to selectively choose whether or not they want the polyfilled version or the native version.

It occurred to me this morning that we could go about this the opposite way and default to spec-compliant behavior while still allowing site authors to opt-in to the native implementation. In other words, the polyfill would be used for any browser not implementing the isIntersecting property, and site authors could stub that property if they know they're not going to use it and want the native implementation for everything else:

<script>
if ('IntersectionObserverEntry' in window &&
    !('isIntersecting' in IntersectionObserverEntry.prototype)) {
  IntersectionObserverEntry.prototype.isIntersecting = () => {
    throw new Error('isIntersecting is not implemented and shouldn not be called');
  };
}
</script>
<script src="/path/to/intersection-observer-polyfill.js"></script>
@stefanjudis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2017

I'm also good with the opposite way. 馃憤

The thing I'm not really into is that devs would have to deal with this on their side because I wouldn't expect that when I'm dropping in a polyfill. Couldn't we move an additional stubbing and notification logic into the polyfill itself?

Having it inside the polyfill would mean that people that just dropped in the polyfill would then be notified and lead back to the given docs sections (or even this issue) 馃憠 "Heyo you're using the unsupported isIntersecting property which is not easy to polyfill. Go here for further details and ways to deal with it."

// Exits early if all IntersectionObserver and IntersectionObserverEntry
// features are natively supported.
if ('IntersectionObserver' in window &&
    'IntersectionObserverEntry' in window &&
    'intersectionRatio' in window.IntersectionObserverEntry.prototype) {
  !('isIntersecting' in IntersectionObserverEntry.prototype)) {
    Object.defineProperty(window.IntersectionObserverEntry.prototype, 'isIntersecting', {get: () => { throw new Error('isIntersecting is unfortunately not supported. Go here for details.'); }})
  }
  
  return;
}

Edit: Hmm... but we might not throw here, because that might be backwards incompatible for people using the polyfill already. (maybe just a log message)

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

I just spoke with the dev who implemented IntersectionObserver in Edge, and he said:

intersectionRatio > 0 should be a reasonable polyfill solution if the property doesn鈥檛 exist

I haven't thoroughly tested yet, but apparently we have confirmation that Edge's implementation is different from Chrome's in that it won't report intersectionRatio === 0 when transitioning from a non-intersecting state to an intersecting state. This means you can use this as an extremely lightweight polyfill while we work to fix this in Edge. 馃槂

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

Also FWIW we have been using intersectionRatio > 0 as a replacement for isIntersecting in Mastodon v1.4.1 and AFAICT the only bug reports have come from Chrome or Chromium-based browsers (tootsuite/mastodon#3502 tootsuite/mastodon#3600). Also I've used Mastodon myself in Edge and Firefox (Firefox uses the polyfill) for the past week and haven't observed the bug.

@philipwalton

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

apparently we have confirmation that Edge's implementation is different from Chrome's in that it won't report intersectionRatio === 0 when transitioning from a non-intersecting state to an intersecting state. This means you can use this as an extremely lightweight polyfill while we work to fix this in Edge. 馃槂

Cool, if it matches the native Edge implementation then I see no reason not to.

The only other case I can think of is when an element starts out on the boundary of the root element (as opposed to "transitioning from a non-intersecting state"). According to the spec such elements should be isIntersecting === true and intersectionRatio === 0. Do you know if the native Edge implementation reports this as 0?

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

I just wrote a small repro to test Edge's behavior vs Chrome, as well as Firefox with IntersectionObserver enabled in about:config.

TL;DR: In the case of Edge, yes the polyfill of using intersectionRatio > 0 for isIntersecting does work, because Edge isn't fully spec-compliant (with step 7 of this part of the spec) in that it doesn't emit an event for elements that start out exactly edge-adjacent. So I have another demo showing that this minimal polyfill does indeed work:

  if ('IntersectionObserver' in window &&
    'IntersectionObserverEntry' in window &&
    'intersectionRatio' in window.IntersectionObserverEntry.prototype &&
    !('isIntersecting' in IntersectionObserverEntry.prototype)) {

      Object.defineProperty(window.IntersectionObserverEntry.prototype, 'isIntersecting', {
        get: function () {
          return this.intersectionRatio > 0
        }
      })
  }

Unfortunately, though, there appear to be a number of inconsistencies between Chrome, Edge, and Firefox's implementations, as you can see in this screenshot:

2017-06-16 14_41_36-127 0 0 1_9033

Non-matching behavior:

  1. Firefox emits events out of order
  2. Chrome emits events for all elements, Edge only emits events for the elements that are immediately intersecting (not including edge-adjacent), and Firefox only emits events for elements that are immediately intersecting (including edge-adjacent).
  3. Edge and Firefox do not have intersectionRatio

I'm not sure if this is something that should be hammered out at the spec level or not. There also appear to be no web-platform-tests for IntersectionObserver, so perhaps this would be a good opportunity to write some. I'm gonna go talk to our dev team about what to do about this; may also be good to alert Firefox folks to potential compat issues.

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

OK I stand corrected on web-platform-tests; Gecko and Chromium have open PRs for it. (web-platform-tests/wpt#4384 web-platform-tests/wpt#6216)

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 19, 2017

Just checked Firefox Developer Edition (55.0b2), and it aligns more closely with Chrome. It appears the only difference is in the ordering of events. (Note box6.)

2017-06-19 09_31_37-intersectionobserver demo - firefox developer edition

My team is working on the isIntersecting fix and should hopefully have it soon in an upcoming release of Edge.

@tobytailor

This comment has been minimized.

Copy link

commented Jun 19, 2017

Making sure notifying in the same order as observers in Firefox has been worked on in https://bugzilla.mozilla.org/show_bug.cgi?id=1359311. We had some stability issues with related code in the past, thats why we decided to do some extra rounds of testing before landing that patch, which might lead to some delays here. But I expect it to land soonish.

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

Good to know. FWIW we discovered another potential interop issue around firing events when isIntersecting has not changed (due to edge-adjacency) but intersectionRatio has. (See demo and screenshot below.) We're currently investigating whether or not this is unspecced behavior and whether we should align with Chrome.

2017-06-21 09_48_41-intersectionobserver event demo - microsoft edge

@philipwalton

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

@nolanlawson with these new interop issues, has your thinking changed on the polyfill updates for isIntersecting, or do you think we should go ahead with that?

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

@philipwalton I'm in the process of filing a lot of bugs and working out what direction we want to go in. Expect my team to open up some spec issues. 馃槂

As for isIntersecting, the best polyfill IMO is the one I cited in #211 (comment). For Edge 15 this polyfill will Do The Right Thing鈩, and then when Edge 16 hits you'll get an isIntersecting that looks more like Chrome's implementation. For the other compat problems, I'll follow up in separate bugs.

@nolanlawson

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

I forked the compat discussion into #222. Let's keep this thread about the polyfill for isIntersecting. Sorry for derailing. 馃槄

sideshowbarker added a commit to saschanaz/IntersectionObserver that referenced this issue Mar 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.