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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

need notifications when observe is called on offscreen elements #165

Closed
ojanvafai opened this issue Oct 25, 2016 · 18 comments
Closed

need notifications when observe is called on offscreen elements #165

ojanvafai opened this issue Oct 25, 2016 · 18 comments

Comments

@ojanvafai
Copy link
Collaborator

Right now, we assume that an element is not visible when observe is called, queue up a run of the rendering pipeline, and then a callback gets delivered if the element turns out to be visible. There are use cases where the author content wants to assume the opposite, that the element is visible and to get a callback if it's hidden.

There's a couple options to solve this:

  1. Add a ternary option to the constructor. assume:visible|hidden|both or something like that.
  2. Just always have it fire the callback once whenever observe is called.
    Provide Microsoft Edge-team feedback on PositionObserver #2 seems simplest. I suspect the overhead of delivering the callbacks once to not be too much.

@szager-chromium @esprehn

@szager-chromium
Copy link
Collaborator

I think #2 makes sense.

MXEBot pushed a commit to mirror/chromium that referenced this issue Feb 1, 2017
Generate a notification for the initial state of a target, even if
it's offscreen.  This implements a change to the spec:

w3c/IntersectionObserver#165

[email protected],[email protected]
BUG=

Review-Url: https://codereview.chromium.org/2645283008
Cr-Commit-Position: refs/heads/master@{#447299}
@ziyunfei
Copy link

ziyunfei commented Feb 1, 2017

@jetvillegas
Copy link

The danger here is noted in the first comment on the Firefox bug above ^

Scripts that lazy-load on notification may fire prematurely based on that first notification. I'm inclined not to fix it in Firefox since this is an Ad-perf feature and the lazy-load issue will not be a good effect. Since Chrome is shipping it this way now, can you cite public cases that count on this behavior?

@tantek
Copy link
Member

tantek commented May 17, 2017

Indeed, assuming the use-case referred to in the issue description, presumably those users are now using this? Could you (@ojanvafai) provide URLs to actual examples that are using the Chrome implementation? Or if they're waiting on other implementations that would be good to know too.

@szager-chromium
Copy link
Collaborator

szager-chromium commented May 17, 2017

I'm trying to remember where the original request came from, but I believe the use case was that someone needed a "one-shot" observer, something along the lines of:

var newElement = foo();
var observer = new IntersectionObserver(function(changes) {
  doSomethingInterestingBasedOnInitialVisibility(newElement, changes.pop().isIntersecting);
  observer.unobserve(newElement);
});
observer.observe(newElement);

I will chase down Ojan and see if he remembers more, or where the request came from.

@a2sheppy
Copy link

What about if you're constructing a page which is performing tasks when elements become visible or not-visible, and you don't know whether an element is visible at the time it's added to the page or not? Getting this notification after observe() would let you easily handle that initial state properly.

@szager-chromium
Copy link
Collaborator

szager-chromium commented May 18, 2017

Right -- without the initial notification, there's no way to tell definitively that a newly-added element is not visible, unless you use some gymnastics like this:

var newElement = foo();
var gotNotification = false;
var observer = new IntersectionObserver(function(changes) { gotNotification = true; });
observer.observe(newElement);
requestAnimationFrame(function() {
  // If newElement is visible in the upcoming frame, observer will generate a notification.
  setTimeout(function() {
    // If newElement is initially visible, this is *still* too early to have received a notification.
    setTimeout(function() {
      // If newElement were initially visible, the observer callback would already have run by now.
      regisiterInitialVisibility(newElement, gotNotification);
      observer.unobserve(newElement);
    });
  });
});

That's super gross.

@jetvillegas
Copy link

I get the use case. I'm looking to understand who's using it.

To be clear, we 're about to ship IntersectionObserver without this fix (just as Chromium did for several versions before 58.) There's a lot of resistance to any code changes at this point. I'm looking to determine what will break if we ship, and if we should ship at all.

@jakub300
Copy link

Hey Jet,
This change broke two websites that I was working on. In one case I use IntersectionObserver to apply slide in effect when element enters the screen, in another case I use it to detect elements that are on screen to apply parallax to them.
As a front end developer I really appreciate your hesitance to make this backward incompatible change. Unfortunately Chrome team didn't had it and everything that could be broken by this already has been and to be honest at this point discussion about breaking seems meaningless.

@jetvillegas
Copy link

At this point, I'm inclined to ship the behavior that we've got, since all the feedback I've heard from actual usage is that it broke sites. If enough people report that this broke their sites, we may see this get reverted in Chromium too.

@jetvillegas
Copy link

...or, if it turns out we need to match the Chromium behavior, we'll fix it in the next release.

@szager-chromium
Copy link
Collaborator

I'm still trying to dig up the original request(s) for this, but I want to add some $.02 first.

When we first considered making this change, we realized that it would be disruptive to existing users of the API. However, we decided that since the API was still quite new, and only supported in Chrome, that if there was ever a time to make the change, the sooner the better.

After rolling out the change, I did field a number of bugs from developers, and helped them fix their code (which was pretty simple to do, once they understood the change). Nevertheless, it was certainly disruptive.

I am reluctant to revert the behavior now, because it's useful, and because I would rather not inflict this on developers again. I also really don't want this to be a compat issue, so one way or another, I'd very much like Mozilla and Chrome to align on this.

@mstange
Copy link

mstange commented May 18, 2017

I'm inclined to agree - now that the switch has been made, I think it's more risky to ship the old behavior than the new behavior.

@a2sheppy
Copy link

I'd like to add another comment here: I'm actively writing code that uses Intersection Observer and this is really causing me a headache. It's a simple example which creates a few fake "articles" with fake ads interspersed here and there. The goal is to track how much time each ad spends at least 75% visible.

The problem I run into is that since the observer doesn't get called when I first observe a newly-inserted ad, my code that turns timing of the visibility on and off doesn't run. I'm having to try to jury-rig a bit of code to check to see if the ad is visible upon initial insertion and then configure the timer if it's visible. If the observer was getting called when a newly observed element is visible, I'd be able to do this work all in the same place for all situations, which would be much more efficient.

@szager-chromium
Copy link
Collaborator

@a2sheppy Just so I understand: are you only interested in the visible time, or are you also interested in the not-visible time?

If you're only interested in the visible time, then presumably the old behavior (i.e., no automatic first notification) would work: you could just wait to set up the timer until the observer callback fires.

But if you're interested in measuring the time from when the ad first gets painted to when it first becomes visible, then the initial notification is important.

@a2sheppy
Copy link

(Had a brain misfire and deleted a bogus comment -- something that makes actual sense -- I hope -- follows)...

I have logic that needs to run for every ad the first time it's looked at by the DOM, to record the time at which it was first "known" by the DOM, and to update some values used for handling later drawing work.

And I can actually imagine the amount of time an ad is on the page but not visible as being a useful statistic to track; it could be helpful in figuring out best ad placement for calculating ad rates and the like.

At any rate, I agree that this change makes sense and that since Chrome has already done it, we ought to go ahead and do it in Firefox too, as soon as we can. The very fact that there are definite legitimate use cases and that the API is quite new makes this change worthwhile and makes this the least disruptive time to do it.

@ojanvafai
Copy link
Collaborator Author

Like @szager-chromium I don't know of specific sites that break. We got a lot of developer feedback about use cases that needed to know things like @a2sheppy's example of tracking the time an ad is not visible or for getting the initial state of an element right (e.g. if it is supposed to throttle when not visible).

@jakub300 sorry we broke you! We try to be careful and deliberate about backwards incompatible changes, but it's hard.

@jetvillegas
Copy link

Closing the loop here with a fix in Firefox, per spec:
https://bugzilla.mozilla.org/show_bug.cgi?id=1335644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants