w3c / IntersectionObserver

Intersection Observer
https://www.w3.org/TR/intersection-observer/
Other
3.62k stars 523 forks source link

What are the use cases of `takeRecords`? #133

Closed philipwalton closed 2 years ago

philipwalton commented 8 years ago

In the following code, which console.log statement should log populated records first?

var io = new IntersectionObserver(function(records) {
  console.log('callback', records);
});
io.observe(someTargetAlreadyVisible);
console.log('takeRecords', io.takeRecords());

The way it's currently implemented in Chrome, takeRecords returns an empty array here, so the callback logs populated records first. And this doesn't just happen after the initial .observe call. It even happens if you later do:

someTargetAlreadyVisible.moveOutOfView();
console.log('takeRecords', io.takeRecords()); // returns [];

Is this the intended behavior? Will this change after https://crbug.com/612323 makes it in?

To ask a broader question, should takeRecords run synchronously (i.e. force a lookup at that moment)? If it doesn't, and if you can't reliably call takeRecords when you know there are intersections (and thus theoretically records available), what's the point of the method? What are its real use cases?

And to ask a more specific question, if there's no way to ensure that io.takeRecords runs before the callback, how can you reliably test that it's working?

szager-chromium commented 8 years ago

It's important to understand that the timing of when IO generates notifications is tightly bound to frame generation. The only way to "force run" an IO observation is with requestAnimationFrame; and even then, the observation will happen after the requestAnimationFrame handler returns.

The purpose of takeRecords() is to receive any pending, already-generated IO observations without waiting for idle callbacks to run. For example:

