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

Add intersection observer to the event loop's rendering #708

Merged
merged 1 commit into from
Feb 27, 2016

Conversation

domenic
Copy link
Member

@domenic domenic commented Feb 18, 2016

This incorporates the monkey-patch from the intersection observer spec
directly into HTML, per
w3c/IntersectionObserver#94.

As a drive-by while doing so, changed some nearby steps to link to the
algorithms in [CSSOMVIEW], instead of pretending to be the defining
instance of these terms. See also #707.


This cannot be merged until the IO spec makes some updates to the referenced algorithm; see w3c/IntersectionObserver#94 for details. However, it is ready for review.

@domenic domenic added addition/proposal New features or enhancements do not merge yet Pull request must not be merged per rationale in comment labels Feb 18, 2016
@Ms2ger
Copy link
Member

Ms2ger commented Feb 19, 2016

Is this a spec that has wide support already? I'm a little afraid of this step being used as an argument to push browsers to implement this spec with no further review whether that'd be desirable.

@domenic
Copy link
Member Author

domenic commented Feb 19, 2016

@Ms2ger that was my concern too, but I was assured by @ojanvafai that there's general support. He can give more details. I don't think anyone would take this as a recommendation to not review the spec before implementing it though; this is just a small hook. And "update the rendering" already has web animations, which to me seems to be in a similar position.

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Feb 25, 2016
@domenic
Copy link
Member Author

domenic commented Feb 25, 2016

This is ready for review/merge now that the IO spec has been updated.

@annevk
Copy link
Member

annevk commented Feb 26, 2016

So what about defining "render tasks" in #707? The hooks given by the specification today are not super useful since everyone will have to define their own queue. We could instead generalize the animation frame queue to be a per-feature queue so specifications only need to queue render tasks of a certain type that HTML then takes care of invoking.

@domenic
Copy link
Member Author

domenic commented Feb 26, 2016

Seems like a reasonable refactoring step for later, that should not block this? Although I am not sure it buys us much; HTML still has to define the relative ordering between the different types of render tasks.

@annevk
Copy link
Member

annevk commented Feb 26, 2016

It does not buy HTML much, but it gives everyone else (10 dependencies or so) an easy integration system. I guess it's fine to merge this, hopefully the intersection observer folks are happy to refactor later on too.

@domenic
Copy link
Member Author

domenic commented Feb 26, 2016

I guess it's fine to merge this, hopefully the intersection observer folks are happy to refactor later on too.

Thanks

It does not buy HTML much, but it gives everyone else (10 dependencies or so) an easy integration system.

Hmm, how so? They still have to pull request HTML to say "run the XYZ render tasks". I guess they don't have to maintain their own queue, since we abstract out that boilerplate for them? But I'm not sure that would help with IOs, for example, as their queue is nontrivial and depends on idle timing etc.

@annevk
Copy link
Member

annevk commented Feb 26, 2016

Does IO even integrate with HTML? It queues "idle request callbacks"? That setup looks very broken. Especially queuing tasks as callbacks without defining what enqueue or task means there. That timeout variable set to 100 also appears to go nowhere.

@domenic
Copy link
Member Author

domenic commented Feb 26, 2016

Hmm, that is a good point. Will have to ask them.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Feb 26, 2016
@domenic
Copy link
Member Author

domenic commented Feb 26, 2016

OK. Upon re-reading I see that I just linked to the wrong thing. IOs work as follows:

  • Every frame, they do intersection computations. That is what this PR is about.
  • If the intersection computation reveals a change in intersections, then it queues an intersection observer task, which is done via the separate idle queue.

I agree that the integration with the idle task spec is not perfect. But I think that is a separate issue (that I will file on them). The integration with HTML, as in this PR, seems good.


Regarding moving to rendering tasks. I am still not sure it would help, since the IO intersection checking is supposed to happen every frame. There's not much value in using a queue of rendering tasks if you're trying to perform a single operation every frame.


I'll leave this to you to give final LGTM since it got confusing toward the end. But I think everything is OK now and this is mergeable.

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Feb 26, 2016
This incorporates the monkey-patch from the intersection observer spec
directly into HTML, per
w3c/IntersectionObserver#94.

As a drive-by while doing so, changed some nearby steps to link to the
algorithms in [CSSOMVIEW], instead of pretending to be the defining
instance of these terms. See also #707.
@annevk annevk merged commit a5f708c into master Feb 27, 2016
@annevk annevk deleted the io-integration branch February 27, 2016 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

Successfully merging this pull request may close these issues.

3 participants