function moveStuffAround() { changeLayout(); requestAnimationFrame(() => { // IO notifications are generated after this method returns. setTimeout(() => { // IO notifications have been generated, but idle handlers have not yet run. // Let's be proactive! var changes = observer.takeRecords(); }); }); }

The most likely application for takeRecords() is in click jack detection, where the clicked-on element needs up-to-the-frame information about its visibility to determine whether the click is legitimate:

var clickable = document.getElementById("clickable"); clickable.onclick = (() => { // Get any IO notifications that are waiting for an idle period to be delivered. var changes = observer.takeRecords(); if (stillVisibleAfterChanges(changes)) handleClick(); else reportClickJack(); });

There may also be other latency-sensitive applications that need takeRecords.

szager-chromium commented 8 years ago

As a practical matter for writing tests, you probably want to use this paradigm:

var changes = []; var observer = new IntersectionObserver((newChanges) => { changes.push(...newChanges) }); observer.observe(target); changeLayoutToGenerateAnIntersectionEvent(); requestAnimationFrame(() => { setTimeout(() => { changes.push(...observer.takeRecords()); checkForExpectedNotifications(changes); }) });

philipwalton commented 8 years ago

Having to do nested requestAnimationFrame + setTimeout calls just to get the records at the fastest possible moment is a pretty undesirable API, not to mention the fact that it will be an education nightmare to explain why this is to web developers.

A much simpler API would be to have takeRecords accept a callback, and then the IntersectionObserver would invoke the callback at the soonest possible moment (this would also not tie it so much to implementation details). You'd also want some guarantees spec'ed to say something like: any takeRecords calls are guaranteed to run before the next time the callback is invoked. Otherwise you get back to the original problem -- where a developer can't confidently say which method will get back results first.

Re:

The most likely application for takeRecords() is in click jack detection, where the clicked-on element needs up-to-the-frame information about its visibility to determine whether the click is legitimate:

I'm not sure this is a real use-case (unless I'm missing something). DOM event handlers should typically be synchronous because you can't call things like event.preventDefault() or event.stopPropagation() in a different call stack, so I don't see how you could prevent clickjacking with your suggested approach.

philipwalton commented 8 years ago

Also, just FYI, I'm using the following code to test the takeRecords method, and about 25% of the time it fails (meaning records.length is 0).

var io = new IntersectionObserver(noop);
io.observe(target);

requestAnimationFrame(function() {
  setTimeout(function() {
    var records = io.takeRecords();
    expect(records.length).to.be(1);
  }, 0);
});

Are there other things that factor into when the records are available?

szager-chromium commented 8 years ago

There are really no use cases for nested requestAnimationFrame/setTimeout, except for testing. Consumers should never need to do that.

I think you may be misunderstanding this API a bit. Since notifications are generated during frame construction, they are a very accurate indicator of what was on-screen at any given time, i.e., jank due to long-running javascript will not cause them to be inaccurate. However, the API is intentionally not designed to deliver the notifications at the earliest possible time -- that is why it uses an idle callback rather than simply posting a task (a la setTimeout(callback, 0)).

If you really truly want notifications at the earliest convenience, then you would probably put takeRecords() in a requestAnimationFrame loop:

function consume(changes) { doSomething(observer.takeRecords()); requestAnimationFrame(consume); } requestAnimationFrame(consume);

However, that idiom would defeat the purpose of IntersectionObserver, which is to get very accurate information about visibility events, but deliver them asynchronously at a time when the callback code will not jank the page.

I don't understand what you said about the clickjack-prevention scenario not working. When the onclick code runs, takeRecords() is meant to answer the question: "Were there any recent visibility events, up to and including the most recent frame show on screen?" Based on the answer to that question, you can determine whether the clicked object was visible at the time the click event happened. I don't understand your point about different call stacks.

As for the 25% failure rate: is that with the polyfill or the native implementation? It's impossible to know for certain when idle tasks will run, so it's possible that the notification is delivered via idle callback before your setTimeout code runs. That's why I suggested having the observer callback and the takeRecords call both append to the same array:

var changes = []; var observer = new IntersectionObserver((newChanges) => { changes.push(...newChanges) }); observer.observe(target); requestAnimationFrame((newChanges) => { setTimeout(() => { // Maybe the notifications have already been delivered to "changes", maybe not. expect(observer.takeRecords().length + changes.length).to.be(1); }, 0); });

philipwalton commented 8 years ago

@szager-chromium, I don't think I'm misunderstanding this API. However, I do think we're kind of talking past each other here, so let me try to reiterate my original question so I can make sure you (or anyone else reading) is fully understanding what I'm asking.

I'm not asking asking for advice on how to use the takeRecords method. I'm also not asking how to detect changes as soon as possible (that was just one example of a possible use case). I'm simply posing the question: how useful is the takeRecords method as currently spec'ed?, and I'm suggesting that perhaps it could be improved to be more useful to web developers and support a wider range of use cases.

As you've mentioned in this thread, we never know when the callback will fire, so for any given piece of JavaScript code, calling takeRecords could return records, or it could return an empty array. Which means the fact that takeRecords returned an empty array in a given situation is not -- in and of itself -- actionable information.

If the goal of the takeRecords method is to get the most up-to-date intersection information for a particular element, and if takeRecords isn't useful on its own (i.e. it's only useful in conjunction with the callback), then perhaps it's worth considering a new method that handles this use case and doesn't require manual coordination between the callback and the takeRecords method.

In other words, what's the information that developers really want? And then how can this API give them access to that in the cleanest/easiest way possible.

philipwalton commented 8 years ago

As for the 25% failure rate: is that with the polyfill or the native implementation? It's impossible to know for certain when idle tasks will run, so it's possible that the notification is delivered via idle callback before your setTimeout code runs.

It's with the native implementation. But as I'm updating the polyfill, I'm trying to write test cases that run against both the polyfill and the native implementation, and with the takeRecords methods, I haven't found a way to reliably get it to return results, so it's difficult/impossible to test.

For example, I put together this codepen demo that calls the takeRecords methods in a continuous requestAnimationFrame loop, and even with this continuous loop, you'll notice that far more often than not, the records are reported in the callback and not via the takeRecords method.

If you have any suggestions for how to reliably get the takeRecords method to return results, I'd love to hear them.

szager-chromium commented 8 years ago

It seems like what you're after is a way to say, "The next time the observer generates notifications, please give them to me right away; I don't want to wait for an idle callback." And my response to that is: it defeats the purpose of using an idle callback with a 100ms timeout. If you want more synchronous access to intersection information, you can get it using existing methods (like forcing synchronous layout in a requestAnimationFrame handler), and IntersectionObserver won't be an improvement over those methods.

The purpose of takeRecords is not to give you a callback ASAP as soon as notifications are generated. If that was the purpose, then we would simply use setTimeout(callback, 0) to deliver. Instead, the purpose of takeRecords is: you're already running user code, and you want to get immediate information about notifications that have already been generated. The canonical use case for this is validating an input gesture, though there may be others. But if you're trying to use takeRecords to receive notifications synchronously as they are generated, then you're barking up the wrong tree.

szager-chromium commented 8 years ago

And again: my best guess as to why you're seeing flaky behavior is that you can't rely on the timing of idle callbacks running. You should be inspecting both changes reported to the callback and changes received from takeRecords, we can't guarantee than one will happen before the other.

ziyunfei commented 8 years ago

This method is confusing.

szager-chromium commented 8 years ago

I think the conversation on this bug is much more confusing than the actual feature. I will try to summarize:

IntersectionObserver notifications are delivered asynchronously, using the same algorithm as requestIdleCallback (https://w3c.github.io/requestidlecallback/#the-requestidlecallback-method).

The IntersectionObserverEntry objects have very accurate timestamps, but there will be a delay of up to 100ms between when the entries were generated and when the callback runs. For the most common use cases (e.g., recording ad impressions), that's fine.

But for some less common use cases (e.g., checking the visibility of a control element when it receives a click), the delay between when the entries are generated and when the callback runs is unacceptable. For those use cases, takeRecords() will synchronously get any IntersectionObserverEntry objects that have been generated but not yet delivered.

alijuma commented 6 years ago

Given that IntersectionObserver notifications are now delivered by posting a task rather than using requestIdleCallback, are there still good use cases for takeRecords?

jdfreder commented 6 years ago

We use takeRecords to confirm that the measurement hasn't changed after X elapsed milliseconds.

For example, lets say we want to notify someone when an element has been 100% visible for 3 seconds. Intersection observer is great for telling us that the element has become visible, but doesn't tell us that it's still visible. Simply setting a timeout for 3 seconds and confirming that we haven't received observations doesn't work, since observations may be pending. To fix this, in addition we call takeRecords to ensure that there are no pending observations, effectively making sure that our last observation holds true at NOW (versus NOW - milliseconds elapsed since last pending observation).

Are you saying that this is no longer needed because there's no delay between observation measurement and observation event firing?

alijuma commented 6 years ago

Thanks, that sounds like good use case.

The delay now is much smaller than before -- the task to fire the event will be added to the event loop immediately, so the delay will be only be from other tasks already posted to the event loop. If, in your setTimeout callback, you set another timeout for 0 seconds, then by the time that second setTimeout fires, any observation events that were pending during the first setTimeout will have fired. But I can see how this might be more awkward than just calling takeRecords during the initial setTimeout callback.

miketaylr commented 2 years ago

It sounds like there are a handful of use cases listed here, so I propose we close this issue.